NetBSD/MIPS supports dlopen

Hi,

Recent version of NetBSD/MIPS support dlopen. This patch makes the
netbsd dynloader tests availability of dlopen on HAVE_DLOPEN rather
than on __mips__

Tested on NetBSD 4.99.20 on mips

I plan on registering a buildfarm member once this patch is in (and
maybe after upgrading to a more current NetBSD-current).

Regards,

Rémi Zara

Rémi Zara wrote:
> Hi,
>
> Recent version of NetBSD/MIPS support dlopen. This patch makes the
> netbsd dynloader tests availability of dlopen on HAVE_DLOPEN rather than
> on __mips__

Applied, thanks.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Rmi Zara wrote:
>> Recent version of NetBSD/MIPS support dlopen. This patch makes the
>> netbsd dynloader tests availability of dlopen on HAVE_DLOPEN rather than
>> on __mips__

> Applied, thanks.

Weird, I haven't seen the commit message come through here.

Anyway: (1) this should probably be back-patched; (2) please clean up
the ugly double negative here:

! #if !defined(HAVE_DLOPEN)
#else
dlclose(handle);
#endif

regards, tom lane

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

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Rmi Zara wrote:
> >> Recent version of NetBSD/MIPS support dlopen. This patch makes the
> >> netbsd dynloader tests availability of dlopen on HAVE_DLOPEN rather than
> >> on __mips__
>
> > Applied, thanks.
>
> Anyway: (1) this should probably be back-patched; (2) please clean up
> the ugly double negative here:

Both done -- I backpatched all the way down to 7.4 (and later I noticed
that the only 7.3 BF members are NetBSD).

> Weird, I haven't seen the commit message come through here.

Yeah, that's strange -- the (2) commit message got to me, but this one
hasn't.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> Weird, I haven't seen the commit message come through here.

> Yeah, that's strange -- the (2) commit message got to me, but this one
> hasn't.

Moderation filter got it for some reason? None of the back-patch
commits came through either, so there's something going on there...

regards, tom lane

Alvaro Herrera wrote:
> Both done -- I backpatched all the way down to 7.4 (and later I noticed
> that the only 7.3 BF members are NetBSD).
>
>

Haven't we declared 7.3 at EOL anyway?

cheers

andrew

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > Tom Lane wrote:
> >> Weird, I haven't seen the commit message come through here.
>
> > Yeah, that's strange -- the (2) commit message got to me, but this one
> > hasn't.
>
> Moderation filter got it for some reason?

Hmm, not moderation, because I am a moderator and didn't get it.

> None of the back-patch
> commits came through either, so there's something going on there...

Perhaps it's the fact that I used Rémi's name with the accent. I'll
check Majordomo logs if it lets me.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Andrew Dunstan wrote:
>
>
> Alvaro Herrera wrote:
>> Both done -- I backpatched all the way down to 7.4 (and later I noticed
>> that the only 7.3 BF members are NetBSD).
>
> Haven't we declared 7.3 at EOL anyway?

That's why I didn't backpatch it there. But if that's the case, why are
we still reporting 7.3 in the buildfarm status page?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > > Tom Lane wrote:
> > >> Weird, I haven't seen the commit message come through here.
> >
> > > Yeah, that's strange -- the (2) commit message got to me, but this one
> > > hasn't.

> > None of the back-patch
> > commits came through either, so there's something going on there...
>
> Perhaps it's the fact that I used Rémi's name with the accent. I'll
> check Majordomo logs if it lets me.

I checked the Majordomo logs and there's nothing about those patches.
I do see one message with the "Subject: pgsql: Clean up double negative,
per Tom Lane." line. A message held for moderation shows up in the logs
with a "stall" status. So these messages where chopped _before_ they
got into Majordomo at all ...

Perhaps a bug in the script that sends the email?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>> Tom Lane wrote:
>>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>>> Tom Lane wrote:
>>>>> Weird, I haven't seen the commit message come through here.
>>>> Yeah, that's strange -- the (2) commit message got to me, but this one
>>>> hasn't.
>
>>> None of the back-patch
>>> commits came through either, so there's something going on there...
>> Perhaps it's the fact that I used Rémi's name with the accent. I'll
>> check Majordomo logs if it lets me.
>
> I checked the Majordomo logs and there's nothing about those patches.
> I do see one message with the "Subject: pgsql: Clean up double negative,
> per Tom Lane." line. A message held for moderation shows up in the logs
> with a "stall" status. So these messages where chopped _before_ they
> got into Majordomo at all ...
>
> Perhaps a bug in the script that sends the email?

I see a bunch of emails from you leaving the system today. My first
guess for problem location would be the antispam, but before we rule out
the sender completely - at exactly what time was the commit(s) that made
it through made, and at what time was the commit(s) that didn't make it
through? In GMT time, please :-)

//Magnus

Alvaro Herrera wrote:
> Andrew Dunstan wrote:
>
>> Alvaro Herrera wrote:
>>
>>> Both done -- I backpatched all the way down to 7.4 (and later I noticed
>>> that the only 7.3 BF members are NetBSD).
>>>
>> Haven't we declared 7.3 at EOL anyway?
>>
>
> That's why I didn't backpatch it there. But if that's the case, why are
> we still reporting 7.3 in the buildfarm status page?
>
>

Because until a couple of weeks ago those two machines were still
reporting that branch. When they are 30 days old the reports will drop
off the page. (It looks like salamander has stopped altogether, which
Tom mentioned the other day would distress him some.)

cheers

andrew

Magnus Hagander wrote:
> Alvaro Herrera wrote:

>> I checked the Majordomo logs and there's nothing about those patches.
>> I do see one message with the "Subject: pgsql: Clean up double negative,
>> per Tom Lane." line. A message held for moderation shows up in the logs
>> with a "stall" status. So these messages where chopped _before_ they
>> got into Majordomo at all ...
>>
>> Perhaps a bug in the script that sends the email?
>
> I see a bunch of emails from you leaving the system today. My first
> guess for problem location would be the antispam, but before we rule out
> the sender completely - at exactly what time was the commit(s) that made
> it through made, and at what time was the commit(s) that didn't make it
> through? In GMT time, please :-)

Huh, I have zero idea and I had already closed the windows. So, from
the CVS logs:

