Re: Problem with displaying "wide" tables in psql

Hi,

This is my review about 3th version of the patch. It is an useful
improvement in my opinion. It worked well on my environment.

2013-12-11 17:43:06, Sergey Muraviov <sergey(dot)k(dot)muraviov(at)gmail(dot)com>:
> It works in expanded mode when either format option is set to wrapped
> (\pset format wrapped), or we have no pager, or pager doesn't chop long
> lines (so you can still use the trick).

I do not like this logic on the IsWrappingNeeded function. It does not
seems right to check the environment variables for less. It would be hard
to explain this behavior to the users. It is better to make this only
the behavior of the wrapped format in expanded mode, in my opinion.

> {
> if (opt_border < 2)
> fprintf(fout, "%s\n", dlineptr[line_count].ptr);
> else
> fprintf(fout, "%-s%*s %s\n", dlineptr[line_count].ptr,
> dwidth - dlineptr[line_count].width, "",
> dformat->rightvrule);
> }

Is it necessary to keep this old print line code? It seems to me the new
code works well on (dlineptr[line_count].width <= dwidth) condition.

Hi.

Thanks for your review.

2014-02-15 20:08 GMT+04:00 Emre Hasegeli <emre(at)hasegeli(dot)com>:

> Hi,
>
> This is my review about 3th version of the patch. It is an useful
> improvement in my opinion. It worked well on my environment.
>
> 2013-12-11 17:43:06, Sergey Muraviov <sergey(dot)k(dot)muraviov(at)gmail(dot)com>:
> > It works in expanded mode when either format option is set to wrapped
> > (\pset format wrapped), or we have no pager, or pager doesn't chop long
> > lines (so you can still use the trick).
>
> I do not like this logic on the IsWrappingNeeded function. It does not
> seems right to check the environment variables for less. It would be hard
> to explain this behavior to the users. It is better to make this only
> the behavior of the wrapped format in expanded mode, in my opinion.
>

You are right. This logic is too complicated.
New patch works with PRINT_WRAPPED option only.

> > {
> > if (opt_border < 2)
> > fprintf(fout, "%s\n",
> dlineptr[line_count].ptr);
> > else
> > fprintf(fout, "%-s%*s
> %s\n", dlineptr[line_count].ptr,
> > dwidth -
> dlineptr[line_count].width, "",
> >
> dformat->rightvrule);
> > }
>
> Is it necessary to keep this old print line code? It seems to me the new
> code works well on (dlineptr[line_count].width <= dwidth) condition.
>

New code doesn't work with empty strings but I've done minor optimization
for this case.

--
Best regards,
Sergey Muraviov

2014-02-16 18:37, Sergey Muraviov <sergey(dot)k(dot)muraviov(at)gmail(dot)com>:

> New code doesn't work with empty strings but I've done minor optimization
> for this case.

It seems better now. I added some new lines and spaces, removed unnecessary
parentheses and marked it as "Ready for Committer".

Thanks.

2014-02-17 12:22 GMT+04:00 Emre Hasegeli <emre(at)hasegeli(dot)com>:

> 2014-02-16 18:37, Sergey Muraviov <sergey(dot)k(dot)muraviov(at)gmail(dot)com>:
>
> > New code doesn't work with empty strings but I've done minor optimization
> > for this case.
>
> It seems better now. I added some new lines and spaces, removed unnecessary
> parentheses and marked it as "Ready for Committer".
>

--
Best regards,
Sergey Muraviov

On Sat, Feb 15, 2014 at 11:08 AM, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote:
> This is my review about 3th version of the patch. It is an useful
> improvement in my opinion. It worked well on my environment.

I'm reviewing this patch.

One thing to comment:

With no doc changes and no regression tests I was halfway inclined to
just reject it out of hand. To be fair there were no regression tests
for wrapped output prior to the patch but still I would have wanted to
see them added. We often pare down regression tests when committing
patches but it's easier to pare them down than write new ones and it
helps show the author's intention.

In this case I'm inclined to expand the regression tests. We've had
bugs in these formatting functions before and at least I find it hard
to touch code like this with any confidence that I'm not breaking
things. Formatting code ends up being pretty spaghetti-like easily and
there are lots of cases so it's easy to unintentionally break cases
you didn't realize you were touching.

In addition there are several cases of logic that looks like this:

if (x)
..
else
{
if (y)
...
else
...
}

I know there are other opinions on this but I find this logic very
difficut to follow. It's almost always clearer to refactor the
branches into a flat if / else if / else if /.../ else form. Even if
it results in some duplication of code (typically there's some trivial
bit of code outside the second "if") it's easy to quickly see whether
all the cases are handled and understand whether any of the cases have
forgotten any steps.

