Thread: Re: [HACKERS] Spaces in psql output (Was: FW: PGBuildfarm member snake Branch HEAD Status changed)

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@svana.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@svana.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@svana.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@svana.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@svana.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@svana.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@svana.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@candle.pha.pa.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@svana.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. +