Обсуждение: wrapping in extended mode doesn't work well with default pager
Hello
I am checking feature http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6513633b94173fc1d9e2b213c43f9422ddbf5faasee attached screenshots, pls
It is expected behave?
It is expected behave?
Regards
Pavel
Pavel
Вложения
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
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
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+| yyyyyyyyyyyyyyyyyyb | -[ RECORD 2 ]-----------a+| xxxx + +| xxxxxx +b | xxxxxxxx + | xxxxxxxxxx + | xxxxxxxxxxxx + | xxxxxxxxxxxxxx + | xxxxxxxxxxxxxxxx + | xxxxxxxxxxxxxxxxxx + | xxxxxxxxxxxxxxxxxxxxa+|yyyyyyyyyyyyyyyy +b | yyyyyyyyyyyyyy + | yyyyyyyyyyyy + | yyyyyyyyyy + | yyyyyyyy + | yyyyyy + | yyyy + | yy + | -- greg
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
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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
is not that you added them but that the accounting for line length
fails to include them.
regards, tom lane
Best regards,
Sergey MuraviovPlease 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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
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 perfectPavel
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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
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
sorry
there is still small issueCREATE 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$
2014-05-14 8:32 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
RegardsThank youHelloWith this patch it works perfect
Pavel2014-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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
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
Вложения
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>:
PavelRegardsbut border2 fixes it (screenshots 3)After wrap mode, it add useless new line into source code (screenshoot 2)Default expanded view of select * from pg_proc where proname = 'foo_update_trg'; is little bit broken (screenshoot 1)I have a plpgsql function:sorrythere is still small issue
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$2014-05-14 8:32 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:RegardsThank youHelloWith this patch it works perfect
Pavel2014-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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
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
Pavel
PSIssues 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>:PavelRegardsbut border2 fixes it (screenshots 3)After wrap mode, it add useless new line into source code (screenshoot 2)Default expanded view of select * from pg_proc where proname = 'foo_update_trg'; is little bit broken (screenshoot 1)I have a plpgsql function:sorrythere is still small issue
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$2014-05-14 8:32 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:RegardsThank youHelloWith this patch it works perfect
Pavel2014-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:Ah. I wonder whether anyone will complain that the format changed?
> 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:Barring anyone complaining that the format changed, I'd say the issue
> 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.
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
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 | yyyyyyyyyyyyyyyyyyxxxx | yyyyyyyyyyyyyyyyxxxxxx : yyyyyyyyyyyyyyxxxxxxxx : yyyyyyyyyyyyxxxxxxxxxx : yyyyyyyyyyxxxxxxxxxxxx : yyyyyyyyxxxxxxxxxxxxxx : yyyyyyxxxxxxxxxxxxxxxx : yyyyxxxxxxxxxxxxxxxxxx : yyxxxxxxxxxxxxxxxxxxxx: (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)
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.
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
Hello
where we are with this feature?
Is there some barriers to commit bugfix?where we are with this feature?
2014-05-18 19:46 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:
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
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.)
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 linecol | 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 barbaz')) as _ ("col"); stark'***#col | foo <- This is the end of the line -----+--------------------------------------------------------------------------<- This is the end of the linecol | foo bar +<- This is the end of the line | baz <- This is the end of theline 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
And Gmail has thoroughly mangled that email. Let me see if I can resend it from Emacs more clearly.
On Wed, Jun 11, 2014 at 08:59:34PM +0100, Greg Stark wrote: > 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. Based on the commit message and procedural history, I thought commit 6513633 was changing behavior solely for the combination of "\pset expanded" and "\pset format wrapped". Peter's and my test cases show that it also changed behavior for "\pset expanded" alone. That's a bug, unless someone sees to argue that the new "\pset expanded" behavior is a desirable improvement in spite of its origin as an accident. Altering an entrenched psql output format is a big deal. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
Noah Misch <noah@leadboat.com> writes: > Based on the commit message and procedural history, I thought commit 6513633 > was changing behavior solely for the combination of "\pset expanded" and > "\pset format wrapped". Peter's and my test cases show that it also changed > behavior for "\pset expanded" alone. That's a bug, unless someone sees to > argue that the new "\pset expanded" behavior is a desirable improvement in > spite of its origin as an accident. Altering an entrenched psql output format > is a big deal. TBH I'm wondering if we shouldn't just revert that patch (and the subsequent fix attempts). It was not a major feature and I'm thinking we have better things to do right now than try to fix the multiple logic holes it evidently has. The author's certainly welcome to try again with a more carefully thought-through patch for 9.5. regards, tom lane
Hi.
I tried to take into account all of yours comments.
PS
This patch prints newline indicators in aligned mode, but this functionality can be easily disabled.
PPS
I'm on vacations now and don't read mail everyday.
2014-06-12 6:16 GMT+04:00 Tom Lane <tgl@sss.pgh.pa.us>:
Noah Misch <noah@leadboat.com> writes:TBH I'm wondering if we shouldn't just revert that patch (and the
> Based on the commit message and procedural history, I thought commit 6513633
> was changing behavior solely for the combination of "\pset expanded" and
> "\pset format wrapped". Peter's and my test cases show that it also changed
> behavior for "\pset expanded" alone. That's a bug, unless someone sees to
> argue that the new "\pset expanded" behavior is a desirable improvement in
> spite of its origin as an accident. Altering an entrenched psql output format
> is a big deal.
subsequent fix attempts). It was not a major feature and I'm thinking
we have better things to do right now than try to fix the multiple
logic holes it evidently has. The author's certainly welcome to try
again with a more carefully thought-through patch for 9.5.
regards, tom lane
Best regards,
Sergey MuraviovВложения
On Wed, Jun 11, 2014 at 10:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Noah Misch <noah@leadboat.com> writes: >> Based on the commit message and procedural history, I thought commit 6513633 >> was changing behavior solely for the combination of "\pset expanded" and >> "\pset format wrapped". Peter's and my test cases show that it also changed >> behavior for "\pset expanded" alone. That's a bug, unless someone sees to >> argue that the new "\pset expanded" behavior is a desirable improvement in >> spite of its origin as an accident. Altering an entrenched psql output format >> is a big deal. > > TBH I'm wondering if we shouldn't just revert that patch (and the > subsequent fix attempts). It was not a major feature and I'm thinking > we have better things to do right now than try to fix the multiple > logic holes it evidently has. The author's certainly welcome to try > again with a more carefully thought-through patch for 9.5. So, it seems like we need to do something about this one way or another. Who's working on that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Jun 16, 2014 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote: > So, it seems like we need to do something about this one way or > another. Who's working on that? So I'm fine finishing what I started. I've just been a bit busy this past week. My inclination is to try to push forward and commit this patch, document the changes and make sure we check for any consequences of them. The alternate plan is to revert it for 9.4 and commit the changes to 9.5 and that gives us more time to be sure we're ok with them. -- greg
On Wed, Jun 11, 2014 at 12:59 PM, Greg Stark <stark@mit.edu> wrote: > I think this whole exercise has mostly just convinced me we should > implement an HTTP interface and reimplement psql as a browser app. I certainly hope not. I've seen lots of browser apps that were nice enough to use for casual use by a casual user. I've never seen one that was an effective power tool for power users, the way psql is. Now maybe they are out there and I just don't know about them, but I have my doubts. As an additional tool, to each his own. But a browser-based replacement for psql, -1 from me. And as far browser-based things apply to this patch, I must say I've tried micromanaging the way large amounts of data wrap in a HTML table when I found the default to be inadequate, and I have not found that to be noticeably easy, either. The original version of this patch was only a few lines long and did one very simple and useful thing: avoiding the printing of whole screens full of hyphens when in 'expanded mode'. If we end up reverting the other parts of this patch, hopefully we don't lose that part. Cheers, Jeff
2014-06-16 23:28 GMT+02:00 Jeff Janes <jeff.janes@gmail.com>:
On Wed, Jun 11, 2014 at 12:59 PM, Greg Stark <stark@mit.edu> wrote:I certainly hope not. I've seen lots of browser apps that were nice
> I think this whole exercise has mostly just convinced me we should
> implement an HTTP interface and reimplement psql as a browser app.
enough to use for casual use by a casual user. I've never seen one
that was an effective power tool for power users, the way psql is.
Now maybe they are out there and I just don't know about them, but I
have my doubts.
As an additional tool, to each his own. But a browser-based
replacement for psql, -1 from me.
We can integrate a text console browsers like links, elinks or lynx instead
and we can call a BROWSER instead PAGER when \pset is html
pavel@localhost postgresql92]$ PAGER="elinks -force-html" psql postgres
psql (9.4beta1)
Type "help" for help.
postgres=# \pset format html
Output format (format) is html.
postgres=# SELECT * FROM pg_proc;
pavel@localhost postgresql92]$ PAGER="elinks -force-html" psql postgres
psql (9.4beta1)
Type "help" for help.
postgres=# \pset format html
Output format (format) is html.
postgres=# SELECT * FROM pg_proc;
works perfect
[pavel@localhost postgresql92]$ PAGER="lynx -stdin" psql postgres
psql (9.4beta1)
Type "help" for help.
postgres=# \pset format html
Output format (format) is html.
postgres=# SELECT * FROM pg_proc;
[pavel@localhost postgresql92]$ PAGER="lynx -stdin" psql postgres
psql (9.4beta1)
Type "help" for help.
postgres=# \pset format html
Output format (format) is html.
postgres=# SELECT * FROM pg_proc;
Writing html browsing into psql is useless now and I don't think so it is good idea. On second hand better integration mentioned browsers can be very useful.
Regards
Pavel
Pavel
And as far browser-based things apply to this patch, I must say I've
tried micromanaging the way large amounts of data wrap in a HTML table
when I found the default to be inadequate, and I have not found that
to be noticeably easy, either.
The original version of this patch was only a few lines long and did
one very simple and useful thing: avoiding the printing of whole
screens full of hyphens when in 'expanded mode'. If we end up
reverting the other parts of this patch, hopefully we don't lose that
part.
Cheers,
Jeff
Hi.
Is there any problem with the patch?
2014-06-17 0:21 GMT+04:00 Greg Stark <stark@mit.edu>:
On Mon, Jun 16, 2014 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:So I'm fine finishing what I started. I've just been a bit busy this past week.
> So, it seems like we need to do something about this one way or
> another. Who's working on that?
My inclination is to try to push forward and commit this patch,
document the changes and make sure we check for any consequences of
them.
The alternate plan is to revert it for 9.4 and commit the changes to
9.5 and that gives us more time to be sure we're ok with them.
--
greg
Best regards,
Sergey Muraviov2014-06-24 19:45 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:
Hi.Is there any problem with the patch?
I tested it and I had not any issue with last version
So, please, commit it
Regards
Pavel
2014-06-17 0:21 GMT+04:00 Greg Stark <stark@mit.edu>:On Mon, Jun 16, 2014 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:So I'm fine finishing what I started. I've just been a bit busy this past week.
> So, it seems like we need to do something about this one way or
> another. Who's working on that?
My inclination is to try to push forward and commit this patch,
document the changes and make sure we check for any consequences of
them.
The alternate plan is to revert it for 9.4 and commit the changes to
9.5 and that gives us more time to be sure we're ok with them.
--
greg--Best regards,Sergey Muraviov
Hi.
Is there anyone who can commit the patch?
2014-06-25 20:17 GMT+04:00 Pavel Stehule <pavel.stehule@gmail.com>:
2014-06-24 19:45 GMT+02:00 Sergey Muraviov <sergey.k.muraviov@gmail.com>:Hi.Is there any problem with the patch?I tested it and I had not any issue with last versionSo, please, commit itRegardsPavel2014-06-17 0:21 GMT+04:00 Greg Stark <stark@mit.edu>:On Mon, Jun 16, 2014 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:So I'm fine finishing what I started. I've just been a bit busy this past week.
> So, it seems like we need to do something about this one way or
> another. Who's working on that?
My inclination is to try to push forward and commit this patch,
document the changes and make sure we check for any consequences of
them.
The alternate plan is to revert it for 9.4 and commit the changes to
9.5 and that gives us more time to be sure we're ok with them.
--
greg--Best regards,Sergey Muraviov
Best regards,
Sergey MuraviovOn Sun, Jul 6, 2014 at 8:40 AM, Sergey Muraviov <sergey.k.muraviov@gmail.com> wrote: > Is there anyone who can commit the patch? So what I'm inclined to do here (sigh) is commit it into 9.5 and revert it in 9.4. I think it's an improvement but I there's enough confusion and surprise about the changes from people who weren't expecting them and enough surprising side effects from things like check_postgres that letting it simmer for a full release so people can get used to it might be worthwhile. -- greg
So what's wrong with the patch?
And what should I change in it for 9.5?
2014-07-07 3:12 GMT+04:00 Greg Stark <stark@mit.edu>:
On Sun, Jul 6, 2014 at 8:40 AM, Sergey MuraviovSo what I'm inclined to do here (sigh) is commit it into 9.5 and
<sergey.k.muraviov@gmail.com> wrote:
> Is there anyone who can commit the patch?
revert it in 9.4. I think it's an improvement but I there's enough
confusion and surprise about the changes from people who weren't
expecting them and enough surprising side effects from things like
check_postgres that letting it simmer for a full release so people can
get used to it might be worthwhile.
--
greg
Best regards,
Sergey MuraviovOn Mon, Jul 7, 2014 at 8:35 AM, Sergey Muraviov <sergey.k.muraviov@gmail.com> wrote: > So what's wrong with the patch? > And what should I change in it for 9.5? Possibly nothing. The concern was tha it's modifying the output in cases where the output is not \expanded and/or not wrapped. Now I've mostly convinced myself that those cases should change to be consistent with the wrapped output but there was at least one tool depending on that format (check_postgres) so perhaps it's not worthwhile and having it be inconsistent would be safer. -- greg
This remains open for 9.4. Your proposal to revert the feature in 9.4 and fix it in 9.5 sounds reasonable. On Thu, Jul 10, 2014 at 04:15:35PM +0100, Greg Stark wrote: > On Mon, Jul 7, 2014 at 8:35 AM, Sergey Muraviov > <sergey.k.muraviov@gmail.com> wrote: > > So what's wrong with the patch? > > And what should I change in it for 9.5? > > Possibly nothing. The concern was tha it's modifying the output in > cases where the output is not \expanded and/or not wrapped. Now I've > mostly convinced myself that those cases should change to be > consistent with the wrapped output but there was at least one tool > depending on that format (check_postgres) so perhaps it's not > worthwhile and having it be inconsistent would be safer. I did try psql-wrapped-expanded-fix-v5.patch with the tests Peter and I posted upthread, and those tests now behave as they do in released versions. What cases did you find that still change vs. 9.3? Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On Tue, Aug 5, 2014 at 3:41 AM, Noah Misch <noah@leadboat.com> wrote: > This remains open for 9.4. Your proposal to revert the feature in 9.4 and fix > it in 9.5 sounds reasonable. Ok, I've gone ahead and done this. I'm sorry for the delays and confusion. > On Thu, Jul 10, 2014 at 04:15:35PM +0100, Greg Stark wrote: >> On Mon, Jul 7, 2014 at 8:35 AM, Sergey Muraviov >> <sergey.k.muraviov@gmail.com> wrote: >> > So what's wrong with the patch? >> > And what should I change in it for 9.5? >> >> Possibly nothing. The concern was tha it's modifying the output in >> cases where the output is not \expanded and/or not wrapped. Now I've >> mostly convinced myself that those cases should change to be >> consistent with the wrapped output but there was at least one tool >> depending on that format (check_postgres) so perhaps it's not >> worthwhile and having it be inconsistent would be safer. > > I did try psql-wrapped-expanded-fix-v5.patch with the tests Peter and I posted > upthread, and those tests now behave as they do in released versions. What > cases did you find that still change vs. 9.3? I was trying to build a spreadsheet of every combination of these options. It turns out that 4-dimensional spreadsheets are kind of awkward. I think the fundamental dilemma was the same that was discussed upthread. If wrapped expanded mode should have an extra space padding column for the wrap indicators then all expanded modes should have that column to be consistent since wrapping shouldn't change the amount of padding. But that means unrelated queries changes format in ways people weren't expecting. They were depending on the basically inconsistent formatting where expanded didn't have the same padding that non-expanded formats had which was only the case because nobody had anticiapted expanded format needing space for wrapping indicators. -- greg
On Mon, Aug 18, 2014 at 8:30 PM, Greg Stark <stark@mit.edu> wrote: > On Tue, Aug 5, 2014 at 3:41 AM, Noah Misch <noah@leadboat.com> wrote: >> This remains open for 9.4. Your proposal to revert the feature in 9.4 and fix >> it in 9.5 sounds reasonable. > > Ok, I've gone ahead and done this. I'm sorry for the delays and confusion. I imagine that you also need to fix the release notes accordingly. Patch attached for master and REL9_4_STABLE. Regards, -- Michael
Вложения
On Mon, Aug 18, 2014 at 12:55 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > I imagine that you also need to fix the release notes accordingly. > Patch attached for master and REL9_4_STABLE. Thanks. Done for 9.4 but the patch is still in master. In fact it's the most recent version and I'm still pretty convinced it's a good patch. -- greg
On Mon, Aug 18, 2014 at 10:02 PM, Greg Stark <stark@mit.edu> wrote: > On Mon, Aug 18, 2014 at 12:55 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > Done for 9.4 but the patch is still in master. In fact it's the most > recent version and I'm still pretty convinced it's a good patch. If this feature is not going to be part of 9.4 anymore, the related release notes should be updated in consequence on REL9_4_STABLE and master, no? Am I missing something? -- Michael
On Mon, Aug 18, 2014 at 12:30:40PM +0100, Greg Stark wrote: > On Tue, Aug 5, 2014 at 3:41 AM, Noah Misch <noah@leadboat.com> wrote: > > This remains open for 9.4. Your proposal to revert the feature in 9.4 and fix > > it in 9.5 sounds reasonable. > > Ok, I've gone ahead and done this. I'm sorry for the delays and confusion. Thanks. > > I did try psql-wrapped-expanded-fix-v5.patch with the tests Peter and I posted > > upthread, and those tests now behave as they do in released versions. What > > cases did you find that still change vs. 9.3? > > I was trying to build a spreadsheet of every combination of these > options. It turns out that 4-dimensional spreadsheets are kind of > awkward. What's one query that still behaves differently in 9.5 vs. 9.3 (under formatting options that exist in both versions)? > I think the fundamental dilemma was the same that was discussed > upthread. If wrapped expanded mode should have an extra space padding > column for the wrap indicators then all expanded modes should have > that column to be consistent since wrapping shouldn't change the > amount of padding. I might agree for a greenfield design, but -1 for changing expanded mode now to improve consistency in this way. I predict the complaints from users of expanded mode in automation would overpower any applause for the consistency.