wrapping in extended mode doesn't work well with default pager

Pavel Stehule <pavel.stehule@gmail.com>:

Hello

I am checking feature
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6513633b94173fc1d9e2b213c43f9422ddbf5faa

It works perfect with pager "less", but it works badly with default "more"

see attached screenshots, pls

It is expected behave?

I do not think so. It looks broken with or without any pager when
border != 2. Your less configuration might be hiding the problem from you.

I think it is because of miscalculation of the width used by
the separators. Increasing this variable for border = 0 and 1 fixed
the problem, but it might not be the right fix. The patch without
regression test changes attached.

While looking at it, I found another problem. It seems to me, a minus sign
is missing after -[RECORD ] when border = 1.

Emre Hasegeli <emre@hasegeli.com> writes:

Pavel Stehule <pavel.stehule@gmail.com>:

I am checking feature
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6513633b94173fc1d9e2b213c43f9422ddbf5faa

It works perfect with pager "less", but it works badly with default "more"

I do not think so. It looks broken with or without any pager when
border != 2. Your less configuration might be hiding the problem from you.

This seems broken in several ways. I tried this test case:

regression=# \x \pset format wrapped
Expanded display (expanded) is on.
Output format (format) is wrapped.
regression=# select * from pg_proc where prolang!=12;

In 9.3, the output looks like this:

-[ RECORD 1 ]---+---------------------------------------------------------------
--------------------------------------------------------------------------------
------------------------------------
proname | to_timestamp
pronamespace | 11
proowner | 10
prolang | 14
procost | 1
prorows | 0
provariadic | 0
protransform | -
...

In HEAD, I see:

-[ RECORD 1 ]---+---------------------------------------------------------------
proname | to_timestamp

pronamespace | 11

proowner | 10

prolang | 14

procost | 1

prorows | 0

provariadic | 0

protransform | -

After "\pset columns 77" it looks a little better:

-[ RECORD 1 ]---+------------------------------------------------------------
proname | to_timestamp
pronamespace | 11
proowner | 10
prolang | 14
procost | 1
prorows | 0
provariadic | 0
protransform | -
proisagg | f
proiswindow | f

but where did those leading spaces come from? The header line is
definitely not on board with that, and I think those spaces are
contributing to the lines being too long for the window. I think
possibly the code is also adding a space that shouldn't be there
at the end of the lines, because it prints lines that wrap around
if I \pset columns to either 79 or 80 in an 80-column window, so
the accounting is off by 2 someplace.

Also, this code looks quite broken:

width = dwidth + swidth + hwidth;
if ((output_columns > 0) && (width > output_columns))
{
dwidth = output_columns - hwidth - swidth;
width = output_columns;
}

What happens if output_columns is less than hwidth + swidth? The code
goes crazy is what happens, because all of these are unsigned ints and so
wraparound leads to setting dwidth to something approaching 4 billion.
Try the same example after "\pset columns 10". I don't necessarily expect
it to produce beautiful output, but I do expect it to not lock up.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, May 12, 2014 at 2:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

but where did those leading spaces come from? The header line is
definitely not on board with that, and I think those spaces are
contributing to the lines being too long for the window. I think
possibly the code is also adding a space that shouldn't be there
at the end of the lines, because it prints lines that wrap around
if I \pset columns to either 79 or 80 in an 80-column window, so
the accounting is off by 2 someplace.

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, May 12, 2014 at 2:12 PM, Greg Stark <stark@mit.edu> wrote:

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

Those spaces are coming from the ascii wrapping indicators. i.e. the periods in:

+-----------------+--------------------+
| a +| a +|
| +| b |
| b | |
+-----------------+--------------------+
| xx | yyyyyyyyyyyyyyyyyy |
| xxxx +| yyyyyyyyyyyyyyyy +|
| xxxxxx +| yyyyyyyyyyyyyy +|
| xxxxxxxx +| yyyyyyyyyyyy +|
| xxxxxxxxxx +| yyyyyyyyyy +|
| xxxxxxxxxxxx +| yyyyyyyy +|
| xxxxxxxxxxxxxx +| yyyyyy +|
| xxxxxxxxxxxxxxx.| yyyy +|
|.x +| yy +|
| xxxxxxxxxxxxxxx.| |
|.xxx +| |
| xxxxxxxxxxxxxxx.| |
|.xxxxx | |
+-----------------+--------------------+

