SEO

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 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbb
: b
: c
1 | 2 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; 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:

aaaaaaaaaa-
aaaaaaaaaa-
aaaa

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 <bruce@momjian.us> 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

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

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

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 <bruce@momjian.us> 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

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 <bruce@momjian.us> 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

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];
> ! bool string_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 Herrera http://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

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];
> > ! bool string_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 <bruce@momjian.us> 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

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 Herrera http://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

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 <bruce@momjian.us> 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