Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote: The newline handling code was, by far, the most complex part of this patch. While I think it would be nicer to filter up past the newline, I just don't see this as a common need. Large text fields with newlines are difficult to really view in psql anyway, and they are likely to be the longest columns in any given query. Bottom line: not worth messing with. OK, we can always return to it. I'm split on the new formatting style. It looks a lot less grid-like, which I might like once I get used to it (which will be a while, because I can't run my own patch in daily use, as my servers are too old). I use a version of the wrapping patch that I backported to postgres 8.1, which was prior to multibyte support, and much much simpler. What new formatting style are you referring to above? Did I modify what you sent as the original patch? I got this warning compiling: print.c:784: warning: pointer targets in passing argument 1 of ?mb_strlen_max_width? differ in signedness And I did have trouble applying the patch -- I had to manually give it the filename, and tell it to reverse the patch. OK, fixed. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote:
1) \pset columns XX should make it clear that's for file output only.
OK, docs updated.
2) There's an extra space, which breaks \pset border 2
717c717
fputc(' ', fout);;
---
fputc(' ', fout);
842c842
fputs( | , fout);
---
fputs( |, f
OK, got them fixed.
2) With \pset border 2, the far left border, for symmetry, should work
like the middle borders.
OK, how does it look now with this patch?
3) I'm getting bolder: how about having \pset format wrapped as the
default? Any downsides?
I think we are going to want it as the default for many psql
informational commands, like \df. Not sure about a more general
default. We were just discussing using \x as a default for wide output
but it seems this wrap style is probably a better solution than
switching for \x for wide columns (less distracting for the user and
cleaner). That will have to be a separate discussion once we are done.
Oh, I found a problem in my coding of the new wrap function I added.
While I handled the case where a character might span multiple bytes, I
assumed all characters had a display width of one. You can see from
pg_wcsformat()'s use of PQdsplen() that this isn't always the case. I
have modified the patch to properly use PQdsplen() but we are going to
need multi-byte users to test this once we are done.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/ref/psql-ref.sgml
===
RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v
retrieving revision 1.199
diff -c -c -r1.199 psql-ref.sgml
*** doc/src/sgml/ref/psql-ref.sgml 30 Mar 2008 18:10:20 - 1.199
--- doc/src/sgml/ref/psql-ref.sgml 21 Apr 2008 15:27:55 -
***
*** 1513,1519
listitem
para
Sets the output format to one of literalunaligned/literal,
! literalaligned/literal, literalhtml/literal,
literallatex/literal, or literaltroff-ms/literal.
Unique abbreviations are allowed. (That would mean one letter
is enough.)
--- 1513,1520
listitem
para
Sets the output format to one of literalunaligned/literal,
! literalaligned/literal, literalwrapped/literal,
! literalhtml/literal,
literallatex/literal, or literaltroff-ms/literal.
Unique abbreviations are allowed. (That would mean one letter
is enough.)
***
*** 1525,1532
is intended to create output that might be intended to be read
in by other programs (tab-separated, comma-separated).
quoteAligned/quote mode is the standard, human-readable,
! nicely formatted text output that is default. The
! quoteacronymHTML/acronym/quote and
quoteLaTeX/quote modes put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! (This might not be
--- 1526,1535
is intended to create output that might be intended to be read
in by other programs (tab-separated, comma-separated).
quoteAligned/quote mode is the standard, human-readable,
! nicely formatted text output that is default.
! quoteWrapped/quote is like literalaligned/ but wraps
! the output to fit the detected screen width or literal\pset
! columns/. The quoteacronymHTML/acronym/quote and
quoteLaTeX/quote modes put out tables that are intended to
be included in documents using the respective mark-up
language. They are not complete documents! (This might not be
***
*** 1537,1542
--- 1540,1556
/varlistentry
varlistentry
+ termliteralcolumns/literal/term
+ listitem
+ para
+ Controls the target width for the literalwrap/ format when
+ output is to a file, or the operating system does not support
+ screen width detection.
+ /para
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteralborder/literal/term
listitem
para
Index: src/bin/psql/command.c
===
RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.186
diff -c -c -r1.186 command.c
*** src/bin/psql/command.c 1 Jan 2008 19:45:55 - 1.186
--- src/bin/psql/command.c 21 Apr 2008 15:27:56 -
***
*** 1526,1531
--- 1526,1534
case PRINT_ALIGNED:
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote:
! /* print a divider, middle columns only
*/
! if ((j + 1) % col_count)
{
! if (opt_border == 0)
! fputc(' ', fout);
! /* first line for values? */
! else if (line_count == 0
col_line_count == 0)
! fputs( | , fout);
! /* next value is beyond height?
*/
! else if (line_count =
heights[j + 1])
! fputs( , fout);
! /* start of another newline
string? */
! else if (col_line_count == 0)
! fputs( : , fout);
! else
! {
! /* Does the next column
wrap to this line? */
! struct lineptr
*this_line = col_lineptrs[j+1][line_count];
! boolstring_done =
*(this_line-ptr + bytes_output[j+1]) == 0;
!
! fputs(string_done ?
: ; , fout);
! }
}
I think it's a bad idea to use the same : separator in the two last
cases. They are different and they should be displayed differently.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro Herrera wrote:
Bruce Momjian wrote:
! /* print a divider, middle columns only
*/
! if ((j + 1) % col_count)
{
! if (opt_border == 0)
! fputc(' ', fout);
! /* first line for values? */
! else if (line_count == 0
col_line_count == 0)
! fputs( | , fout);
! /* next value is beyond height?
*/
! else if (line_count =
heights[j + 1])
! fputs( , fout);
! /* start of another newline
string? */
! else if (col_line_count == 0)
! fputs( : , fout);
! else
! {
! /* Does the next column
wrap to this line? */
! struct lineptr
*this_line = col_lineptrs[j+1][line_count];
! boolstring_done =
*(this_line-ptr + bytes_output[j+1]) == 0;
!
! fputs(string_done ?
: ; , fout);
! }
}
I think it's a bad idea to use the same : separator in the two last
cases. They are different and they should be displayed differently.
I confirmed with Alvaro that he didn't notice the first uses a colon and
the second a semicolon, so he is OK.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro Herrera wrote: Bruce Momjian wrote: ! fputs(string_done ? : ; , fout); ! } } I think it's a bad idea to use the same : separator in the two last cases. They are different and they should be displayed differently. Bruce noted by IM that I misread the ? : expression :-) Sorry for the noise. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: I think it's a bad idea to use the same : separator in the two last cases. They are different and they should be displayed differently. I confirmed with Alvaro that he didn't notice the first uses a colon and the second a semicolon, so he is OK. FYI, I added a comment to the patch so others wouldn't confuse this. The ? : C syntax makes such confusion easy. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
The newline handling code was, by far, the most complex part of this patch. While I think it would be nicer to filter up past the newline, I just don't see this as a common need. Large text fields with newlines are difficult to really view in psql anyway, and they are likely to be the longest columns in any given query. Bottom line: not worth messing with. I'm split on the new formatting style. It looks a lot less grid-like, which I might like once I get used to it (which will be a while, because I can't run my own patch in daily use, as my servers are too old). I use a version of the wrapping patch that I backported to postgres 8.1, which was prior to multibyte support, and much much simpler. I got this warning compiling: print.c:784: warning: pointer targets in passing argument 1 of ‘mb_strlen_max_width’ differ in signedness And I did have trouble applying the patch -- I had to manually give it the filename, and tell it to reverse the patch. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
1) \pset columns XX should make it clear that's for file output only.
2) There's an extra space, which breaks \pset border 2
717c717
fputc(' ', fout);;
---
fputc(' ', fout);
842c842
fputs( | , fout);
---
fputs( |, f
2) With \pset border 2, the far left border, for symmetry, should work
like the middle borders.
3) I'm getting bolder: how about having \pset format wrapped as the
default? Any downsides?
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote:
Bruce Momjian wrote:
I don't think you need to do that to get it applied --- there is nothing
windows-specific in your code.
Is this ready to be applied? Do you want to send a final update or are
you still testing?
Looks good, but I suggest adding give up if the header is too wide:
if (tcolumns 0 tcolumns = total_header_width)
OK, I have created a new version of the patch, attached. Bryce, I
included your code line above and the other changes you had in the new
version of print.c.
I also set it to use ioctl() width if the output is to the terminal, and
\pset column for other cases, and adjusted the documentation.
I played a little with how wrapping would look with values that
contained newlines:
test= \pset format wrapped
Output format is wrapped.
test= \pset columns 70
Target column width for wrap format is 70.
test= select 1, 2, repeat('a', 80), repeat('b', 80), E'a\nb\nc', 1 from
generate_series(1,2)\g /rtmp/d
?column? | ?column? | repeat | repeat| ?column? | ?column?
--+--++-+--+--
1 |2 | aa | bbb | a|1
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
: b
: c
1 |2 | aa | bbb | a|1
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
; aa ; bbb
: b
: c
(2 rows)
Notice that the marker for wrapping is now ;. The marker is always on
the left edge, assuming you are not in the first column, in which case
there is no marker. This is how newline behaves and I copied that.
Also note that there is no separator for columns that don't wrap down
far enough. This is also how the newline case is handled. Is this the
way we want it to display? (Bryce, I am not sure your original code
handled this right.)
So, a query with only one column is going to be unclear whether it has
embedded newlines or wrapping has happening. One idea is for wrapping to
place a dash at the end of the value, so you have:
aa-
aa-
or something like that.
Another issue is whether newlines should filter up into the rows already
used for wrapping in adjacent columns. Right now it doesn't do that but
we could.
I found a problem with the original patch related to multibyte. The code
was fine up until the wrapping happened, at which point 'width' was
assumed to equal characters and the value was chopped into width byte
chunks. It has to be done in width character chunks so each chunk has
the same number of characters and you don't split a multi-byte character
across a line. I fixed it by creating a new function
mb_strlen_max_width().
I also restructured some of the code and added comments.
Community, I am looking for feedback on how to handle some of my
questions above.
Bryce, I am sorry this patch is taking so many iterations but the
feature has to work perfectly in lots of complex configurations so it
takes longer to complete and apply.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/ref/psql-ref.sgml
===
RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v
retrieving revision 1.199
diff -c -c -r1.199 psql-ref.sgml
*** doc/src/sgml/ref/psql-ref.sgml 30 Mar 2008 18:10:20 - 1.199
--- doc/src/sgml/ref/psql-ref.sgml 20 Apr 2008 03:53:39 -
***
*** 1513,1519
Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro Herrera wrote: * Don't lose warning comments like this one (unless you've removed the assumption of course) /* * Assumption: This code used only on strings * without multibyte characters, otherwise * this_line-width strlen(this_ptr) and we get * an overflow */ In fact, that particular assumption was causing a problem, causing a segfault. I can't be certain, because the multibyte stuff is pretty intense, but I think I nailed it. Thanks for all your comments! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut wrote: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I asked the folks over at Experts Exchange to test the behavior of the ioctl and $COLUMNS on various platforms. I'd been told that I would face huge problems if a console was resized. But the results were pretty consistent, and nothing had problems with resize: http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html It appears impossible to override $COLUMNS, on some platforms as the readline call sets it. On many platforms $COLUMNS is null until the call to readline. OSX does not set $COLUMNS at all. In short, I recommend the ioctl instead. In order to provide a way to wrap output to a pipe, I think a different mechanism will have to be found. -Bryce -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Tom Lane [EMAIL PROTECTED] writes: Bryce Nesbitt [EMAIL PROTECTED] writes: pre wrap=I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. [ Please get rid of the HTML formatting ... ] Bash can update the environment all it wants, but that will not affect what is seen by a program that's already running. Personally I often resize the window while psql is running, and I expect that to work. Hm, then having COLUMNS override the ioctl isn't such a good idea. Checking GNU ls source the ioctl overrides COLUMNS if it works, but there's a --width option which trumps both of the other two. I guess just a psql variable would be the equivalent. I'm with Peter on this one: we have used ioctl, and nothing else, to determine the vertical window dimension for many years now, to the tune of approximately zero complaints. It's going to take one hell of a strong argument to persuade me that determination of the horizontal dimension should not work exactly the same way. Well the cases are not analogous. Firstly, the window height doesn't actually alter the output. Secondly there's really no downside in a false positive since most pagers just exit if they decide the output fit on the screen -- which probably explains why no ssh users have complained... And in any case you can always override it by piping the output to a pager yourself -- which is effectively all I'm suggesting doing here. So here are your two hella-strong arguments: a) not all terminals support the ioctl. Emacs shell users may be eccentric but surely using psql over ssh isn't especially uncommon. Falling back to COLUMNS is standard, GNU ls is not alone, Solaris and FreeBSD both document supporting COLUMNS. b) you don't necessarily *want* the output formatted to fill the screen. You may be generating a report to email and want to set the width to the RFC recommended 72 characters. You may just have a full screen terminal but not enjoy reading 200-character long lines -- try it, it's really hard: MANWIDTH If $MANWIDTH is set, its value is used as the line length for which manual pages should be formatted. If it is not set, manual pages will be formatted with a line length appropriate to the current terminal (using an ioctl(2) if available, the value of $COLUMNS, or falling back to 80 characters if neither is available). Cat pages will only be saved when the default formatting can be used, that is when the terminal line length is between 66 and 80 characters. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut wrote: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I asked the folks over at Experts Exchange to test the behavior of the ioctl and $COLUMNS on various platforms. I'd been told that I would face huge problems if a console was resized. But the results were pretty consistent, and $COLUMNS had no problems with resize: http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html But appears impossible to override $COLUMNS, on some platforms as the readline call sets it. On many platforms $COLUMNS is null until the call to readline. OSX does not set $COLUMNS at all. So I think the ioctl should be used to determine the wrap width for terminals, and some other mechanism used for pipes. There's no way I'd want wrapping as the default for pipe output. I was not bold enough to propose that wrapping be the default behavior for the terminal. -Bryce -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt [EMAIL PROTECTED] writes: I asked the folks over at Experts Exchange to test the behavior of the ioctl I always thought that was a joke domain name, like Pen Island.com. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
On Fri, 18 Apr 2008 17:21:26 +0100 Gregory Stark [EMAIL PROTECTED] wrote: Bryce Nesbitt [EMAIL PROTECTED] writes: I asked the folks over at Experts Exchange to test the behavior of the ioctl I always thought that was a joke domain name, like Pen Island.com. Its not and a lot of postgresql users interact there. Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: In testing I found the regression tests were failing because of a divide by zero error (fixed), and a missing case where the column delimiter has to be :. In fact I now see all your line continuation cases using : rather than !. It actually looks better --- ! was too close to | to be easily recognized. (Did you update your patch to use :. I didn't see ! in your patch.) I think we should use a different separator when there is an actual newline in the data. Currently we have a : there, so using a : here is probably not the best idea. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote: Bruce Momjian wrote: I spent time reviewing your patch --- quite impressive. I have attached and updated version with mostly stylistic changes. In testing I found the regression tests were failing because of a divide by zero error (fixed), and a missing case where the column delimiter has to be :. In fact I now see all your line continuation cases using : rather than !. It actually looks better --- ! was too close to | to be easily recognized. (Did you update your patch to use :. I didn't see ! in your patch.) Nice! I'll merge with my current version. As you note I changed to :. Good, I thought so. I also found that for hugely wide output it was better to give up (do nothing), rather than mangle the output in a futile attempt to squash it to the window width. So there is one more clause in the wrapping if. Was it because of performance? I have a way to fix that by decrementing by more than one to shrink a column? I am attaching a new patch with my improved loop. It remembers the previous maximum ratio. I have tested on several unix flavors, but not on Windows or cygwin. I don't think you need to do that to get it applied --- there is nothing windows-specific in your code. Is this ready to be applied? Do you want to send a final update or are you still testing? -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/ref/psql-ref.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v retrieving revision 1.199 diff -c -c -r1.199 psql-ref.sgml *** doc/src/sgml/ref/psql-ref.sgml 30 Mar 2008 18:10:20 - 1.199 --- doc/src/sgml/ref/psql-ref.sgml 17 Apr 2008 14:05:39 - *** *** 1513,1519 listitem para Sets the output format to one of literalunaligned/literal, ! literalaligned/literal, literalhtml/literal, literallatex/literal, or literaltroff-ms/literal. Unique abbreviations are allowed. (That would mean one letter is enough.) --- 1513,1520 listitem para Sets the output format to one of literalunaligned/literal, ! literalaligned/literal, literalwrapped/literal, ! literalhtml/literal, literallatex/literal, or literaltroff-ms/literal. Unique abbreviations are allowed. (That would mean one letter is enough.) *** *** 1525,1531 is intended to create output that might be intended to be read in by other programs (tab-separated, comma-separated). quoteAligned/quote mode is the standard, human-readable, ! nicely formatted text output that is default. The quoteacronymHTML/acronym/quote and quoteLaTeX/quote modes put out tables that are intended to be included in documents using the respective mark-up --- 1526,1535 is intended to create output that might be intended to be read in by other programs (tab-separated, comma-separated). quoteAligned/quote mode is the standard, human-readable, ! nicely formatted text output that is default. ! quoteWrapped/quote is like literalaligned/ but wraps ! the output to fit the screen's width, based on the environment ! variable envarCOLUMNS/ or the autodetected width. The quoteacronymHTML/acronym/quote and quoteLaTeX/quote modes put out tables that are intended to be included in documents using the respective mark-up *** *** 2708,2713 --- 2712,2730 variablelist varlistentry + termenvarCOLUMNS/envar/term + + listitem + para + The character width to wrap output in literalwrapped/ format + mode. Many shells automatically update envarCOLUMNS/ when + a window is resized. If not set the screen width is automatically + detected, if possible. + /para + /listitem +/varlistentry + +varlistentry termenvarPAGER/envar/term listitem Index: src/bin/psql/command.c === RCS file: /cvsroot/pgsql/src/bin/psql/command.c,v retrieving revision 1.186 diff -c -c -r1.186 command.c *** src/bin/psql/command.c 1 Jan 2008 19:45:55 - 1.186 --- src/bin/psql/command.c 17 Apr 2008 14:05:39 - *** *** 1526,1531 --- 1526,1534 case PRINT_ALIGNED: return aligned; break; + case PRINT_WRAP: + return wrapped; + break; case PRINT_HTML:
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote:
I've attached a patch, against current 8.4 cvs, which optionally sets a
maximum width for psql output:
Some random comments:
* Don't use C++ style comments (//). Some compilers don't like these.
* Beware of brace position: we use braces on their own, indented at the
start of a new line, so
! while(--count) {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
becomes
! while(--count)
! {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
(with correct indentation anyway)
* Always use tabs, not spaces, to indent. Tabs are 4 spaces wide.
* Don't use double stars in comments.
* We're not in the habit of giving credit in code comments. It gets
messy fast.
* Don't lose warning comments like this one (unless you've removed the
assumption of course)
/*
* Assumption: This code used only on strings
* without multibyte characters, otherwise
* this_line-width strlen(this_ptr) and we get
* an overflow
*/
In fact I wonder if you've introduced this assumption in the other case
on that code (i.e. when alignment is not 'r'). I'm not seeing any
checks for multibytes in there, but perhaps I'm missing it.
* } else is forbidden too. Use two separate lines.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro is correct. I made most or all of these adjustments in the
updated version I posted yesterday.
---
Alvaro Herrera wrote:
Bryce Nesbitt wrote:
I've attached a patch, against current 8.4 cvs, which optionally sets a
maximum width for psql output:
Some random comments:
* Don't use C++ style comments (//). Some compilers don't like these.
* Beware of brace position: we use braces on their own, indented at the
start of a new line, so
! while(--count) {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
becomes
! while(--count)
! {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
(with correct indentation anyway)
* Always use tabs, not spaces, to indent. Tabs are 4 spaces wide.
* Don't use double stars in comments.
* We're not in the habit of giving credit in code comments. It gets
messy fast.
* Don't lose warning comments like this one (unless you've removed the
assumption of course)
/*
* Assumption: This code used only on strings
* without multibyte characters, otherwise
* this_line-width strlen(this_ptr) and we get
* an overflow
*/
In fact I wonder if you've introduced this assumption in the other case
on that code (i.e. when alignment is not 'r'). I'm not seeing any
checks for multibytes in there, but perhaps I'm missing it.
* } else is forbidden too. Use two separate lines.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
--
Bruce Momjian [EMAIL PROTECTED]http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: Alvaro is correct. I made most or all of these adjustments in the updated version I posted yesterday. Doh. I didn't realize you had posted a new version :-( People is complaining here that we don't teach people here anyway, so hopefully my comments were still useful :-) -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro Herrera wrote: Bruce Momjian wrote: Alvaro is correct. I made most or all of these adjustments in the updated version I posted yesterday. Doh. I didn't realize you had posted a new version :-( People is complaining here that we don't teach people here anyway, so hopefully my comments were still useful :-) Oh, yea, certainly. I didn't mention it to the author at first because it was his first patch, and he did a _very_ nice job considering the complexity of what he was doing. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut [EMAIL PROTECTED] writes: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I do that all the time. I normally am running under an emacs terminal so I don't know what width the ioctl's going to get back but it's unlikely to be right. In any case I may want to format the output to a width narrower than the window because I'm going to narrow it. Also, how would you suggest figuring the width to use for output going to a file? ioctl is irrelevant in that case, imho it should just default to 80 columns if COLUMNS is unset. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's Slony Replication support! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Gregory Stark [EMAIL PROTECTED] writes: Also, how would you suggest figuring the width to use for output going to a file? ioctl is irrelevant in that case, imho it should just default to 80 columns if COLUMNS is unset. It would be a spectacularly awful idea for this patch to affect the output to a file at all. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Tom Lane [EMAIL PROTECTED] writes: Gregory Stark [EMAIL PROTECTED] writes: Also, how would you suggest figuring the width to use for output going to a file? ioctl is irrelevant in that case, imho it should just default to 80 columns if COLUMNS is unset. It would be a spectacularly awful idea for this patch to affect the output to a file at all. It's a compromise of convenience over principle to allow the default output format to vary but I would still want to have the same set of output formats _available_ to me regardless of whether I'm redirecting to a file or not. Much like ls -C is available even if you're redirecting to a file and -1 is available if you're on a terminal. It sucks to run a program, decide you want to capture that output and find you get something else. It *really* sucks to find there's just no way to get the same output short of heroic efforts. I also have the converse problem occasionally. I run everything under emacs and occasionally run into programs which default to awkward output formats. Usually it's not too bad because it's still on a pty but the window width is a particular one which confuses programs. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's RemoteDBA services! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
It would be a spectacularly awful idea for this patch to affect the output to a file at all. . It sucks to run a program, decide you want to capture that output and find you get something else. It *really* sucks to find there's just no way to get the same output short of heroic efforts. I agree with Gregory here: I may want to capture either the wrapped or unwrapped output to a file or a pipe. Perhaps the enabling flag for this feature should take a parameter, which is the number of columns to wrap to. I was not bold enough to propose that wrapping be the default behavior for the terminal. And there's no way I'd want wrapping as the default for pipe output. -Bryce
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut wrote: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I asked the folks over at "Experts Exchange" to test the behavior of the ioctl and $COLUMNS on various platforms. I'd been told that I would face huge problems if a console was resized. But the results were pretty consistent, and nothing had problems with resize: http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html It appears impossible to override $COLUMNS, on some platforms as the readline call sets it. On many platforms $COLUMNS is null until the call to readline. OSX does not set $COLUMNS at all. -Bryce
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: I also found that for hugely wide output it was better to give up (do nothing), rather than mangle the output in a futile attempt to squash it to the window width. So there is one more clause in the wrapping if. Was it because of performance? I have a way to fix that by decrementing by more than one to shrink a column? I am attaching a new patch with my improved loop. It remembers the previous maximum ratio. No! Performance was not the issue. The out just looked like a jumble onscreen when the line was word wrapped BUT did not fit on the screen anyway. To increase the number of layouts that fit, a co-worker suggested I squeeze out the 2 spaces in each column header. -Bryce
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt [EMAIL PROTECTED] writes: pre wrap=I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. [ Please get rid of the HTML formatting ... ] Bash can update the environment all it wants, but that will not affect what is seen by a program that's already running. Personally I often resize the window while psql is running, and I expect that to work. I'm with Peter on this one: we have used ioctl, and nothing else, to determine the vertical window dimension for many years now, to the tune of approximately zero complaints. It's going to take one hell of a strong argument to persuade me that determination of the horizontal dimension should not work exactly the same way. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt wrote: I've attached a patch, against current 8.4 cvs, which optionally sets a maximum width for psql output: # \pset format aligned-wrapped # \pset border 2 # select * from distributors order by did; +--++-+---+ | did |name|descr| long_col_name | +--++-+---+ |1 | Food fish and wine | default | | |2 | Cat Food Heaven 2 | abcdefghijklmnopqrs ! | | || tuvwxyz | | |3 | Cat Food Heaven 3 | default | | | 10 | Lah| default | | | 12 | name | line one| | | 2892 ! short name | short | | | 8732 || | | +--++-+---+ (8 rows) The interactive terminal column width comes from char * temp = getenv(COLUMNS); Which has the strong advantage of great simplicity and portability. But it may not be 1000% universal. If $COLUMNS is not defined, the code bails to assuming an infinitely wide terminal. I will also backport this to Postgres 8.1, for my own use. Though the code is almost totally different in structure. I spent time reviewing your patch --- quite impressive. I have attached and updated version with mostly stylistic changes. In testing I found the regression tests were failing because of a divide by zero error (fixed), and a missing case where the column delimiter has to be :. In fact I now see all your line continuation cases using : rather than !. It actually looks better --- ! was too close to | to be easily recognized. (Did you update your patch to use :. I didn't see ! in your patch.) I have added an XXX comment questioning whether the loop to find the column to wrap is inefficient because it potentially loops over the length of the longest column and for each character loops over the number of columns. Not sure if that is a problem. I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? I don't think Win32 supports that ioctl(), does it? I added some comments and clarified some variable names. I also renamed the option to a shorter wrapped. I added documentation too. For testers compare: \df with: \pset format wrap \df Impressive! -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/ref/psql-ref.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/ref/psql-ref.sgml,v retrieving revision 1.199 diff -c -c -r1.199 psql-ref.sgml *** doc/src/sgml/ref/psql-ref.sgml 30 Mar 2008 18:10:20 - 1.199 --- doc/src/sgml/ref/psql-ref.sgml 17 Apr 2008 02:45:38 - *** *** 1513,1519 listitem para Sets the output format to one of literalunaligned/literal, ! literalaligned/literal, literalhtml/literal, literallatex/literal, or literaltroff-ms/literal. Unique abbreviations are allowed. (That would mean one letter is enough.) --- 1513,1520 listitem para Sets the output format to one of literalunaligned/literal, ! literalaligned/literal, literalwrapped/literal, ! literalhtml/literal, literallatex/literal, or literaltroff-ms/literal. Unique abbreviations are allowed. (That would mean one letter is enough.) *** *** 1525,1531 is intended to create output that might be intended to be read in by other programs (tab-separated, comma-separated). quoteAligned/quote mode is the standard, human-readable, ! nicely formatted text output that is default. The quoteacronymHTML/acronym/quote and quoteLaTeX/quote modes put out tables that are intended to be included in documents using the respective mark-up --- 1526,1535 is intended to create output that might be intended to be read in by other programs (tab-separated, comma-separated). quoteAligned/quote mode is the standard, human-readable, ! nicely formatted text output that is default. ! quoteWrapped/quote is like literalaligned/ but
Re: [PATCHES] Proposed patch - psql wraps at window width
Bruce Momjian wrote: I spent time reviewing your patch --- quite impressive. I have attached and updated version with mostly stylistic changes. In testing I found the regression tests were failing because of a divide by zero error (fixed), and a missing case where the column delimiter has to be :. In fact I now see all your line continuation cases using : rather than !. It actually looks better --- ! was too close to | to be easily recognized. (Did you update your patch to use :. I didn't see ! in your patch.) Nice! I'll merge with my current version. As you note I changed to :. I also found that for hugely wide output it was better to give up (do nothing), rather than mangle the output in a futile attempt to squash it to the window width. So there is one more clause in the wrapping if. I have tested on several unix flavors, but not on Windows or cygwin. -Bryce -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Proposed patch - psql wraps at window width
I've attached a patch, against current 8.4 cvs, which optionally sets a
maximum width for psql output:
# \pset format aligned-wrapped
# \pset border 2
# select * from distributors order by did;
+--++-+---+
| did |name|descr| long_col_name |
+--++-+---+
|1 | Food fish and wine | default | |
|2 | Cat Food Heaven 2 | abcdefghijklmnopqrs ! |
| || tuvwxyz | |
|3 | Cat Food Heaven 3 | default | |
| 10 | Lah| default | |
| 12 | name | line one| |
| 2892 ! short name | short | |
| 8732 || | |
+--++-+---+
(8 rows)
The interactive terminal column width comes from
char * temp = getenv(COLUMNS);
Which has the strong advantage of great simplicity and portability. But
it may not be 1000% universal. If $COLUMNS is not defined, the code
bails to assuming an infinitely wide terminal.
I will also backport this to Postgres 8.1, for my own use. Though the
code is almost totally different in structure.
Bryce Nesbitt
City CarShare San Francisco
? psql
? psql_wrapping.patch
Index: command.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.186
diff -c -r1.186 command.c
*** command.c 1 Jan 2008 19:45:55 - 1.186
--- command.c 5 Mar 2008 20:57:05 -
***
*** 1526,1531
--- 1526,1534
case PRINT_ALIGNED:
return aligned;
break;
+ case PRINT_ALIGNEDWRAP:
+ return aligned-wrapped;
+ break;
case PRINT_HTML:
return html;
break;
***
*** 1559,1564
--- 1562,1569
popt-topt.format = PRINT_UNALIGNED;
else if (pg_strncasecmp(aligned, value, vallen) == 0)
popt-topt.format = PRINT_ALIGNED;
+ else if (pg_strncasecmp(aligned-wrapped, value, vallen) == 0)
+ popt-topt.format = PRINT_ALIGNEDWRAP;
else if (pg_strncasecmp(html, value, vallen) == 0)
popt-topt.format = PRINT_HTML;
else if (pg_strncasecmp(latex, value, vallen) == 0)
***
*** 1567,1573
popt-topt.format = PRINT_TROFF_MS;
else
{
! psql_error(\\pset: allowed formats are unaligned, aligned, html, latex, troff-ms\n);
return false;
}
--- 1572,1578
popt-topt.format = PRINT_TROFF_MS;
else
{
! psql_error(\\pset: allowed formats are unaligned, aligned, aligned-wrapped, html, latex, troff-ms\n);
return false;
}
Index: mbprint.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/mbprint.c,v
retrieving revision 1.29
diff -c -r1.29 mbprint.c
*** mbprint.c 1 Jan 2008 19:45:56 - 1.29
--- mbprint.c 5 Mar 2008 20:57:06 -
***
*** 205,211
* pg_wcssize takes the given string in the given encoding and returns three
* values:
* result_width: Width in display character of longest line in string
! * result_hieght: Number of lines in display output
* result_format_size: Number of bytes required to store formatted representation of string
*/
int
--- 205,211
* pg_wcssize takes the given string in the given encoding and returns three
* values:
* result_width: Width in display character of longest line in string
! * result_height: Number of lines in display output
* result_format_size: Number of bytes required to store formatted representation of string
*/
int
***
*** 279,284
--- 279,288
return width;
}
+ /*
+ ** Filter out unprintable characters, companion to wcs_size.
+ ** Break input into lines (based on \n or \r).
+ */
void
pg_wcsformat(unsigned char *pwcs, size_t len, int encoding,
struct lineptr * lines, int count)
***
*** 353,364
}
len -= chlen;
}
! *ptr++ = '\0';
lines-width = linewidth;
! lines++;
! count--;
! if (count 0)
! lines-ptr = NULL;
}
unsigned char *
--- 357,370
}
len -= chlen;
}
! *ptr++ = '\0'; // Terminate formatted string
lines-width = linewidth;
! // Fill remaining array slots with null
! while(--count) {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
}
unsigned char *
Index: print.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/print.c,v
retrieving revision 1.96
diff -c -r1.96 print.c
*** print.c 1 Jan 2008 19:45:56 - 1.96
--- print.c 5 Mar 2008 20:57:07 -