Apparently we used to print those with border=1 in normal mode but in
expanded mode we left out the space for those on the outermost edges
since there was no need for them. If we put them in for wrapped mode
then we'll be inconsistent if we don't for nonwrapped mode though. And
if we don't put them in for wrapped mode then there's no way to
indicate wrapping versus newlines.

The biggest difference it makes is that in the border=1 mode the lines
ended at the end of the data previously. Now it's expanded to fill the
rectangle because of the plus symbols. ie. It used to look like:

-[ RECORD 1 ]-----------
a | xx
|
b |
a | yyyyyyyyyyyyyyyyyy
b |
-[ RECORD 2 ]-----------
a | xxxx
| xxxxxx
b | xxxxxxxx
| xxxxxxxxxx
| xxxxxxxxxxxx
| xxxxxxxxxxxxxx
| xxxxxxxxxxxxxxxx
| xxxxxxxxxxxxxxxxxx
| xxxxxxxxxxxxxxxxxxxx
a | yyyyyyyyyyyyyyyy
b | yyyyyyyyyyyyyy
| yyyyyyyyyyyy
| yyyyyyyyyy
| yyyyyyyy
| yyyyyy
| yyyy
| yy
|

and now looks like:

-[ RECORD 1 ]-----------
a+| xx
+|
b |
a+| yyyyyyyyyyyyyyyyyy
b |
-[ RECORD 2 ]-----------
a+| xxxx +
+| xxxxxx +
b | xxxxxxxx +
| xxxxxxxxxx +
| xxxxxxxxxxxx +
| xxxxxxxxxxxxxx +
| xxxxxxxxxxxxxxxx +
| xxxxxxxxxxxxxxxxxx +
| xxxxxxxxxxxxxxxxxxxx
a+| yyyyyyyyyyyyyyyy +
b | yyyyyyyyyyyyyy +
| yyyyyyyyyyyy +
| yyyyyyyyyy +
| yyyyyyyy +
| yyyyyy +
| yyyy +
| yy +
|

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Greg Stark <stark@mit.edu> writes:

On Mon, May 12, 2014 at 2:12 PM, Greg Stark <stark@mit.edu> wrote:

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

Those spaces are coming from the ascii wrapping indicators. i.e. the periods in:

Ah. I wonder whether anyone will complain that the format changed?

Apparently we used to print those with border=1 in normal mode but in
expanded mode we left out the space for those on the outermost edges
since there was no need for them. If we put them in for wrapped mode
then we'll be inconsistent if we don't for nonwrapped mode though. And
if we don't put them in for wrapped mode then there's no way to
indicate wrapping versus newlines.

Barring anyone complaining that the format changed, I'd say the issue
is not that you added them but that the accounting for line length
fails to include them.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi.

I'll try to fix it tomorrow.

2014-05-12 18:42 GMT+04:00 Tom Lane <tgl@sss.pgh.pa.us>:

Greg Stark <stark@mit.edu> writes:

On Mon, May 12, 2014 at 2:12 PM, Greg Stark <stark@mit.edu> wrote:

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

Those spaces are coming from the ascii wrapping indicators. i.e. the

periods in:

Ah. I wonder whether anyone will complain that the format changed?

Apparently we used to print those with border=1 in normal mode but in
expanded mode we left out the space for those on the outermost edges
since there was no need for them. If we put them in for wrapped mode
then we'll be inconsistent if we don't for nonwrapped mode though. And
if we don't put them in for wrapped mode then there's no way to
indicate wrapping versus newlines.

Barring anyone complaining that the format changed, I'd say the issue
is not that you added them but that the accounting for line length
fails to include them.

regards, tom lane

--
Best regards,
Sergey Muraviov

Please check this patch.

2014-05-12 22:56 GMT+04:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:

Hi.

I'll try to fix it tomorrow.