--
greg

On 2014-04-08 12:15:47 -0400, Greg Stark wrote:
> With no doc changes and no regression tests I was halfway inclined to
> just reject it out of hand. To be fair there were no regression tests
> for wrapped output prior to the patch but still I would have wanted to
> see them added. We often pare down regression tests when committing
> patches but it's easier to pare them down than write new ones and it
> helps show the author's intention.

I don't think this is easily testable that way - doesn't it rely on
determining the width of the terminal? Which you won't have when started
from pg_regress?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Apr 8, 2014 at 12:19 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I don't think this is easily testable that way - doesn't it rely on
> determining the width of the terminal? Which you won't have when started
> from pg_regress?

There's a pset variable to set the target width so at least the
formatting code can be tested. It would be nice to have the ioctl at
least get called on the regression farm so we're sure we aren't doing
something unportable.

--
greg

Hi.

How can I pass or set the value of pset variable for an regression test?

The code with ioctl was copied from print_aligned_text function, which has
a long history.
43ee2282 (Bruce Momjian 2008-05-16 16:59:05 +0000 680)
if (ioctl(fileno(stdout), TIOCGWINSZ, &screen_size) != -1)

2014-04-08 20:27 GMT+04:00 Greg Stark <stark(at)mit(dot)edu>:

> On Tue, Apr 8, 2014 at 12:19 PM, Andres Freund <andres(at)2ndquadrant(dot)com>
> wrote:
> > I don't think this is easily testable that way - doesn't it rely on
> > determining the width of the terminal? Which you won't have when started
> > from pg_regress?
>
> There's a pset variable to set the target width so at least the
> formatting code can be tested. It would be nice to have the ioctl at
> least get called on the regression farm so we're sure we aren't doing
> something unportable.
>
> --
> greg
>

--
Best regards,
Sergey Muraviov

> How can I pass or set the value of pset variable for an regression test?

I just wrote some tests using \pset columns to control the output.

Having figured out what the point of the patch is I'm pretty happy
with the functionality. It definitely is something I would appreciate
having.

One thing I noticed is that it's not using the formatting characters
to indicate newlines and wrapping. I'm trying to add that myself but
it's taking me a bit to figure out what's going on in the formatting
code. I do have a feeling this is all rearranging the deck chairs on
an obsolete technology and this should all be replaced with a web ui.

Ok, So I've hacked on this a bit. Below is a test case showing the
problems I've found.

1) It isn't using the "newline" and "wrap" indicators or dividing lines.

2) The header is not being displayed properly when it contains a newline.

I can hack in the newline and wrap indicators but the header
formatting requires reworking the logic a bit. The header and data
need to be stepped through in parallel rather than having a loop to
handle the wrapping within the handling of a single line. I don't
really have time for that today but if you can get to it that would be
fine,

Hi.

Thanks for your tests.

I've fixed problem with headers, but got new one with data.
I'll try to solve it tomorrow.

2014-04-10 18:45 GMT+04:00 Greg Stark <stark(at)mit(dot)edu>:

> Ok, So I've hacked on this a bit. Below is a test case showing the
> problems I've found.
>
> 1) It isn't using the "newline" and "wrap" indicators or dividing lines.
>
> 2) The header is not being displayed properly when it contains a newline.
>
> I can hack in the newline and wrap indicators but the header
> formatting requires reworking the logic a bit. The header and data
> need to be stepped through in parallel rather than having a loop to
> handle the wrapping within the handling of a single line. I don't
> really have time for that today but if you can get to it that would be
> fine,
>

--
Best regards,
Sergey Muraviov

Hi.

I've done some corrections for printing "newline" and "wrap" indicators.
Please review the attached patch.

2014-04-11 0:14 GMT+04:00 Sergey Muraviov <sergey(dot)k(dot)muraviov(at)gmail(dot)com>:

> Hi.
>
> Thanks for your tests.
>
> I've fixed problem with headers, but got new one with data.
> I'll try to solve it tomorrow.
>
>
> 2014-04-10 18:45 GMT+04:00 Greg Stark <stark(at)mit(dot)edu>:
>
> Ok, So I've hacked on this a bit. Below is a test case showing the
>> problems I've found.
>>
>> 1) It isn't using the "newline" and "wrap" indicators or dividing lines.
>>
>> 2) The header is not being displayed properly when it contains a newline.
>>
>> I can hack in the newline and wrap indicators but the header
>> formatting requires reworking the logic a bit. The header and data
>> need to be stepped through in parallel rather than having a loop to
>> handle the wrapping within the handling of a single line. I don't
>> really have time for that today but if you can get to it that would be
>> fine,
>>
>
>
>
> --
> Best regards,
> Sergey Muraviov
>

--
Best regards,
Sergey Muraviov

Looks good.

It's still not doing the old-ascii column dividers but to be honest
I'm not sure what the intended behaviour of old-ascii is. I've noticed
old-ascii only displays the line markers for dividing lines, not the
final one. That seems pretty useless and maybe it's why we switched to
the new "ascii" mode, I don't recall.

This is the way old-ascii displays when two columns are wrapping -- it
seems pretty confused to me. The headers are displaying newline
markers from the ascii style instead of : indicators in the dividing
line. The : and ; markers are only displayed for the first column, not
the second one.

Maybe since the : and ; markers aren't shown for the final column that
means the expanded mode doesn't have to do anything since the column
is only ever the final column.

+--------------------+-----------------+
| x | x |
|+ y |+ y |
|+ z |+ z |
+--------------------+-----------------+
| x | xxxxxxxxxxxxxxx |
| xx ; xxxx |
| xxx : xxxxxxxxxxxxxxx |
| xxxx ; xxx |
| xxxxx : xxxxxxxxxxxxxxx |
| xxxxxx ; xx |
| xxxxxxx : xxxxxxxxxxxxxxx |
| xxxxxxxx ; x |
| xxxxxxxxx : xxxxxxxxxxxxxxx |
| xxxxxxxxxx : xxxxxxxxxxxxxx |
| xxxxxxxxxxx : xxxxxxxxxxxxx |
| xxxxxxxxxxxx : xxxxxxxxxxxx |
| xxxxxxxxxxxxx : xxxxxxxxxxx |
| xxxxxxxxxxxxxx : xxxxxxxxxx |
| xxxxxxxxxxxxxxx : xxxxxxxxx |
| xxxxxxxxxxxxxxxx : xxxxxxxx |
| xxxxxxxxxxxxxxxxx : xxxxxxx |
| xxxxxxxxxxxxxxxxxx : xxxxxx |
| xxxxxxxxxxxxxxxxxx : xxxxx |
| x : xxxx |
| xxxxxxxxxxxxxxxxxx : xxx |
| xx : xx |
| xxxxxxxxxxxxxxxxxx : x |
| xxx : |
| xxxxxxxxxxxxxxxxxx : |
| xxxx : |
| xxxxxxxxxxxxxxxxxx : |
| xxxxx : |
| xxxxxxxxxxxxxxxxxx : |
| xxxxxx |
| xxxxxxxxxxxxxxxxxx |
| xxxxxxx |
+--------------------+-----------------+

I hope that I realized old-ascii logic correctly.

2014-04-11 19:10 GMT+04:00 Greg Stark <stark(at)mit(dot)edu>:

> Looks good.
>
> It's still not doing the old-ascii column dividers but to be honest
> I'm not sure what the intended behaviour of old-ascii is. I've noticed
> old-ascii only displays the line markers for dividing lines, not the
> final one. That seems pretty useless and maybe it's why we switched to
> the new "ascii" mode, I don't recall.
>
> This is the way old-ascii displays when two columns are wrapping -- it
> seems pretty confused to me. The headers are displaying newline
> markers from the ascii style instead of : indicators in the dividing
> line. The : and ; markers are only displayed for the first column, not
> the second one.
>
> Maybe since the : and ; markers aren't shown for the final column that
> means the expanded mode doesn't have to do anything since the column
> is only ever the final column.
>
>
> +--------------------+-----------------+
> | x | x |
> |+ y |+ y |
> |+ z |+ z |
> +--------------------+-----------------+
> | x | xxxxxxxxxxxxxxx |
> | xx ; xxxx |
> | xxx : xxxxxxxxxxxxxxx |
> | xxxx ; xxx |
> | xxxxx : xxxxxxxxxxxxxxx |
> | xxxxxx ; xx |
> | xxxxxxx : xxxxxxxxxxxxxxx |
> | xxxxxxxx ; x |
> | xxxxxxxxx : xxxxxxxxxxxxxxx |
> | xxxxxxxxxx : xxxxxxxxxxxxxx |
> | xxxxxxxxxxx : xxxxxxxxxxxxx |
> | xxxxxxxxxxxx : xxxxxxxxxxxx |
> | xxxxxxxxxxxxx : xxxxxxxxxxx |
> | xxxxxxxxxxxxxx : xxxxxxxxxx |
> | xxxxxxxxxxxxxxx : xxxxxxxxx |
> | xxxxxxxxxxxxxxxx : xxxxxxxx |
> | xxxxxxxxxxxxxxxxx : xxxxxxx |
> | xxxxxxxxxxxxxxxxxx : xxxxxx |
> | xxxxxxxxxxxxxxxxxx : xxxxx |
> | x : xxxx |
> | xxxxxxxxxxxxxxxxxx : xxx |
> | xx : xx |
> | xxxxxxxxxxxxxxxxxx : x |
> | xxx : |
> | xxxxxxxxxxxxxxxxxx : |
> | xxxx : |
> | xxxxxxxxxxxxxxxxxx : |
> | xxxxx : |
> | xxxxxxxxxxxxxxxxxx : |
> | xxxxxx |
> | xxxxxxxxxxxxxxxxxx |
> | xxxxxxx |
> +--------------------+-----------------+
>

--
Best regards,
Sergey Muraviov

Sergey Muraviov wrote:
> I hope that I realized old-ascii logic correctly.

I don't know what you changed here, but I don't think we need to
preserve old-ascii behavior in the new code, in any way. If you're
mimicking something obsolete and the end result of the new feature is
not great, perhaps that should be rethought.

Can you please provide sample output for the previous version compared
to this new version? What changed?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

There were no support for wrapping and old-ascii line style in expanded
mode at all.
But now they are.

2014-04-11 21:58 GMT+04:00 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:

> Sergey Muraviov wrote:
> > I hope that I realized old-ascii logic correctly.
>
> I don't know what you changed here, but I don't think we need to
> preserve old-ascii behavior in the new code, in any way. If you're
> mimicking something obsolete and the end result of the new feature is
> not great, perhaps that should be rethought.
>
> Can you please provide sample output for the previous version compared
> to this new version? What changed?
>
>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

--
Best regards,
Sergey Muraviov

Yeah, I think I agree. I'm pretty happy to commit it without old-ascii
doing anything special.

It looks to me like old-ascii just isn't very useful for a single column
output (like expanded mode is implicitly). Maybe that needs to be fixed but
then it needs to be fixed for non expanded mode as well.

I don't think this new output is very useful dive it just displays the
old-ascii info for the header cells. The header cells never wrap and often
have a lot of empty lines so it just looks like noise.

I do think the earlier change today was important. It looks great now with
the wrap and newline indicators in ascii and Unicode mode.

Fwiw I've switched my .psqlrc to use Unicode and wrapped=auto be default.
It makes psql way more usable. It's even better than my old technique of
running it in Emacs and scrolling left and right.

I'll take a look at it this evening and will add regression tests (and
probably more comments too!) and commit. I want to try adding Unicode
regression tests but I'm not sure what the state of the art is for Unicode
fallback behaviour on the build farm. I think there are some existing tests
we can copy iirc.

--
greg

On Fri, Apr 11, 2014 at 9:47 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> I'll take a look at it this evening and will add regression tests (and
> probably more comments too!) and commit.

Well that evening turned into this morning a week later. Thanks to
Emre who fixed the remaining problems.

I added a regression test that tests every combination of linestyle
and border and format. That makes it kind of long but it's fast to run
and this code is easy to break so I would prefer to have tests for the
next time someone tries to fix something in it.

I expect this regression test to fail on platforms that don't support
utf-8 client-side (I'm assuming we such things?). I don't have such a
platform here and I'm not sure how it would fail so I want to go ahead
and apply it and grab the output to add the alternate output when it
fails on the build-farm. Would that be ok?

--
greg

Greg Stark <stark(at)mit(dot)edu> writes:
> I expect this regression test to fail on platforms that don't support
> utf-8 client-side (I'm assuming we such things?). I don't have such a
> platform here and I'm not sure how it would fail so I want to go ahead
> and apply it and grab the output to add the alternate output when it
> fails on the build-farm. Would that be ok?

Are you expecting to carry an alternate expected file for every possible
encoding choice? That does not seem workable to me, and even if we could
do it the cost/benefit ratio would be pretty grim. I think you should
drop the UTF8-dependent tests.

In other words: there are no encoding dependencies in the existing
standard regression tests. This feature is not the place to start adding
them, and two weeks past feature freeze is not the time to start adding
them either. We don't have time right now to shake out a whole new
set of platform dependencies in the regression tests.

If you feel these tests must be preserved someplace, you could add a
new regression test that isn't run by default, following in the
footsteps of collate.linux.utf8.

regards, tom lane

Not sure what other encodings you mean. Psql uses utf8 for the border and
the test uses utf8 to test the formatting. I was only anticipating an error
on platforms where that didn't work.

I would lean towards having it but I'm fine following your judgement,
especially given the timing.

--
greg

Greg Stark <stark(at)mit(dot)edu> writes:
> Not sure what other encodings you mean. Psql uses utf8 for the border and
> the test uses utf8 to test the formatting. I was only anticipating an error
> on platforms where that didn't work.

Well, there are two likely misbehaviors if the regression test is being
run in some other encoding:

1. If it's a single-byte encoding, you probably won't get any bad-encoding
complaints, but the code will think the utf8 characters represent multiple
logical characters, resulting in (at least) spacing differences. It's
possible that all single-byte encodings would act the same, but I'm not
sure.

2. If it's a multi-byte encoding different from utf8, you're almost
certainly going to get badly-encoded-data complaints, at different places
depending on the particular encoding.

I don't remember how many different multibyte encodings we support,
but I'm pretty sure we'd need a separate expected file for each one.
Plus at least one for the single-byters.

The real problem is that I don't have a lot of confidence that the
buildfarm would provide us with full coverage of all the encodings
that somebody might use in the field. So we might not find out about
omissions or wrong expected-files until after we ship.

Anyway, the bottom line for me is that this test isn't worth that
much trouble. I'm okay with putting it in as a separate test file
that we don't support running in non-utf8 encodings.

regards, tom lane

Please fix this compiler warning. I think it came from this patch.

print.c: In function ‘print_aligned_vertical’:
print.c:1354:10: error: pointer targets in passing argument 1 of ‘strlen_max_width’ differ in signedness [-Werror=pointer-sign]
encoding);
^
print.c:126:12: note: expected ‘unsigned char *’ but argument is of type ‘char *’
static int strlen_max_width(unsigned char *str, int *target_width, int encoding);
^

fixed

2014-04-29 5:52 GMT+04:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:

> Please fix this compiler warning. I think it came from this patch.
>
>
> print.c: In function ‘print_aligned_vertical’:
> print.c:1354:10: error: pointer targets in passing argument 1 of
> ‘strlen_max_width’ differ in signedness [-Werror=pointer-sign]
> encoding);
> ^
> print.c:126:12: note: expected ‘unsigned char *’ but argument is of type
> ‘char *’
> static int strlen_max_width(unsigned char *str, int *target_width, int
> encoding);
> ^
>
>
>

--
Best regards,
Sergey Muraviov

On Tue, Apr 29, 2014 at 12:37 PM, Sergey Muraviov
<sergey(dot)k(dot)muraviov(at)gmail(dot)com> wrote:
> 2014-04-29 5:52 GMT+04:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:
>
>> Please fix this compiler warning. I think it came from this patch.
>> print.c: In function 'print_aligned_vertical':
>> print.c:1354:10: error: pointer targets in passing argument 1 of
>> 'strlen_max_width' differ in signedness [-Werror=pointer-sign]
>> encoding);
>> ^
>> print.c:126:12: note: expected 'unsigned char *' but argument is of type
>> 'char *'
>> static int strlen_max_width(unsigned char *str, int *target_width, int
>> encoding);
>> ^
> fixed
Your patch has been committed by Greg not so long ago, you should send
for this warning a patch rebased on the latest master branch commit :)
--
Michael

rebased

2014-04-29 7:43 GMT+04:00 Michael Paquier <michael(dot)paquier(at)gmail(dot)com>:

> On Tue, Apr 29, 2014 at 12:37 PM, Sergey Muraviov
> <sergey(dot)k(dot)muraviov(at)gmail(dot)com> wrote:
> > 2014-04-29 5:52 GMT+04:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:
> >
> >> Please fix this compiler warning. I think it came from this patch.
> >> print.c: In function 'print_aligned_vertical':
> >> print.c:1354:10: error: pointer targets in passing argument 1 of
> >> 'strlen_max_width' differ in signedness [-Werror=pointer-sign]
> >> encoding);
> >> ^
> >> print.c:126:12: note: expected 'unsigned char *' but argument is of type
> >> 'char *'
> >> static int strlen_max_width(unsigned char *str, int *target_width, int
> >> encoding);
> >> ^
> > fixed
> Your patch has been committed by Greg not so long ago, you should send
> for this warning a patch rebased on the latest master branch commit :)
> --
> Michael
>

--
Best regards,
Sergey Muraviov

On Tue, Apr 29, 2014 at 2:52 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Please fix this compiler warning. I think it came from this patch.

Oops, I fixed it in a previous version but didn't notice it had crept
back in in the back-and-forth. Will do.

--
greg