pgsql: Allow psql multi-line column values to align in the proper

Log Message:
-----------
Allow psql multi-line column values to align in the proper columns

If the second output column value is 'a\nb', the 'b' should appear
in the second display column, rather than the first column as it
does now.

Change libpq's PQdsplen() to return more useful values.

> Note: this changes the PQdsplen function, it can now return zero or
> minus one which was not possible before. It doesn't appear anyone is
> actually using the functions other than psql but it is a change. The
> functions are not actually documentated anywhere so it's not like we're
> breaking a defined interface. The new semantics follow the Unicode
> standard.

BACKWARD COMPATIBLE CHANGE.

The only user-visible change I saw in the regression tests is that a
SELECT * on a table where all the columns have been dropped doesn't
return a blank line like before. This seems like a step forward.

Martijn van Oosterhout

Modified Files:
--------------
pgsql/src/backend/utils/mb:
wchar.c (r1.52 -> r1.53)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/mb/wchar.c.diff?r1=1.52&r2=1.53)
pgsql/src/bin/psql:
mbprint.c (r1.18 -> r1.19)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/mbprint.c.diff?r1=1.18&r2=1.19)
mbprint.h (r1.8 -> r1.9)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/mbprint.h.diff?r1=1.8&r2=1.9)
print.c (r1.79 -> r1.80)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/bin/psql/print.c.diff?r1=1.79&r2=1.80)
pgsql/src/test/regress/expected:
alter_table.out (r1.89 -> r1.90)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/alter_table.out.diff?r1=1.89&r2=1.90)
prepare.out (r1.8 -> r1.9)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/expected/prepare.out.diff?r1=1.8&r2=1.9)

On Thu, 9 Feb 2006, Bruce Momjian wrote:

> Log Message:
> -----------
> Allow psql multi-line column values to align in the proper columns
>

You have not fixed the expected results for the contrib regression tests.

Kris Jurka

On Thu, 9 Feb 2006, Bruce Momjian wrote:

> Log Message:
> -----------
> Allow psql multi-line column values to align in the proper columns
>

There is a problem with this on AIX.

http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=asp&dt=2006-02-10%2006:23:00

For tables that have no columns AIX thinks it has an out of memory error.
src/bin/psql/print.c:pg_local_calloc bails out if calloc returns NULL. I
believe AIX is returning NULL for calloc with a count of zero. Efforts
are made not to call pg_local_calloc with a count of zero, but one place
is missed, the attached patch fixes that.

Additionally there are a whole lot of of signedness warnings introduced
which I've attached.

Kris Jurka

OK, I have applied your patch to psql and Teodor has adjusted the
tsearch2 expected results. I can't seem to run pgcrypto without getting
PRNG errors, so I expect my SSL is too old. Would you send me your
pgcrypto/regression.diff? Thanks.

---------------------------------------------------------------------------

Kris Jurka wrote:
>
>
> On Thu, 9 Feb 2006, Bruce Momjian wrote:
>
> > Log Message:
> > -----------
> > Allow psql multi-line column values to align in the proper columns
> >
>
> There is a problem with this on AIX.
>
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=asp&dt=2006-02-10%2006:23:00
>
> For tables that have no columns AIX thinks it has an out of memory error.
> src/bin/psql/print.c:pg_local_calloc bails out if calloc returns NULL. I
> believe AIX is returning NULL for calloc with a count of zero. Efforts
> are made not to call pg_local_calloc with a count of zero, but one place
> is missed, the attached patch fixes that.
>
> Additionally there are a whole lot of of signedness warnings introduced
> which I've attached.
>
> Kris Jurka

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> OK, I have applied your patch to psql and Teodor has adjusted the
> tsearch2 expected results. I can't seem to run pgcrypto without getting
> PRNG errors, so I expect my SSL is too old. Would you send me your
> pgcrypto/regression.diff? Thanks.

I fixed that already. Please see about fixing the compiler warnings you
introduced. On a 64-bit machine there are even more, and the int vs
size_t ones are definitely portability problems.

regards, tom lane

print.c: In function 'print_aligned_text':
print.c:408: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:428: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:479: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:588: warning: pointer targets in passing argument 1 of 'format_numeric_locale' differ in signedness
print.c:589: warning: field width should have type 'int', but argument 3 has type 'size_t'
print.c: In function 'print_aligned_vertical':
print.c:690: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:714: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:823: warning: pointer targets in passing argument 1 of 'format_numeric_locale' differ in signedness
print.c:827: warning: field width should have type 'int', but argument 4 has type 'size_t'
print.c: In function 'printQuery':
print.c:1753: warning: pointer targets in passing argument 1 of 'mbvalidate' differ in signedness
print.c:1753: warning: pointer targets in assignment differ in signedness
print.c:1764: warning: pointer targets in passing argument 1 of 'mbvalidate' differ in signedness
print.c:1764: warning: pointer targets in assignment differ in signedness
mbprint.c: In function 'pg_wcswidth':
mbprint.c:161: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:165: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c: In function 'pg_wcssize':
mbprint.c:194: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:197: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c: In function 'pg_wcsformat':
mbprint.c:260: warning: pointer targets in initialization differ in signedness
mbprint.c:264: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:267: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c:281: warning: pointer targets in assignment differ in signedness
print.c: In function 'print_aligned_text':
print.c:408: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:428: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:479: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:588: warning: pointer targets in passing argument 1 of 'format_numeric_locale' differ in signedness
print.c:589: warning: field width should have type 'int', but argument 3 has type 'size_t'
print.c: In function 'print_aligned_vertical':
print.c:690: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:714: warning: pointer targets in passing argument 4 of 'pg_wcssize' differ in signedness
print.c:823: warning: pointer targets in passing argument 1 of 'format_numeric_locale' differ in signedness
print.c:827: warning: field width should have type 'int', but argument 4 has type 'size_t'
print.c: In function 'printQuery':
print.c:1753: warning: pointer targets in passing argument 1 of 'mbvalidate' differ in signedness
print.c:1753: warning: pointer targets in assignment differ in signedness
print.c:1764: warning: pointer targets in passing argument 1 of 'mbvalidate' differ in signedness
print.c:1764: warning: pointer targets in assignment differ in signedness
mbprint.c: In function 'pg_wcswidth':
mbprint.c:161: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:165: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c: In function 'pg_wcssize':
mbprint.c:194: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:197: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c: In function 'pg_wcsformat':
mbprint.c:260: warning: pointer targets in initialization differ in signedness
mbprint.c:264: warning: pointer targets in passing argument 1 of 'PQmblen' differ in signedness
mbprint.c:267: warning: pointer targets in passing argument 1 of 'PQdsplen' differ in signedness
mbprint.c:281: warning: pointer targets in assignment differ in signedness

I think Tom fixed the final difference in pgcryto. We should be fine now.

---------------------------------------------------------------------------

Kris Jurka wrote:
>
>
> On Thu, 9 Feb 2006, Bruce Momjian wrote:
>
> > Log Message:
> > -----------
> > Allow psql multi-line column values to align in the proper columns
> >
>
> There is a problem with this on AIX.
>
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=asp&dt=2006-02-10%2006:23:00
>
> For tables that have no columns AIX thinks it has an out of memory error.
> src/bin/psql/print.c:pg_local_calloc bails out if calloc returns NULL. I
> believe AIX is returning NULL for calloc with a count of zero. Efforts
> are made not to call pg_local_calloc with a count of zero, but one place
> is missed, the attached patch fixes that.
>
> Additionally there are a whole lot of of signedness warnings introduced
> which I've attached.
>
> Kris Jurka

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, Feb 10, 2006 at 11:02:41AM -0500, Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > OK, I have applied your patch to psql and Teodor has adjusted the
> > tsearch2 expected results. I can't seem to run pgcrypto without getting
> > PRNG errors, so I expect my SSL is too old. Would you send me your
> > pgcrypto/regression.diff? Thanks.
>
> I fixed that already. Please see about fixing the compiler warnings you
> introduced. On a 64-bit machine there are even more, and the int vs
> size_t ones are definitely portability problems.

Ouch. I submitted this patch so long ago I figured it'd gotten lost. I
was actually considering preparing a new version against HEAD.

I'll try to address these warnings (unless someone beats me to it).
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Fri, Feb 10, 2006 at 11:02:41AM -0500, Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > OK, I have applied your patch to psql and Teodor has adjusted the
> > > tsearch2 expected results. I can't seem to run pgcrypto without getting
> > > PRNG errors, so I expect my SSL is too old. Would you send me your
> > > pgcrypto/regression.diff? Thanks.
> >
> > I fixed that already. Please see about fixing the compiler warnings you
> > introduced. On a 64-bit machine there are even more, and the int vs
> > size_t ones are definitely portability problems.
>
> Ouch. I submitted this patch so long ago I figured it'd gotten lost. I
> was actually considering preparing a new version against HEAD.

We loose nothing! :-)

> I'll try to address these warnings (unless someone beats me to it).

I am looking at it now but I can't find the compiler flag to get those
warnings. I am gcc 2.95.3.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Martijn van Oosterhout wrote:
>> I'll try to address these warnings (unless someone beats me to it).

> I am looking at it now but I can't find the compiler flag to get those
> warnings. I am gcc 2.95.3.

You'd need a newer compiler.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Martijn van Oosterhout wrote:
> >> I'll try to address these warnings (unless someone beats me to it).
>
> > I am looking at it now but I can't find the compiler flag to get those
> > warnings. I am gcc 2.95.3.
>
> You'd need a newer compiler.

gcc -pedantic is showing it to me.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

On Fri, Feb 10, 2006 at 12:09:13PM -0500, Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > Martijn van Oosterhout wrote:
> > >> I'll try to address these warnings (unless someone beats me to it).
> >
> > > I am looking at it now but I can't find the compiler flag to get those
> > > warnings. I am gcc 2.95.3.
> >
> > You'd need a newer compiler.
>
> gcc -pedantic is showing it to me.

Thanks for the tip. I'm currently merging CVS with my version and
getting a lot of conflicts (whitespace variations).

It's fairly simple changes AFAICS. Just need to fix the declarations of
a few variables.
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

On Fri, Feb 10, 2006 at 06:16:07PM +0100, Martijn van Oosterhout wrote:
> Thanks for the tip. I'm currently merging CVS with my version and
> getting a lot of conflicts (whitespace variations).
>
> It's fairly simple changes AFAICS. Just need to fix the declarations of
> a few variables.

Does PostgreSQL have a policy on the signedness of strings? When I was
making the patch I generally used "unsigned char" to try to emphasise
that these are not characters, they are bytes of an encoding. But this
doesn't play well with existing functions like PQdsplen and PQmblen.

At this point I'm thinking of dropping the "unsigned" everywhere and
just being really careful of comparisons. OTOH, it's really those two
functions being inconsistant because the underlying functions do use
unsigned. Thoughts?

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> Does PostgreSQL have a policy on the signedness of strings?

For the most part we say "char" where we can and "unsigned char" only
where it really matters, which is mostly inside code that's
encoding-aware anyway.

It was only fairly recently that we cleaned the code up to avoid
signedness warnings, but now that that's done I don't want to backtrack
on it. What I'd suggest is taking a close look at the set of functions
you have and trying to identify a layer that should be "unsigned char"
versus upper layers that can just say "char".

regards, tom lane

On Fri, Feb 10, 2006 at 02:13:26PM -0500, Tom Lane wrote:
> For the most part we say "char" where we can and "unsigned char" only
> where it really matters, which is mostly inside code that's
> encoding-aware anyway.

Well, I've done this and avoided changing any public interfaces. ie the
libpq interface remains unsigned, as does the psql formatting code, but
the printTable stuff only is for the parts that actually do
formatting...

Patch attached. Passes -pedantic on gcc 3.3.5

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> Patch attached. Passes -pedantic on gcc 3.3.5

Applied with some cosmetic cleanups and further fixes for 64-bit
problems.

regards, tom lane