2014-05-12 18:42 GMT+04:00 Tom Lane <tgl@sss.pgh.pa.us>:

Greg Stark <stark@mit.edu> writes:

On Mon, May 12, 2014 at 2:12 PM, Greg Stark <stark@mit.edu> wrote:

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

Those spaces are coming from the ascii wrapping indicators. i.e. the

periods in:

Ah. I wonder whether anyone will complain that the format changed?

Apparently we used to print those with border=1 in normal mode but in
expanded mode we left out the space for those on the outermost edges
since there was no need for them. If we put them in for wrapped mode
then we'll be inconsistent if we don't for nonwrapped mode though. And
if we don't put them in for wrapped mode then there's no way to
indicate wrapping versus newlines.

Barring anyone complaining that the format changed, I'd say the issue
is not that you added them but that the accounting for line length
fails to include them.

regards, tom lane

--
Best regards,
Sergey Muraviov

--
Best regards,
Sergey MuraviovH

Hello

With this patch it works perfect

Thank you

Regards

Pavel

2014-05-13 21:33 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:

sorry

there is still small issue

I have a plpgsql function:

CREATE OR REPLACE FUNCTION public.foo_update_trg()
RETURNS trigger
LANGUAGE plpgsql
AS $function$
DECLARE t text;
BEGIN
EXECUTE format('SELECT $1.%I', TG_ARGV[0]) INTO t USING old;
RAISE NOTICE 'original value of "%" is "%"', TG_ARGV[0], t;
RETURN NULL;
END;
$function$

Default expanded view of select * from pg_proc where proname =
'foo_update_trg'; is little bit broken (screenshoot 1)

After wrap mode, it add useless new line into source code (screenshoot 2)

but border2 fixes it (screenshots 3)

Regards

Pavel

2014-05-14 8:32 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi.
Please review the new patch.

PS
Issues which were described by Tom and Pavel were relevant to single-line
headers.
So I've added appropriate regression tests to the patch.

I've also attached complex regression tests for unicode linestyle and
multibyte symbols.

2014-05-14 10:55 GMT+04:00 Pavel Stehule <pavel.stehule@gmail.com>:

sorry

there is still small issue

I have a plpgsql function:

CREATE OR REPLACE FUNCTION public.foo_update_trg()
RETURNS trigger
LANGUAGE plpgsql
AS $function$
DECLARE t text;
BEGIN
EXECUTE format('SELECT $1.%I', TG_ARGV[0]) INTO t USING old;
RAISE NOTICE 'original value of "%" is "%"', TG_ARGV[0], t;
RETURN NULL;
END;
$function$

Default expanded view of select * from pg_proc where proname =
'foo_update_trg'; is little bit broken (screenshoot 1)

After wrap mode, it add useless new line into source code (screenshoot 2)

but border2 fixes it (screenshots 3)

Regards

Pavel

2014-05-14 8:32 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

With this patch it works perfect

Thank you

Regards

Pavel

2014-05-13 21:33 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:

Please check this patch.

2014-05-12 22:56 GMT+04:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>
:

Hi.

I'll try to fix it tomorrow.

2014-05-12 18:42 GMT+04:00 Tom Lane <tgl@sss.pgh.pa.us>:

Greg Stark <stark@mit.edu> writes:

On Mon, May 12, 2014 at 2:12 PM, Greg Stark <stark@mit.edu> wrote:

Hm, there was an off by one error earlier in some cases, maybe we
fixed it by breaking other case. Will investigate.

Those spaces are coming from the ascii wrapping indicators. i.e. the

periods in:

Ah. I wonder whether anyone will complain that the format changed?

Apparently we used to print those with border=1 in normal mode but in
expanded mode we left out the space for those on the outermost edges
since there was no need for them. If we put them in for wrapped mode
then we'll be inconsistent if we don't for nonwrapped mode though.

And

if we don't put them in for wrapped mode then there's no way to
indicate wrapping versus newlines.

Barring anyone complaining that the format changed, I'd say the issue
is not that you added them but that the accounting for line length
fails to include them.

regards, tom lane

--
Best regards,
Sergey Muraviov

--
Best regards,
Sergey MuraviovH