revision 1.23
date: 2008/03/05 19:42:11; author: alvherre; state: Exp; lines: +4 -4
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
...
revision 1.12.4.1
date: 2008/03/05 21:20:49; author: alvherre; state: Exp; lines: +3 -4
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
----------------------------
revision 1.16.4.1
date: 2008/03/05 21:20:48; author: alvherre; state: Exp; lines: +3 -4
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
----------------------------
revision 1.17.2.1
date: 2008/03/05 21:20:49; author: alvherre; state: Exp; lines: +3 -4
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
----------------------------
revision 1.19.2.1
date: 2008/03/05 21:20:48; author: alvherre; state: Exp; lines: +4 -5
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
----------------------------
revision 1.22.2.1
date: 2008/03/05 21:20:47; author: alvherre; state: Exp; lines: +4 -5
Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.

These are all GMT AFAICT.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Mar 05, 2008 at 07:38:21PM -0300, Alvaro Herrera wrote:
> Magnus Hagander wrote:
> > Alvaro Herrera wrote:
>
> >> I checked the Majordomo logs and there's nothing about those patches.
> >> I do see one message with the "Subject: pgsql: Clean up double negative,
> >> per Tom Lane." line. A message held for moderation shows up in the logs
> >> with a "stall" status. So these messages where chopped _before_ they
> >> got into Majordomo at all ...
> >>
> >> Perhaps a bug in the script that sends the email?
> >
> > I see a bunch of emails from you leaving the system today. My first
> > guess for problem location would be the antispam, but before we rule out
> > the sender completely - at exactly what time was the commit(s) that made
> > it through made, and at what time was the commit(s) that didn't make it
> > through? In GMT time, please :-)
>
> Huh, I have zero idea and I had already closed the windows. So, from
> the CVS logs:

This is enough - I just wanted to be sure which commits we talked about.

> revision 1.23
> date: 2008/03/05 19:42:11; author: alvherre; state: Exp; lines: +4 -4
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.

19:42:11 I have one email going out to pgsql-committers from alvherre.

21:14:10 I have another, that doesn't match any of these commits. Did you
make anotherone that you didn't include in this list?

> ...
> revision 1.12.4.1
> date: 2008/03/05 21:20:49; author: alvherre; state: Exp; lines: +3 -4
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
> ----------------------------
> revision 1.16.4.1
> date: 2008/03/05 21:20:48; author: alvherre; state: Exp; lines: +3 -4
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
> ----------------------------
> revision 1.17.2.1
> date: 2008/03/05 21:20:49; author: alvherre; state: Exp; lines: +3 -4
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
> ----------------------------
> revision 1.19.2.1
> date: 2008/03/05 21:20:48; author: alvherre; state: Exp; lines: +4 -5
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
> ----------------------------
> revision 1.22.2.1
> date: 2008/03/05 21:20:47; author: alvherre; state: Exp; lines: +4 -5
> Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.

21:20:47 I count 1.
21:20:48 I count 2.
21:20:49 I count 2.

All these mails have been acknowledged as received by svr1.postgresql.org.-

So the script that sends them out is working properly. I'm back at blaming
the antispam for eating them before they got out to the list. Especially
since you didn't get a boucne (I assume you would've noticed if you did)

//Magnus

Magnus Hagander wrote:
> On Wed, Mar 05, 2008 at 07:38:21PM -0300, Alvaro Herrera wrote:

> > revision 1.23
> > date: 2008/03/05 19:42:11; author: alvherre; state: Exp; lines: +4 -4
> > Add support for dlopen on recent NetBSD/MIPS, per Rémi Zara.
>
> 19:42:11 I have one email going out to pgsql-committers from alvherre.
>
> 21:14:10 I have another, that doesn't match any of these commits. Did you
> make anotherone that you didn't include in this list?

Correct.

> So the script that sends them out is working properly. I'm back at blaming
> the antispam for eating them before they got out to the list. Especially
> since you didn't get a boucne (I assume you would've noticed if you did)

Hmm, so most likely they are in Maia's hands and only Marc can rescue
them.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Bryce Nesbitt wrote:
> I've attached a patch, against current 8.4 cvs, which optionally sets a
> maximum width for psql output:

I have added this patch to the May commitfest queue,

http://wiki.postgresql.org/wiki/CommitFest:May

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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

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 Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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).

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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 Herrera http://www.CommandPrompt.com/
> The PostgreSQL Company - Command Prompt, Inc.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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 Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Peter Eisentraut" <peter_e(at)gmx(dot)net> 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!

Gregory Stark <stark(at)enterprisedb(dot)com> 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

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> 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!

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=UTF-8" http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
<blockquote cite="mid:871w53bzsi(dot)fsf(at)oxford(dot)xeocode(dot)com" type="cite">
<blockquote type="cite">
<pre wrap="">It would be a spectacularly awful idea for this patch to affect the
output to a file at all.
</pre>
</blockquote>
<pre wrap=""><!---->.....
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.</pre>
</blockquote>
I agree with Gregory here: I may want to capture either the wrapped or
unwrapped output to a file or a pipe.<br>
Perhaps the enabling flag for this feature should take a parameter,
which is the number of columns to wrap to.<br>
<br>
I was not bold enough to propose that wrapping be the default behavior
for the terminal.<br>
And there's no way I'd want wrapping as the default for pipe output.<br>
<br>
                         -Bryce<br>
<br>
</body>
</html>

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
<br>
Peter Eisentraut wrote:
<blockquote cite="mid:200804171509(dot)25472(dot)peter_e(at)gmx(dot)net" type="cite">
<pre wrap="">Bruce Momjian wrote:
</pre>
<blockquote type="cite">
<pre wrap="">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?
</pre>
</blockquote>
<pre wrap=""><!---->
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).
</pre>
</blockquote>
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: 
<a class="moz-txt-link-freetext" href="http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html">http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html</a><br>
<br>
It appears impossible to override $COLUMNS, on some platforms as the
readline call sets it.<br>
On many platforms $COLUMNS is null until the call to readline.<br>
OSX does not set $COLUMNS at all.<br>
<br>
                         -Bryce<br>
<br>
</body>
</html>

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
Bruce Momjian wrote:<br>
<blockquote cite="mid:200804171407(dot)m3HE7EW24318(at)momjian(dot)us" type="cite">
<blockquote type="cite">
<pre wrap="">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.
</pre>
</blockquote>
<pre wrap=""><!---->
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.</pre>
</blockquote>
No!  Performance was not the issue.<br>
The out just looked like a jumble onscreen when the line was word
wrapped BUT did not fit on the screen anyway.<br>
<br>
To increase the number of layouts that fit, a co-worker suggested I
squeeze out the 2 spaces in each column header.<br>
<br>
                                          -Bryce<br>
<br>
</body>
</html>

Bryce Nesbitt <bryce2(at)obviously(dot)com> 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

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!

Alvaro Herrera wrote:
> People [are] complaining here that we don't teach people here anyway, so
> hopefully my comments were still useful :-)
>
Yes they are useful. As a new patcher, where should I look for coding
standards? How about a little FAQ at the
top of the CVS source tree?

Though, darn it, I sure like //

And my vi is set to:
set sw=4
set ts=4
set expandtab
Because my corporate projects require spaces not tabs.

> 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.
>
> * "} else" is forbidden too. Use two separate lines.

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)

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

Bryce Nesbitt wrote:
> Alvaro Herrera wrote:
> > People [are] complaining here that we don't teach people here
> > anyway, so hopefully my comments were still useful :-)
> >
> Yes they are useful. As a new patcher, where should I look for
> coding standards? How about a little FAQ at the
> top of the CVS source tree?
>
> Though, darn it, I sure like //
>
> And my vi is set to:
> set sw=4
> set ts=4
> set expandtab
> Because my corporate projects require spaces not tabs.

See the developer FAQ on the website. IIRC, it even contains what you
should put in your vi config file to make it do the right thing...

//Magnus

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Bryce Nesbitt <bryce2(at)obviously(dot)com> 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

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

Bryce Nesbitt wrote:
> Alvaro Herrera wrote:
>> People [are] complaining here that we don't teach people here anyway, so
>> hopefully my comments were still useful :-)
>>
> Yes they are useful. As a new patcher, where should I look for coding
> standards? How about a little FAQ at the
> top of the CVS source tree?

The developer's FAQ is supposed to contain this kind of thing, but I
think it's rather thin on actual details. (Some time ago it was
proposed to create a "style guide", but people here thought it was a
waste of time and "it will not cover what's really important", so we're
stuck with repeating the advice over and over.)

> Though, darn it, I sure like //
>
> And my vi is set to:
> set sw=4
> set ts=4
> set expandtab
> Because my corporate projects require spaces not tabs.

I use this:

:if match(getcwd(), "/home/alvherre/Code/CVS/pgsql") == 0
: set cinoptions=(0
: set tabstop=4
: set shiftwidth=4
: let $CSCOPE_DB=substitute(getcwd(), "^\\(.*/pgsql/source/[^/]*\\)/.*", "\\1", "")
: let &tags=substitute(getcwd(), "^\\(.*/pgsql/source/[^/]*\\)/.*", "\\1", "") . "/tags"
: let &path=substitute(getcwd(), "^\\(.*/pgsql/source/[^/]*\\)/.*", "\\1", "") . "/src/include"
:endif

Of course, you need to adjust the paths. The cscope, tags and path
things let me quickly navigate through the files, but they don't affect
the editor behavior.

I never set expandtab so it's not a problem for me, but I guess you can
do ":set noexpandtab" in that block to reset it for Postgres use.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

bryce2(at)obviously(dot)com (Bryce Nesbitt) writes:
> Alvaro Herrera wrote:
>> People [are] complaining here that we don't teach people here anyway, so
>> hopefully my comments were still useful :-)
>>
> Yes they are useful. As a new patcher, where should I look for coding
> standards? How about a little FAQ at the
> top of the CVS source tree?
>
> Though, darn it, I sure like //
>
> And my vi is set to:
> set sw=4
> set ts=4
> set expandtab
> Because my corporate projects require spaces not tabs.

Note that you can find config for vim and emacs to get them to support the coding standards in:

/opt/src/pgsql-HEAD/src/tools/editors/emacs.samples
/opt/src/pgsql-HEAD/src/tools/editors/vim.samples

For vim, the essentials are thus:

:if match(getcwd(), "/pgsql") >=0 || match(getcwd(), "/postgresql") >= 0
: set cinoptions=(0
: set tabstop=4
: set shiftwidth=4
:endif

The hooks are slightly different (though not by spectacularly much,
somewhat surprisingly) for Emacs...
--
let name="cbbrowne" and tld="cbbrowne.com" in name ^ "@" ^ tld;;
http://cbbrowne.com/info/advocacy.html
"A language that doesn't affect the way you think about programming,
is not worth knowing." -- Alan J. Perlis

"Bryce Nesbitt" <bryce2(at)obviously(dot)com> 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!

On Fri, 18 Apr 2008 17:21:26 +0100
Gregory Stark <stark(at)enterprisedb(dot)com> wrote:

> "Bryce Nesbitt" <bryce2(at)obviously(dot)com> 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

Alvaro Herrera wrote:
>> Yes they are useful. As a new patcher, where should I look for coding
>> standards? How about a little FAQ at the
>> top of the CVS source tree?
>>
>
> The developer's FAQ is supposed to contain this kind of thing, but I
> think it's rather thin on actual details. (Some time ago it was
> proposed to create a "style guide", but people here thought it was a
> waste of time and "it will not cover what's really important", so we're
> stuck with repeating the advice over and over.)
As a new patcher I'm certain to look at the top of the CVS tree. I'm
not so certain to actually find and read a FAQ. But I've got the advice
now, so I won't make the same missteps again....

-Bryce Nesbitt

PS: less is more, keep any guide very very short....

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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.

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?

Alvaro Herrera wrote:
> The developer's FAQ is supposed to contain this kind of thing, but I
> think it's rather thin on actual details.  (Some time ago it was
> proposed to create a "style guide", but people here thought it was a
> waste of time and "it will not cover what's really important", so we're
> stuck with repeating the advice over and over.)

Excellent wiki material ...

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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

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.

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I have moved this discussion to hackers in hopes of getting more
feedback, and moved the patch to a static URL:

ftp://momjian.us/pub/postgresql/mypatches/wrap

This patch adds a new '\pset format wrapped' mode that wraps long values
to fit the table on the user's screen, or in '\pset columns' columns in
an output to file or pipe. The documentation additions are at the top
of the patch.

Sample:

\pset format wrapped
Output format is wrapped.

\pset columns 70
Target column width for "wrap" format is 70.

SELECT 1, 2, repeat('a', 80), repeat('b', 80), E'a\nb\nc', 1
FROM generate_series(1,2)\g

?column? | ?column? | repeat | repeat | ?column? | ?column?
----------+----------+------------+-------------+----------+----------
1 | 2 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
; aaaaaaaaaa ; bbbbbbbbbbb : b
; aaaaaaaaaa ; bbbbbbbbbbb : c
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbb
1 | 2 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
; aaaaaaaaaa ; bbbbbbbbbbb : b
; aaaaaaaaaa ; bbbbbbbbbbb : c
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbbbbbbbbbb
; aaaaaaaaaa ; bbb
(2 rows)

You will notice:

o I have pulled up newline values to appear in the same rows
as the wrapped text
o Colons are used on the left for newline values
o Semicolons are used on the left for wrapped values
o There are no vertical bars for values that don't extend
to the wrapped or newline rows. This is how our
newline display has always worked so it was copied
by the wrapped code
o The left column has no indicator of wrapping or newlines
because there is no left border

We could use dashes to indicated wrapped values, but we don't. It would
be nice if someone could do some multi-byte testing of this,
particularly for characters that have a display width greater than one.

I think this patch is ready for application.

Should 'wrapped' be the default for certain operations, like \df?
'wrapped' mode is really good for a table that would fit the screen
width except for a few wide values.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, Apr 24, 2008 at 8:30 AM, Bruce Momjian
wrote:
> I have moved this discussion to hackers in hopes of getting more
> feedback, and moved the patch to a static URL:
>

Hi Bruce,

This is a very cool feature! Looking through the patch I did have a
few thoughts.

This is definitely going to introduce merge conflicts with my
printTable API patch. That's not a problem, just a "note to self"
that when/if this patch goes in I'll have to submit a fresh version of
my patch.

This psql message seemed a bit strange:

+ if (!quiet)
+ printf(_("Target column width for \"wrap\" format is %d.\n"),
popt->topt.columns);

To me, this message sounds like you're setting the width of a single
column, when in fact you're setting the target *total* width of the
table. I think this message would be more clear if it read "Target
output width ..." or "Target table width ...". Also, as far as the
user is concerned the format is referred to as "wrapped", not "wrap".

Cheers,
BJ

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFID72J5YBsbHkuyV0RAr45AKDETJTapg6c836Cr1RS8uk3gUUH0ACdH8mQ
M8ikE3VO+3H2/rt8AvhVoew=
=Jfnk
-----END PGP SIGNATURE-----

Brendan Jurd wrote:
> This is a very cool feature! Looking through the patch I did have a
> few thoughts.
>
> This is definitely going to introduce merge conflicts with my
> printTable API patch. That's not a problem, just a "note to self"
> that when/if this patch goes in I'll have to submit a fresh version of
> my patch.
>
> This psql message seemed a bit strange:
>
> + if (!quiet)
> + printf(_("Target column width for \"wrap\" format is %d.\n"),
> popt->topt.columns);
>
> To me, this message sounds like you're setting the width of a single
> column, when in fact you're setting the target *total* width of the
> table. I think this message would be more clear if it read "Target
> output width ..." or "Target table width ...". Also, as far as the
> user is concerned the format is referred to as "wrapped", not "wrap".

Good point. I have updated the text to be:

test=> \pset columns 70
Target width of file and pipe output for "wrap" format is 70.

Patch updated at the same URL.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> Brendan Jurd wrote:
>> To me, this message sounds like you're setting the width of a single
>> column, when in fact you're setting the target *total* width of the
>> table. I think this message would be more clear if it read "Target
>> output width ..." or "Target table width ...". Also, as far as the
>> user is concerned the format is referred to as "wrapped", not "wrap".
>
> Good point. I have updated the text to be:
>
> test=> \pset columns 70
> Target width of file and pipe output for "wrap" format is 70.

I think "file and pipe output" is short-sighted. There are lots more cases
this is necessary including SSH sessions and emacs shell buffers, etc. And as
I pointed out there are often cases where the user may want to override the
terminal width in any case.

Earlier I suggested -- and nobody refuted -- that we should follow the
precedents of ls and man and other tools which need to find the terminal
width: Explicitly set width takes precedence always, if it's not explicitly
set then you use the ioctl, and if that fails then you use the COLUMNS
environment variable.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Gregory Stark wrote:
> "Bruce Momjian" <bruce(at)momjian(dot)us> writes:
>
> > Brendan Jurd wrote:
> >> To me, this message sounds like you're setting the width of a single
> >> column, when in fact you're setting the target *total* width of the
> >> table. I think this message would be more clear if it read "Target
> >> output width ..." or "Target table width ...". Also, as far as the
> >> user is concerned the format is referred to as "wrapped", not "wrap".
> >
> > Good point. I have updated the text to be:
> >
> > test=> \pset columns 70
> > Target width of file and pipe output for "wrap" format is 70.
>
> I think "file and pipe output" is short-sighted. There are lots more cases
> this is necessary including SSH sessions and emacs shell buffers, etc. And as
> I pointed out there are often cases where the user may want to override the
> terminal width in any case.
>
> Earlier I suggested -- and nobody refuted -- that we should follow the
> precedents of ls and man and other tools which need to find the terminal
> width: Explicitly set width takes precedence always, if it's not explicitly
> set then you use the ioctl, and if that fails then you use the COLUMNS
> environment variable.

Yes, I like that better. Patch updated, same URL:

ftp://momjian.us/pub/postgresql/mypatches/wrap

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> Gregory Stark wrote:
>
>> Earlier I suggested -- and nobody refuted -- that we should follow the
>> precedents of ls and man and other tools which need to find the terminal
>> width: Explicitly set width takes precedence always, if it's not explicitly
>> set then you use the ioctl, and if that fails then you use the COLUMNS
>> environment variable.
>
> Yes, I like that better. Patch updated, same URL:
>
> ftp://momjian.us/pub/postgresql/mypatches/wrap

I think it should just be:

if (opt->format == PRINT_WRAP)
{
/* Get terminal width -- explicit setting takes precedence */
output_columns = opt->columns;

#ifdef TIOCGWINSZ
if (output_columns == 0 && isatty(fout))
{
struct winsize screen_size;

if (ioctl(fileno(fout), TIOCGWINSZ, &screen_size) != -1)
output_columns = screen_size.ws_col;
}
#endif

if (output_columns == 0)
{
const char *columns_env = getenv("COLUMNS");

if (columns_env)
output_columns = atoi(columns_env);
}

if (output_columns == 0)
output_columns = 79;
}

The differences this makes are that:

a) if you do -o /dev/tty (perhaps on some kind of cronjob) it will use the
ioctl.

b) If you dump to a file it will still respect COLUMNS. This might be a bit
weird since bash sets COLUMNS so your file width will be based on the size
of your terminal. But people also do things like COLUMNS=120 psql -o f ...

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Gregory Stark wrote:
> "Bruce Momjian" <bruce(at)momjian(dot)us> writes:
> > Gregory Stark wrote:
> >> Earlier I suggested -- and nobody refuted -- that we should follow the
> >> precedents of ls and man and other tools which need to find the terminal
> >> width: Explicitly set width takes precedence always, if it's not explicitly
> >> set then you use the ioctl, and if that fails then you use the COLUMNS
> >> environment variable.
> >
> > Yes, I like that better. Patch updated, same URL:
> >
> > ftp://momjian.us/pub/postgresql/mypatches/wrap
>
> I think it should just be:
>
> if (opt->format == PRINT_WRAP)
> {
> /* Get terminal width -- explicit setting takes precedence */
> output_columns = opt->columns;
>
> #ifdef TIOCGWINSZ
> if (output_columns == 0 && isatty(fout))
> {
> struct winsize screen_size;
>
> if (ioctl(fileno(fout), TIOCGWINSZ, &screen_size) != -1)
> output_columns = screen_size.ws_col;
> }
> #endif
>
> if (output_columns == 0)
> {
> const char *columns_env = getenv("COLUMNS");
>
> if (columns_env)
> output_columns = atoi(columns_env);
> }
>
> if (output_columns == 0)
> output_columns = 79;
> }
>
>
> The differences this makes are that:
>
> a) if you do -o /dev/tty (perhaps on some kind of cronjob) it will use the
> ioctl.

Uh, if you do that I am not sure what the user would want. I duplicated
what we do with PAGER and unless there is a clear mandate I think we
should keep the wrapping detection consistent with that; we have gotten
no complaints. Pager will not work for -o /dev/tty either.

> b) If you dump to a file it will still respect COLUMNS. This might be a bit
> weird since bash sets COLUMNS so your file width will be based on the size
> of your terminal. But people also do things like COLUMNS=120 psql -o f ...

No, that will make the regression tests fail and it is hard to say why
you would want a file wrap width to match your screen width.

Your final change is to assume a width of 79 if no columns are detected.
I also don't think that is a good idea, and if we want to do that we
would need to discuss that too.

I don't want to over-design this.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian escribió:

> > b) If you dump to a file it will still respect COLUMNS. This might be a bit
> > weird since bash sets COLUMNS so your file width will be based on the size
> > of your terminal. But people also do things like COLUMNS=120 psql -o f ...
>
> No, that will make the regression tests fail and it is hard to say why
> you would want a file wrap width to match your screen width.

What this means is that the regression tests should not use the wrapped
mode.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Am Donnerstag, 24. April 2008 schrieb Gregory Stark:
> b) If you dump to a file it will still respect COLUMNS. This might be a bit
>    weird since bash sets COLUMNS so your file width will be based on the
> size of your terminal. But people also do things like COLUMNS=120 psql -o f
> ...

Well, the feature is labeled "psql wraps at window width". If the output
isn't on a window, then it shouldn't wrap. I certainly don't want the output
to a file to depend on the size of the window at the time.

Perhaps having a variable inside psql to control this explicitly could be
useful for the case you describe.

Peter Eisentraut wrote:
> Am Donnerstag, 24. April 2008 schrieb Gregory Stark:
> > b) If you dump to a file it will still respect COLUMNS. This might be a bit
> > ? ?weird since bash sets COLUMNS so your file width will be based on the
> > size of your terminal. But people also do things like COLUMNS=120 psql -o f
> > ...
>
> Well, the feature is labeled "psql wraps at window width". If the output
> isn't on a window, then it shouldn't wrap. I certainly don't want the output
> to a file to depend on the size of the window at the time.
>
> Perhaps having a variable inside psql to control this explicitly could be
> useful for the case you describe.

\pset columns will wrap to the specified width for file output.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> Uh, if you do that I am not sure what the user would want. I duplicated
> what we do with PAGER and unless there is a clear mandate I think we
> should keep the wrapping detection consistent with that; we have gotten
> no complaints. Pager will not work for -o /dev/tty either.

I've explained before why I think the pager case is not analogous. In any case
a pager *can't* work if you do -o /dev/tty.

>> b) If you dump to a file it will still respect COLUMNS. This might be a bit
>> weird since bash sets COLUMNS so your file width will be based on the size
>> of your terminal. But people also do things like COLUMNS=120 psql -o f ...
>
> No, that will make the regression tests fail and it is hard to say why
> you would want a file wrap width to match your screen width.

Well you don't know where the pipe is going, it could be, for example, piped
to a pager.

I think the point is to keep the logic simple and not put in special cases
based on assumptions of what users might do. If you have simple logic which
doesn't do what the user expects in a corner case they understand and we can
tell them to override it with \pset. If you have logic which does what they
want normally but does something different sometimes based on criteria which
they consider irrelevant then they get angry.

> Your final change is to assume a width of 79 if no columns are detected.
> I also don't think that is a good idea, and if we want to do that we
> would need to discuss that too.

Well what width would you use if you have no better info?

> I don't want to over-design this.

I think your design is more complicated than mine. I've *removed* some of the
special cases from your logic. Mine is very straightforward: explicit user
setting takes precedence always, otherwise we try to use the ioctl and if that
fails fall back to COLUMNS.

Yours is "explicit setting takes precedence otherwise if you're on a terminal
and haven't redirected the output then ( we try the terminal if that fails
then we fall back to COLUMNS ) otherwise we ignore the ioctl and COLUMNS and
uh, I don't know what happens.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Peter Eisentraut wrote:
>> Well, the feature is labeled "psql wraps at window width". If the output
>> isn't on a window, then it shouldn't wrap.

> \pset columns will wrap to the specified width for file output.

I agree with Peter: that's a seriously bad idea.

regards, tom lane

Gregory Stark wrote:
> >> b) If you dump to a file it will still respect COLUMNS. This might be a bit
> >> weird since bash sets COLUMNS so your file width will be based on the size
> >> of your terminal. But people also do things like COLUMNS=120 psql -o f ...
> >
> > No, that will make the regression tests fail and it is hard to say why
> > you would want a file wrap width to match your screen width.
>
> Well you don't know where the pipe is going, it could be, for example, piped
> to a pager.
>
> I think the point is to keep the logic simple and not put in special cases
> based on assumptions of what users might do. If you have simple logic which
> doesn't do what the user expects in a corner case they understand and we can
> tell them to override it with \pset. If you have logic which does what they
> want normally but does something different sometimes based on criteria which
> they consider irrelevant then they get angry.

They can always do:

test=> \pset columns `echo $COLUMNS`
Target width for "wrap" format is 127.

My point is that we should do what most people _expect_, and the
majority of people here have stated they don't think wrap should modify
the file output _by_ _default_.

People who want a specific width for files should be setting their
desired width themselves, hence no need for the '79' default.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Peter Eisentraut wrote:
> >> Well, the feature is labeled "psql wraps at window width". If the output
> >> isn't on a window, then it shouldn't wrap.
>
> > \pset columns will wrap to the specified width for file output.
>
> I agree with Peter: that's a seriously bad idea.

Uh, I am confused. Are you saying \pset columns should not control file
output? And if it doesn't how does someone get file output in a
specified width?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
> > Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > > Peter Eisentraut wrote:
> > >> Well, the feature is labeled "psql wraps at window width". If the output
> > >> isn't on a window, then it shouldn't wrap.
> >
> > > \pset columns will wrap to the specified width for file output.
> >
> > I agree with Peter: that's a seriously bad idea.
>
> Uh, I am confused. Are you saying \pset columns should not control file
> output? And if it doesn't how does someone get file output in a
> specified width?

For reference Peter's comment is here:

http://archives.postgresql.org/pgsql-hackers/2008-04/msg01633.php

and the patch is here:

ftp://momjian.us/pub/postgresql/mypatches/wrap

with documentation.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> My point is that we should do what most people _expect_, and the
> majority of people here have stated they don't think wrap should modify
> the file output _by_ _default_.
>
> People who want a specific width for files should be setting their
> desired width themselves, hence no need for the '79' default.

Consider that this means that

psql -Pformat=wrapped -e 'select * from tab'

and

psql -Pformat=wrapped -e 'select * from tab' | more

will generate different output. That's bunk.

This is what I mean about trying to guess about what users expect. If you come
up with complicated logic trying to anticipate every case and get it wrong
users get angry.

If you just have simple rules which always apply then users understand they
they need to override them for their corner cases.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!

Gregory Stark wrote:
> "Bruce Momjian" <bruce(at)momjian(dot)us> writes:
>
> > My point is that we should do what most people _expect_, and the
> > majority of people here have stated they don't think wrap should modify
> > the file output _by_ _default_.
> >
> > People who want a specific width for files should be setting their
> > desired width themselves, hence no need for the '79' default.
>
> Consider that this means that
>
> psql -Pformat=wrapped -e 'select * from tab'
>
> and
>
> psql -Pformat=wrapped -e 'select * from tab' | more
>
> will generate different output. That's bunk.

'ls' and 'ls | more' generate different outputs, and I have never heard
anyone call that "bunk".

Also, this does work:

psql -Pformat=wrapped -Pcolumns=70 -c "select repeat('a', 100)" test | more

If you want non-terminal output to wrap, you have to specify the width
--- that seems only reasonable because the file case really needs to
have the width specified.

> This is what I mean about trying to guess about what users expect. If you come
> up with complicated logic trying to anticipate every case and get it wrong
> users get angry.
>
> If you just have simple rules which always apply then users understand they
> they need to override them for their corner cases.

You are going to need to find community members who support your
analysis if you want to make any headway in changing the patch.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> You are going to need to find community members who support your
> analysis if you want to make any headway in changing the patch.

Let's turn that around, shall we? I think at this point it's *you*
that are standing alone and need to find someone who agrees with
your approach.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > You are going to need to find community members who support your
> > analysis if you want to make any headway in changing the patch.
>
> Let's turn that around, shall we? I think at this point it's *you*
> that are standing alone and need to find someone who agrees with
> your approach.

I am confused exactly what people want changed in the patch. Some want
no control over wrapping in file output, and others want $COLUMN to
control column file output. The only person I am seeing code from is
Greg Stark, but I think most don't want his changes.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

* Bruce Momjian <bruce(at)momjian(dot)us> [080424 14:37]:

> I am confused exactly what people want changed in the patch. Some want
> no control over wrapping in file output, and others want $COLUMN to
> control column file output. The only person I am seeing code from is
> Greg Stark, but I think most don't want his changes.

If I had to vote as as user, Greg's suggestion made the most sense to
me. It was a simple approach that I could easily understand, and easily
envision using with and without a pager.

But basically, I pretty much *always* use a pager, having settled on
comfortable $LESS.

That said, I really don't see myself using the wrapped format so maybe
my vote shouldn't count.

a.
--
Aidan Van Dyk Create like a god,
aidan(at)highrise(dot)ca command like a king,
http://www.highrise.ca/ work like a slave.

"Bruce Momjian" <bruce(at)momjian(dot)us> writes:

> 'ls' and 'ls | more' generate different outputs, and I have never heard
> anyone call that "bunk".

The analogue of that would be making psql default to wrapped mode if isatty is
true and normal mode if it's false. I wouldn't be entirely against that but I
don't really care much either way.

Note that there's still -C and -1 to override that default. And if you specify
-C or isatty returns true it *always* uses the same deterministic logic to
determine the width: -w first, then ioctl, then COLUMNS.

> Also, this does work:
>
> psql -Pformat=wrapped -Pcolumns=70 -c "select repeat('a', 100)" test | more
>
> If you want non-terminal output to wrap, you have to specify the width
> --- that seems only reasonable because the file case really needs to
> have the width specified.

No it's not reasonable. I promise you users will report this as a bug.

This isn't anything new. Offhand I could only think of two precedents, ls and
man, but I'm sure there are others. They both use the same basic logic. And
not just GNU, FreeBSD and Solaris document the same behaviour.

I'm puzzled what you think should happen for the above. You think it should
just ignore the user's -Pformat=wrapped ?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Aidan Van Dyk wrote:
-- Start of PGP signed section.
> * Bruce Momjian <bruce(at)momjian(dot)us> [080424 14:37]:
>
> > I am confused exactly what people want changed in the patch. Some want
> > no control over wrapping in file output, and others want $COLUMN to
> > control column file output. The only person I am seeing code from is
> > Greg Stark, but I think most don't want his changes.
>
> If I had to vote as as user, Greg's suggestion made the most sense to
> me. It was a simple approach that I could easily understand, and easily
> envision using with and without a pager.
>
> But basically, I pretty much *always* use a pager, having settled on
> comfortable $LESS.
>
> That said, I really don't see myself using the wrapped format so maybe
> my vote shouldn't count.

Greg's suggestion is to use $COLUMNS if the width can't be determined
because the output is not directly to the screen. $COLUMNS is updated
by many shells.

The arguments I have heard are:

o wrapped never affects file/pipe output
o wrapped to file/pipe controlled only by \pset columns
o wrapped to file/pipe controlled by $COLUMNS
o wrapped to file/pipe controlled by \pset columns or $COLUMNS

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Gregory Stark wrote:
> "Bruce Momjian" <bruce(at)momjian(dot)us> writes:
>
> > 'ls' and 'ls | more' generate different outputs, and I have never heard
> > anyone call that "bunk".
>
> The analogue of that would be making psql default to wrapped mode if isatty is
> true and normal mode if it's false. I wouldn't be entirely against that but I
> don't really care much either way.

Yea, we have to discuss any default changes once we are done.

> Note that there's still -C and -1 to override that default. And if you specify
> -C or isatty returns true it *always* uses the same deterministic logic to
> determine the width: -w first, then ioctl, then COLUMNS.

Interesting. That is a powerful argument. I see if I do:

ls -C > /tmp/x

/tmp/x is wrapped, but if I make the window wider (on Ubuntu), the file
ouput is not wider. It seems to default to 72 columns as a target, even
though $COLUMNS is updated. This seems to indicate that 'ls' doesn't
check the terminal width at all when doing output.

What that would translate into for psql is that the interactive behavior
is as posted in the patch (try ioctl, then try $COLUMNS), but for
file/pipe, wrap is to 72 but can be overridden with \pset columns.

> > Also, this does work:
> >
> > psql -Pformat=wrapped -Pcolumns=70 -c "select repeat('a', 100)" test | more
> >
> > If you want non-terminal output to wrap, you have to specify the width
> > --- that seems only reasonable because the file case really needs to
> > have the width specified.
>
> No it's not reasonable. I promise you users will report this as a bug.
>
> This isn't anything new. Offhand I could only think of two precedents, ls and
> man, but I'm sure there are others. They both use the same basic logic. And
> not just GNU, FreeBSD and Solaris document the same behaviour.
>
> I'm puzzled what you think should happen for the above. You think it should
> just ignore the user's -Pformat=wrapped ?

Well, I have two people who don't want wrap to ever affect file/pipe
output, and now have two who want wrapped to try to affect file/pipe,
even if it has to read $COLUMNS. Obviously someone is going to be
unhappy in the end.

The only distinction I can think of is that 'ls' uses per-command flags,
while psql \pset format is for all commands, but you are kind of right
it is like 'ls'.

I can modify the patch for further review. I need feedback.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> 'ls' and 'ls | more' generate different outputs, and I have never heard
> anyone call that "bunk".

bunk

Bruce Momjian wrote:
> Greg's suggestion is to use $COLUMNS if the width can't be determined
> because the output is not directly to the screen.  $COLUMNS is updated
> by many shells.

I think it is best not to look at $COLUMNS at all. If the output is to a
terminal, then use ioctl to query the terminal. And provide a \pset command
to set a width explicitly, which can apply in all cases.

Peter Eisentraut wrote:
> Bruce Momjian wrote:
> > Greg's suggestion is to use $COLUMNS if the width can't be determined
> > because the output is not directly to the screen. ?$COLUMNS is updated
> > by many shells.
>
> I think it is best not to look at $COLUMNS at all. If the output is to a
> terminal, then use ioctl to query the terminal. And provide a \pset command
> to set a width explicitly, which can apply in all cases.

Yes, that is pretty much what we have now, except we try for $COLUMNS if
ioctl() fails for interactive use.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian escribió:
> Peter Eisentraut wrote:
> >
> > I think it is best not to look at $COLUMNS at all. If the output is to a
> > terminal, then use ioctl to query the terminal. And provide a \pset command
> > to set a width explicitly, which can apply in all cases.
>
> Yes, that is pretty much what we have now, except we try for $COLUMNS if
> ioctl() fails for interactive use.

On what platforms does ioctl() fail?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Bruce Momjian escribi?:
> > Peter Eisentraut wrote:
> > >
> > > I think it is best not to look at $COLUMNS at all. If the output is to a
> > > terminal, then use ioctl to query the terminal. And provide a \pset command
> > > to set a width explicitly, which can apply in all cases.
> >
> > Yes, that is pretty much what we have now, except we try for $COLUMNS if
> > ioctl() fails for interactive use.
>
> On what platforms does ioctl() fail?

I don't think Win32 supports it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> On what platforms does ioctl() fail?

On ssh for example.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:
>> On what platforms does ioctl() fail?

> On ssh for example.

That'd certainly be a showstopper if true, but it seems to be okay for
me. ssh'ing from an xterm window, and running psql on the remote side,
I can see with gdb that ioctl(TIOCGWINSZ) succeeds and gives a result
that correctly tracks window resizes --- indeed there are several bits
of psql that I'd never have committed if they didn't work in this
environment, because it's been my everyday work environment for years.

Maybe you are used to an ancient ssh version?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> "Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:
>>> On what platforms does ioctl() fail?
>
>> On ssh for example.
>
> That'd certainly be a showstopper if true, but it seems to be okay for
> me. ssh'ing from an xterm window, and running psql on the remote side,
> I can see with gdb that ioctl(TIOCGWINSZ) succeeds and gives a result
> that correctly tracks window resizes --- indeed there are several bits
> of psql that I'd never have committed if they didn't work in this
> environment, because it's been my everyday work environment for years.

Hum. It appears you're right. I did run a test earlier with ssh and strace
where I saw an error from the ioctl.

I have a sneaking suspicion I ran it within my usual emacs shell session
without realizing.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

I have updated the documentation for this patch. I consider it ready to
apply. I think it is as close to a middle ground as we are going to
get. Further adjustment will have to happen when we have more reports
from the field.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> I have moved this discussion to hackers in hopes of getting more
> feedback, and moved the patch to a static URL:
>
> ftp://momjian.us/pub/postgresql/mypatches/wrap
>
> This patch adds a new '\pset format wrapped' mode that wraps long values
> to fit the table on the user's screen, or in '\pset columns' columns in
> an output to file or pipe. The documentation additions are at the top
> of the patch.
>
> Sample:
>
> \pset format wrapped
> Output format is wrapped.
>
> \pset columns 70
> Target column width for "wrap" format is 70.
>
> SELECT 1, 2, repeat('a', 80), repeat('b', 80), E'a\nb\nc', 1
> FROM generate_series(1,2)\g
>
> ?column? | ?column? | repeat | repeat | ?column? | ?column?
> ----------+----------+------------+-------------+----------+----------
> 1 | 2 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
> ; aaaaaaaaaa ; bbbbbbbbbbb : b
> ; aaaaaaaaaa ; bbbbbbbbbbb : c
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbb
> 1 | 2 | aaaaaaaaaa | bbbbbbbbbbb | a | 1
> ; aaaaaaaaaa ; bbbbbbbbbbb : b
> ; aaaaaaaaaa ; bbbbbbbbbbb : c
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbbbbbbbbbb
> ; aaaaaaaaaa ; bbb
> (2 rows)
>
> You will notice:
>
> o I have pulled up newline values to appear in the same rows
> as the wrapped text
> o Colons are used on the left for newline values
> o Semicolons are used on the left for wrapped values
> o There are no vertical bars for values that don't extend
> to the wrapped or newline rows. This is how our
> newline display has always worked so it was copied
> by the wrapped code
> o The left column has no indicator of wrapping or newlines
> because there is no left border
>
> We could use dashes to indicated wrapped values, but we don't. It would
> be nice if someone could do some multi-byte testing of this,
> particularly for characters that have a display width greater than one.
>
> I think this patch is ready for application.
>
> Should 'wrapped' be the default for certain operations, like \df?
> 'wrapped' mode is really good for a table that would fit the screen
> width except for a few wide values.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> I have updated the documentation for this patch. I consider it ready to
> apply. I think it is as close to a middle ground as we are going to
> get. Further adjustment will have to happen when we have more reports
> from the field.
I heard a pretty compelling argument to make "wrapped" part of "aligned", and thus I think the patch is ready to go only after adjusting the user-facing syntax:

\pset border 2
\pset format aligned
Output format is aligned, no wrapping.

\pset format aligned autowrap
Output format is aligned, with automatic wrapping to the window width for terminals.

\pset format aligned 80
Output format is aligned, with a target width of 80 characters.

\a
Output format is unaligned, no wrapping.
\a
Output format is aligned, with a target width of 80 characters.

If the wrapping code can't fit the column headings in the wrap width, it gives up and produces aligned output. To do otherwise is totally unreadable. Please give the patch a try, before complaining about this particular aspect of it.

Bryce Nesbitt wrote:
> Bruce Momjian wrote:
> > I have updated the documentation for this patch. I consider it ready to
> > apply. I think it is as close to a middle ground as we are going to
> > get. Further adjustment will have to happen when we have more reports
> > from the field.
>
> I heard a pretty compelling argument to make "wrapped" part of "aligned",
> and thus I think the patch is ready to go only after adjusting the
> user-facing syntax:
>
>
> \pset border 2
> \pset format aligned
> Output format is aligned, no wrapping.
>
> \pset format aligned autowrap
> Output format is aligned, with automatic wrapping to the window width for terminals.
>
> \pset format aligned 80
> Output format is aligned, with a target width of 80 characters.
>
> \a
> Output format is unaligned, no wrapping.
> \a
> Output format is aligned, with a target width of 80 characters.
>
>
>
> If the wrapping code can't fit the column headings in the wrap width,
> it gives up and produces aligned output. To do otherwise is totally
> unreadable. Please give the patch a try, before complaining about this
> particular aspect of it.

Uh, well, we can do that, though looking at the psql code \pset only
wants two arguments. We would have to modify how \pset works. Also, I
am afraid making wrapping part of aligned is overly complicating the
API. For example, I specificially kept \pset columns rather than
allowing a third argument to pset because it was starting to look too
complicated to describe in the manual.

I am not 100% sure myself so hopefully others will comment.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Bryce Nesbitt wrote:
> > Bruce Momjian wrote:
> > > I have updated the documentation for this patch. I consider it ready to
> > > apply. I think it is as close to a middle ground as we are going to
> > > get. Further adjustment will have to happen when we have more reports
> > > from the field.
> >
> > I heard a pretty compelling argument to make "wrapped" part of "aligned",
> > and thus I think the patch is ready to go only after adjusting the
> > user-facing syntax:

I think this is what makes the most sense of what I've seen so far.
Wrapped is a special case of aligned anyway (there's no "wrapped
unaligned" or "wrapped HTML" for example, nor would we want there to
be.)

> > Output format is aligned, no wrapping.
> >
> > \pset format aligned autowrap
> > Output format is aligned, with automatic wrapping to the window width for terminals.
> >
> > \pset format aligned 80
> > Output format is aligned, with a target width of 80 characters.

> Uh, well, we can do that, though looking at the psql code \pset only
> wants two arguments.

I think that could be fixed easily by having the syntax be something
like

\pset format aligned:80
\pset format aligned:autowrap

etc.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

"Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> Bruce Momjian wrote:
>> Bryce Nesbitt wrote:
>> > Bruce Momjian wrote:
>> > > I have updated the documentation for this patch. I consider it ready to
>> > > apply. I think it is as close to a middle ground as we are going to
>> > > get. Further adjustment will have to happen when we have more reports
>> > > from the field.
>> >
>> > I heard a pretty compelling argument to make "wrapped" part of "aligned",
>> > and thus I think the patch is ready to go only after adjusting the
>> > user-facing syntax:
>
> I think this is what makes the most sense of what I've seen so far.
> Wrapped is a special case of aligned anyway (there's no "wrapped
> unaligned" or "wrapped HTML" for example, nor would we want there to
> be.)

Well there's no unaligned HTML or aligned unaligned either...

> I think that could be fixed easily by having the syntax be something
> like
>
> \pset format aligned:80
> \pset format aligned:autowrap

I suppose. It seems kind of inconvenient though, what advantage does it have?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's RemoteDBA services!

Gregory Stark wrote:
> "Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> > I think that could be fixed easily by having the syntax be something
> > like
> >
> > \pset format aligned:80
> > \pset format aligned:autowrap
>
> I suppose. It seems kind of inconvenient though, what advantage does it have?

Over what?

I think having a separate "\pset format wrapped" does not make much
sense. The fact that it's wrapped is not a new format in itself, just a
property of the aligned format.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

"Alvaro Herrera" <alvherre(at)commandprompt(dot)com> writes:

> The fact that it's wrapped is not a new format in itself, just a property of
> the aligned format.

Well I agree I just don't see any benefit to presenting it that way. I mean,
sure "wrapped" means "aligned and wrapped", but it's just shorthand notation
anyways.

In fact, it seems the only reason to separate the two like you're describing
would be if it *were* possible to have a wrapped html. Then your notation
could handle things "\pset format wrapped" couldn't:

\pset format html:autowrap

But I think we agree that isn't happening so why spell it "aligned:autowrap"
instead of just "wrap"?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning