Re: [HACKERS] Spaces in psql output (Was: FW: PGBuildfarm

> -----Original Message-----
> From: Marko Kreen [mailto:markokr(at)gmail(dot)com]
> Sent: 10 February 2006 15:07
> To: Dave Page
> Cc: pgsql-hackers(at)postgresql(dot)org
> Subject: Re: [HACKERS] FW: PGBuildfarm member snake Branch
> HEAD Status changed from OK to ContribCheck failure
>
> On 2/10/06, Dave Page <dpage(at)vale-housing(dot)co(dot)uk> wrote:
> > Something broke snake again :-(. Looks like tsearch2
> through the haze of
> > my Lemsip... I hate winter :-(
>
> AFAIR the reason for different length of psql's '----' header lines
> was database encoding being UNICODE not SQL_ASCII. Can this
> be the case
> there?

Oh, didn't spot those (barely functioning at all today though!).

No-one has even logged into Snake for a week or more though, so I would
still suspect a source change as the culprit.

Regards, Dave.

The failure, I think, it because of the newline patch we got for psql
yesterday. I am seeking a diff from pgcrypto to fix it. My openssl is
too old.

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

Dave Page wrote:
>
>
> > -----Original Message-----
> > From: Marko Kreen [mailto:markokr(at)gmail(dot)com]
> > Sent: 10 February 2006 15:07
> > To: Dave Page
> > Cc: pgsql-hackers(at)postgresql(dot)org
> > Subject: Re: [HACKERS] FW: PGBuildfarm member snake Branch
> > HEAD Status changed from OK to ContribCheck failure
> >
> > On 2/10/06, Dave Page <dpage(at)vale-housing(dot)co(dot)uk> wrote:
> > > Something broke snake again :-(. Looks like tsearch2
> > through the haze of
> > > my Lemsip... I hate winter :-(
> >
> > AFAIR the reason for different length of psql's '----' header lines
> > was database encoding being UNICODE not SQL_ASCII. Can this
> > be the case
> > there?
>
> Oh, didn't spot those (barely functioning at all today though!).
>
> No-one has even logged into Snake for a week or more though, so I would
> still suspect a source change as the culprit.
>
> Regards, Dave.
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster
>

--
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 wrote:

>
> The failure, I think, it because of the newline patch we got for psql
> yesterday. I am seeking a diff from pgcrypto to fix it. My openssl is
> too old.

A side affect of this newline patch is that all fields are now filled with
white space up to the displayed column width, even for the last (or only
column). I guess this is somehow required for consistent display. Otherwise,
it makes copy&paste from a psql session more painful, because of all the
added white-space. I hope that psql output intended for script output using
the available flags (i.e. not the "nice display" output) is unaffected?

Best Regards,
Michael Paesold

On Fri, Feb 10, 2006 at 08:06:53PM +0100, Michael Paesold wrote:
> A side affect of this newline patch is that all fields are now filled with
> white space up to the displayed column width, even for the last (or only
> column). I guess this is somehow required for consistent display.
> Otherwise, it makes copy&paste from a psql session more painful, because of
> all the added white-space. I hope that psql output intended for script
> output using the available flags (i.e. not the "nice display" output) is
> unaffected?

My intention was to only change formatted output. Unformatted should be
unchanged from previous.

The extra spaces is an interesting side-effect. In the past it would
only have worked for the last column anyway, right?

Anyway, it is a fixable issue and I'd consider doing it if people think
it's worth it.

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 wrote:

> On Fri, Feb 10, 2006 at 08:06:53PM +0100, Michael Paesold wrote:
> > A side affect of this newline patch is that all fields are now filled
> > with
> > white space up to the displayed column width, even for the last (or only
> > column).
>
> My intention was to only change formatted output. Unformatted should be
> unchanged from previous.
>
> The extra spaces is an interesting side-effect. In the past it would
> only have worked for the last column anyway, right?

Right, my explanation was not correct. It should have been "the last column
is now also filled with spaces". Of course all but the last column were
always filled with spaces.

> Anyway, it is a fixable issue and I'd consider doing it if people think
> it's worth it.

I personally don't like the added spaces (feels inefficient), but that is
only a matter of taste, so you can rather ignore it.
I am not sure about people who perhaps rely on the output format in scripts
(even if it's bad to rely on that specific output format, because the output
of psql can be changed to be more suitable for scripts). For multi-column
output things have not really changed anyway.

Best Regards,
Michael Paesold

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> The extra spaces is an interesting side-effect. In the past it would
> only have worked for the last column anyway, right?

Of course.

> Anyway, it is a fixable issue and I'd consider doing it if people think
> it's worth it.

I think it would be a good idea to expect this patch to cause zero
change in psql output except in the cases where there are actually
control characters in the data. Otherwise there are likely to be
complaints. (I'm already unhappy at the prospect that this means
every single regression test's output has changed, even if diff
--ignore-space is hiding them.)

I'd settle for stripping trailing blanks on the last line of a multiline
field value, if that'd be any easier than stripping them for all lines.

regards, tom lane

On Fri, Feb 10, 2006 at 03:21:47PM -0500, Tom Lane wrote:
> I think it would be a good idea to expect this patch to cause zero
> change in psql output except in the cases where there are actually
> control characters in the data. Otherwise there are likely to be
> complaints. (I'm already unhappy at the prospect that this means
> every single regression test's output has changed, even if diff
> --ignore-space is hiding them.)
>
> I'd settle for stripping trailing blanks on the last line of a multiline
> field value, if that'd be any easier than stripping them for all lines.

Well, the attached patch removes the padding on the last column,
irrespective of the line. It will pad all the way to the end if the
cell is empty due to one of the earlier columns being multiline.

I did it this way because it's not always straight forward to know when
you're on the last line. In any case, this should bring all the
regression output back to the way it was. I didn't realise the
regression diffs ignored spaces...

As to the way the code is done, I prefer seperating out the test into a
variable because fitting it all on a single line is messy. OTOH, some
people discourage the use of ?: but I prefer it to a whole if
statement.

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:
> As to the way the code is done, I prefer seperating out the test into a
> variable because fitting it all on a single line is messy. OTOH, some
> people discourage the use of ?: but I prefer it to a whole if
> statement.

I don't object to that, but I do object to declaring bool variables
as int ...

regards, tom lane

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> Well, the attached patch removes the padding on the last column,
> irrespective of the line. It will pad all the way to the end if the
> cell is empty due to one of the earlier columns being multiline.

Applied with some cosmetic changes. I've confirmed that all the
regression tests pass with "-w" removed from DIFFFLAGS ... which
was a good exercise, actually, because it exposed the fact that
the line-wrapping patch broke ReportSyntaxErrorPosition. I've put a
band-aid on that, though it might be worth trying to make it smarter
about control characters in general.

regards, tom lane

On Sat, Feb 11, 2006 at 10:42:59PM -0500, Tom Lane wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > Well, the attached patch removes the padding on the last column,
> > irrespective of the line. It will pad all the way to the end if the
> > cell is empty due to one of the earlier columns being multiline.
>
> Applied with some cosmetic changes. I've confirmed that all the
> regression tests pass with "-w" removed from DIFFFLAGS ... which
> was a good exercise, actually, because it exposed the fact that
> the line-wrapping patch broke ReportSyntaxErrorPosition. I've put a
> band-aid on that, though it might be worth trying to make it smarter
> about control characters in general.

Thanks for putting so much effort into this. I wish the patch could
have had more feedback before it was applied but that's water under the
bridge.

About the ReportSyntaxErrorPosition thing, it was considered at the
time (can't find the email right now) that control characters would be
problematic but it wouldn't break anything that wasn't broken already.
How tab was missed I don't know, nor a couple of other failures cases
I've thought of since. Thanks for fixing that.

There's still the idea of automatically wrapping wide columns, but
that doesn't appear to have gained any traction yet so I'll leave that
for now.

Thanks again,

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:
> About the ReportSyntaxErrorPosition thing, it was considered at the
> time (can't find the email right now) that control characters would be
> problematic but it wouldn't break anything that wasn't broken already.
> How tab was missed I don't know, nor a couple of other failures cases
> I've thought of since. Thanks for fixing that.

What I was thinking of doing was making it translate any control
character (other than \r and \n) to a space, not only tab. However
this should probably wait on the result of the discussion about whether
we really like the \x09 output for tabs in data ... that might yield an
idea that could be applied here too.

regards, tom lane

Tom Lane wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > About the ReportSyntaxErrorPosition thing, it was considered at the
> > time (can't find the email right now) that control characters would be
> > problematic but it wouldn't break anything that wasn't broken already.
> > How tab was missed I don't know, nor a couple of other failures cases
> > I've thought of since. Thanks for fixing that.
>
> What I was thinking of doing was making it translate any control
> character (other than \r and \n) to a space, not only tab. However
> this should probably wait on the result of the discussion about whether
> we really like the \x09 output for tabs in data ... that might yield an
> idea that could be applied here too.

Perhaps we should just output "\009" for tab and "\xxx" for other
control characters. That seems the most natural. I can't see why we
would convert every control character to " ".

--
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 wrote:
> Tom Lane wrote:
> > Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > > About the ReportSyntaxErrorPosition thing, it was considered at the
> > > time (can't find the email right now) that control characters would be
> > > problematic but it wouldn't break anything that wasn't broken already.
> > > How tab was missed I don't know, nor a couple of other failures cases
> > > I've thought of since. Thanks for fixing that.
> >
> > What I was thinking of doing was making it translate any control
> > character (other than \r and \n) to a space, not only tab. However
> > this should probably wait on the result of the discussion about whether
> > we really like the \x09 output for tabs in data ... that might yield an
> > idea that could be applied here too.
>
> Perhaps we should just output "\009" for tab and "\xxx" for other
> control characters. That seems the most natural. I can't see why we
> would convert every control character to " ".

I assume everyone is happy with the next hex output so we will leave it:

test=> select '<tab>';
?column?
----------
\x09
(1 row)

FYI, as of 8.1 we now accept hex in all place we accept octal.

--
Bruce Momjian http://candle.pha.pa.us
SRA OSS, Inc. http://www.sraoss.com

+ If your life is a hard drive, Christ can be your backup. +