--
Best regards,
Sergey Muraviov

Hello

2014-05-15 15:04 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:

Hi.
Please review the new patch.

This version works perfect

Regards

Pavel

I'm trying to review all the combinations of the options exhaustively
but in the process I noticed a few pre-existing psql oddities. Both of
these are present in 9.3:

Can anyone explain this? It's linestyle=old-style, border=1,
expanded=off, format=aligned. It looks like it's using new-style ascii
indicators in the header but old-style in the data cells:

          a           |         a
+                     |+        b
+         b           |+
----------------------+--------------------
 xx                   | yyyyyyyyyyyyyyyyyy
 xxxx                 | yyyyyyyyyyyyyyyy
 xxxxxx               : yyyyyyyyyyyyyy
 xxxxxxxx             : yyyyyyyyyyyy
 xxxxxxxxxx           : yyyyyyyyyy
 xxxxxxxxxxxx         : yyyyyyyy
 xxxxxxxxxxxxxx       : yyyyyy
 xxxxxxxxxxxxxxxx     : yyyy
 xxxxxxxxxxxxxxxxxx   : yy
 xxxxxxxxxxxxxxxxxxxx :
(2 rows)

Also the line-ending white-space is very odd here. It's
linestyle=old-ascii, border=0, expanded=off, format=aligned. There's
an extra space on the header and the first line of the data, but not
on the subsequent lines of the data:

a a
+ b
b +
-------------------- ------------------
xx yyyyyyyyyyyyyyyyyy
xxxx yyyyyyyyyyyyyyyy
xxxxxx yyyyyyyyyyyyyy
xxxxxxxx yyyyyyyyyyyy
xxxxxxxxxx yyyyyyyyyy
xxxxxxxxxxxx yyyyyyyy
xxxxxxxxxxxxxx yyyyyy
xxxxxxxxxxxxxxxx yyyy
xxxxxxxxxxxxxxxxxx yy
xxxxxxxxxxxxxxxxxxxx
(2 rows)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Sorry, a couple things still look to not be quite right.

1) The width of the table when linestyle=old-ascii and border=0 or
border=1 (and expanded=on and format=wrapped) seems to off by one.

2) The hyphens following the RECORD NN are short by one

I'm surprised the last patch was so big since it sounded like a simple
off-by-one bug. It looks like you've removed the leading space on the
border=0 expanded case. I guess that makes sense but we should
probably stop making significant changes now and just focus on fixing
the off by one bugs.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I found some new bugs and fix them.
And I had to make many changes.

2014-05-17 21:31 GMT+04:00 Greg Stark <stark@mit.edu>:

Sorry, a couple things still look to not be quite right.

1) The width of the table when linestyle=old-ascii and border=0 or
border=1 (and expanded=on and format=wrapped) seems to off by one.

2) The hyphens following the RECORD NN are short by one

I'm surprised the last patch was so big since it sounded like a simple
off-by-one bug. It looks like you've removed the leading space on the
border=0 expanded case. I guess that makes sense but we should
probably stop making significant changes now and just focus on fixing
the off by one bugs.

--
Best regards,
Sergey Muraviov

Sergey Muraviov wrote:

I found some new bugs and fix them.
And I had to make many changes.

This version fixes some bugs I had noticed in expanded mode too. For instance,
the original looked like this (five lines plus header):

-[ RECORD 49 ]-----+-------------------------------------------------------------------------
pg_identify_object | (rule,,,"""_RETURN"" on pg_catalog.pg_available_extension_versions")

pg_identify_object | (view,pg_catalog,pg_available_extension_versions,pg_catalog.pg_availabl
e.
|._extension_versions)

whereas it's correctly only three lines plus header with this patch
applied. I can't tell whether this patch is minimal enough.

Having a more comprehensive test case is good, of course, though I
didn't check the expected file. Note that some things cannot be tested
correctly in this way, namely column count that actually match the
terminal -- to wit: I had to add line breaks manually to the above paste
so that it would look like what it does in my terminal. If I just paste
it, it looks correct, but then my email terminal is wider than the one I
ran psql in. I would presume that failure to account for this is what
caused (some of?) the bugs being fixed now ...

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello

where we are with this feature?

Is there some barriers to commit bugfix?

Regards

Pavel

2014-05-18 19:46 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:

On Fri, May 23, 2014 at 10:10:23AM -0400, Alvaro Herrera wrote:

Sergey Muraviov wrote:

I found some new bugs and fix them.
And I had to make many changes.

This version fixes some bugs I had noticed in expanded mode too. For instance,
the original looked like this (five lines plus header):

-[ RECORD 49 ]-----+-------------------------------------------------------------------------
pg_identify_object | (rule,,,"""_RETURN"" on pg_catalog.pg_available_extension_versions")

pg_identify_object | (view,pg_catalog,pg_available_extension_versions,pg_catalog.pg_availabl
e.
|._extension_versions)

whereas it's correctly only three lines plus header with this patch
applied.

I had noticed a similar-looking behavior change with aligned format and
expanded display, so I gave psql-wrapped-expanded-fix-v4.patch a spin with
this test case:

\pset expanded on
\pset format aligned
select repeat('a', 2) union all select repeat('a', 500);

The patch did not restore 9.3 behavior for that one. Starting with commit
6513633, the first line of letters is space-padded on the right to the width
of the second line of letters. To illustrate, I have attached raw psql output
from both commit 6513633 and its predecessor. Also note that
psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
bytes to 511 bytes; 509 is the longstanding width.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

On 6/8/14, 11:29 PM, Noah Misch wrote:

The patch did not restore 9.3 behavior for that one. Starting with commit
6513633, the first line of letters is space-padded on the right to the width
of the second line of letters. To illustrate, I have attached raw psql output
from both commit 6513633 and its predecessor. Also note that
psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
bytes to 511 bytes; 509 is the longstanding width.

I noticed that (or perhaps a related) problem today. Here is a simple demo:

psql -X -q -t -x -c 'select * from (values (1),(2)) as _ (col)'

9.3:

col | 1
----+--
col | 2

9.4:

col | 1
----+--
col | 2

This breaks check_postgres. (Why check_postgres doesn't use unaligned
output is beyond me.)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jun 11, 2014 at 7:52 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 6/8/14, 11:29 PM, Noah Misch wrote:

The patch did not restore 9.3 behavior for that one. Starting with commit
6513633, the first line of letters is space-padded on the right to the width
of the second line of letters. To illustrate, I have attached raw psql output
from both commit 6513633 and its predecessor. Also note that
psql-wrapped-expanded-fix-v4.patch expands each [ RECORD x ] header from 509
bytes to 511 bytes; 509 is the longstanding width.

I noticed that (or perhaps a related) problem today. Here is a simple demo:

I don't think these two issues are related.

The leading space that you (ie Peter) are complaining about in:

col | 1
----+--
col | 2

Is there because if the cell wrapped it would get an ellipsis (ie
'...' but it's a single unicode character) in that column to indicate
that it's wrapped. However we don't wrap headers so the only reason to
change it is for the "old-ascii" linestyle:

stark=***# select * from (values (1),(2)) as _ ("col
col");
stark"***#
col | 1
+col ;
-----+-----
col | 2
+col ;

Noah's complaint is about the space padding on the *right*. Ie

stark=***# select * from (values ('foo'),('foo bar baz')) as _ ("col");
col | foo
<- This is the end of the line
-----+--------------------------------------------------------------------------<-
This is the end of the line
col | foo bar baz
<- This is the end of the line

We didn't used to do that in expanded and I kind of agree it would be
nice to avoid. But then there are lots of cases where it would still
be necessary:

stark=***# select * from (values ('foo'),('foo bar
 baz')) as _ ("col");
stark'***#
 col | foo
         <- This is the end of the line
-----+--------------------------------------------------------------------------<-
This is the end of the line
 col | foo bar
        +<- This is the end of the line
     |  baz
        <- This is the end of the line

Obviously we would need to space padd to insert the "+" there.

I think this whole exercise has mostly just convinced me we should
implement an HTTP interface and reimplement psql as a browser app.

--
greg

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers