psql: Count all table footer lines in pager setup

Patch looks good, applies and works. Needs a pgindent run:

-                       for (f = cont->footers; f; f = f->next) {
-                               int f_lines;
+                       for (f = cont->footers; f; f = f->next)
+                       {
+                               int                     f_lines;

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

On 2025-08-11 20:28 +0200, Greg Sabino Mullane wrote:

Patch looks good, applies and works. Needs a pgindent run:

Thanks for the review. Here's v2 with proper formatting.

--
Erik

Erik Wienhold <ewie@ewie.name> writes:

On 2025-08-11 20:28 +0200, Greg Sabino Mullane wrote:

Patch looks good, applies and works. Needs a pgindent run:

Thanks for the review. Here's v2 with proper formatting.

This appears to fix the problem it sets out to fix, but it looks
to me like there are adjacent problems of the same ilk; do you
feel like looking at those?

Specifically, I wondered whether we had the same bug with table
headers or cells that contain newlines. It looks like
print_aligned_text() is okay, because it counts up those newlines
and includes them in the extra_output_lines value that it passes
to IsPagerNeeded(). But print_aligned_vertical() and printTable()
have no such logic. Am I missing something about why those cases
need not consider this issue? If not, can we fix that by moving
all the "extra_lines" logic into IsPagerNeeded(), and if not
what shall we do about it?

regards, tom lane

On 2025-08-17 17:19 +0200, Tom Lane wrote:

Erik Wienhold <ewie@ewie.name> writes:

On 2025-08-11 20:28 +0200, Greg Sabino Mullane wrote:

Patch looks good, applies and works. Needs a pgindent run:

Thanks for the review. Here's v2 with proper formatting.

This appears to fix the problem it sets out to fix, but it looks
to me like there are adjacent problems of the same ilk; do you
feel like looking at those?

Specifically, I wondered whether we had the same bug with table
headers or cells that contain newlines. It looks like
print_aligned_text() is okay, because it counts up those newlines
and includes them in the extra_output_lines value that it passes
to IsPagerNeeded(). But print_aligned_vertical() and printTable()
have no such logic.

I looked into these cases now and the unaligned and expanded modes
indeed miss to use the pager. Tested with the following definitions and
patch v2 applied:

create function generate_lines(n int)
returns table (lines text)
language sql
as $$ select string_agg(s::text, e'\n') from generate_series(1, n) s $$;

-- This creates a view name and column name with 24 line breaks when truncated
-- to the default NAMEDATALEN (63 = 9*2 + 15*3)
select format('create view %I (c) as select null;'
'create view nl_column (%1$I) as select null;',
string_agg(s::text, e'\n'))
from generate_series(1, current_setting('max_identifier_length')::int) s
\gexec

Test results:

\pset format aligned \pset expanded off

1a) select * from generate_lines(25); -- uses pager (<= 27 lines)
1b) select * from nl_column; -- uses pager (<= 27 lines)
1c) \d nl_column -- uses pager (<= 27 lines)
1d) \d+ nl_column -- uses pager (<= 53 lines)
1e) \d "1<TAB> -- no pager
1f) \d+ "1<TAB> -- no pager

\pset format unaligned \pset expanded off

2a) select * from generate_lines(25); -- no pager
2b) select * from nl_column; -- no pager
2c) \d nl_column -- no pager
2d) \d+ nl_column -- uses pager (<= 28 lines)
2e) \d "1<TAB> -- no pager
2f) \d+ "1<TAB> -- no pager

\pset format unaligned \pset expanded on

3a) select * from generate_lines(25); -- no pager
3b) select * from nl_column; -- no pager
-- Expanded mode does not apply to \d -> same output as 2c to 2f
3c) \d nl_column -- no pager
3d) \d+ nl_column -- uses pager (<= 28 lines)
3e) \d "1<TAB> -- no pager
3f) \d+ "1<TAB> -- no pager

\pset format aligned \pset expanded on

4a) select * from generate_lines(25); -- no pager
4b) select * from nl_column -- no pager
-- Here as well: same output as 1c to 1f
4c) \d nl_column -- uses pager (<= 27 lines)
4d) \d+ nl_column -- uses pager (<= 53 lines)
4e) \d "1<TAB> -- no pager
4f) \d+ "1<TAB> -- no pager

Fixing the generate_lines cases goes without saying since that one is
about cells. But I'm not so sure if fixing the header cases (nl_column
and view u&"1\000a2\000a3...") is worth the effort. I've seen some
annoying uses of quoted identifiers in the past but so far haven't come
across identifiers with line breaks. (That would really take the cake!)
But of course, fixing those as well would tie up loose ends.

Am I missing something about why those cases need not consider this
issue?

I don't know either. The commits that added the IsPagerNeeded calls
with extra_lines=0 don't say anything about that.

If not, can we fix that by moving all the "extra_lines" logic into
IsPagerNeeded(), and if not what shall we do about it?

Looks doable. I'll prepare patch. Manually testing this with the
various printing options won't be fun though. Or are there tools to
simulate the terminal size? I'm using tmux so it's fairly easy to
create a horizonal split and resize the pane to be N lines high. But a
script to run psql and check if the pager was invoked for a terminal
with N lines would be cool.

--
Erik Wienhold

On 2025-08-19 03:52 +0200, Erik Wienhold wrote:

On 2025-08-17 17:19 +0200, Tom Lane wrote:

This appears to fix the problem it sets out to fix, but it looks
to me like there are adjacent problems of the same ilk; do you
feel like looking at those?

Specifically, I wondered whether we had the same bug with table
headers or cells that contain newlines. It looks like
print_aligned_text() is okay, because it counts up those newlines
and includes them in the extra_output_lines value that it passes
to IsPagerNeeded(). But print_aligned_vertical() and printTable()
have no such logic.

I looked into these cases now and the unaligned and expanded modes
indeed miss to use the pager.

Fixing the generate_lines cases goes without saying since that one is
about cells. But I'm not so sure if fixing the header cases (nl_column
and view u&"1\000a2\000a3...") is worth the effort.

If not, can we fix that by moving all the "extra_lines" logic into
IsPagerNeeded(), and if not what shall we do about it?

Looks doable. I'll prepare patch.

Here's v3 to address all of this. I split it into three separate
patches:

* v3-0001 is just a refactoring of the extra line counting, moving it
into IsPagerNeeded. This fixes the line counting for unaligned and
expanded output which, until now, called IsPagerNeeded with zero
extra_lines.

* v3-0002 fixes the line count in expanded mode when headers
contain more line breaks than the respective cells. This must be
checked for every cell because expanded mode repeats the headers for
every record. The header line counts are calculated once before
looping over all cells.

* v3-0003 is the original fix for the footer lines with two additions:
Here I also fix the counting of header lines in normal output mode
because I noticed that header lines are always counted right now even
when the header is not printed (in tuples_only mode or expanded mode.)
This now also considers the table title, if present.

Manually testing this with the various printing options won't be fun
though. Or are there tools to simulate the terminal size? I'm using
tmux so it's fairly easy to create a horizonal split and resize the
pane to be N lines high. But a script to run psql and check if the
pager was invoked for a terminal with N lines would be cool.

I also came up with the attached test-psql-pager.py script to run the
test cases from my previous post against different output modes to count
the maximum number of lines for which psql sill triggers the pager.
This uses termios to control the terminal size. The script compares the
actual output (pager line count, output mode, test case) against a *.out
file with the expected output (similar to pg_regress) which I've also
attached. The *.out files correspond the respective patch numbers to
show how each patch improves the pager setup. The expected output in
0-master.out is the baseline on master.

--
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:

Here's v3 to address all of this. I split it into three separate
patches:

Thanks! While reviewing this I decided that splitting it wasn't
such a great idea, because I kept getting distracted by obvious
bugs in the code you were copying around, only to realize that
the next patches fixed those bugs. So in the attached v4 I
merged these into one patch. It's mostly the same as yours
(I did find one outright bug, in a typo'd pg_malloc call),
but I split the line-counting logic into a new subroutine of
its own, which allows IsPagerNeeded to retain pretty much its
previous shape. There's some cosmetic changes too, mostly
expanding comments.

While I was looking at this I got annoyed at how many times we call
pg_wcssize. That's not tremendously cheap, and we're actually doing
it twice for every table cell, which is going to add up to a lot for
large tables. (We've had complaints before about the speed of psql
on big query results...) I'm not sure there is any great way to
avoid that altogether, but I did realize that we could skip the vast
majority of that work in the line-counting path if we recognize that
we can stop as soon as we know the table is longer than screen height.
So 0002 attached is a cut at doing that (which now shows why I wanted
the counting to be in its own subroutine).

I am not entirely sure that we should commit 0002 though; it may be
that the savings is down in the noise anyway once you consider all the
other work that happens while printing a big table. A positive reason
not to take it is something I realized while checking test coverage:
we never execute any of the maybe-use-the-pager branch of PageOutput
in the regression tests, because isatty(stdout) will always fail.
So that lack of coverage would also apply to count_table_lines in this
formulation, which is kind of sad. However, I'm not sure how useful
it really is to have code coverage there, since by this very token
the tests are paying zero attention to the value computed by
count_table_lines. It could be arbitrarily silly and we'd not see
that.

So, while I'm content with v4-0001, I'm not quite convinced about
v4-0002. WDYT?

regards, tom lane

On 2025-10-02 00:25 +0200, Tom Lane wrote:

Erik Wienhold <ewie@ewie.name> writes:

Here's v3 to address all of this. I split it into three separate
patches:

Thanks! While reviewing this I decided that splitting it wasn't
such a great idea, because I kept getting distracted by obvious
bugs in the code you were copying around, only to realize that
the next patches fixed those bugs. So in the attached v4 I
merged these into one patch. It's mostly the same as yours
(I did find one outright bug, in a typo'd pg_malloc call),
but I split the line-counting logic into a new subroutine of
its own, which allows IsPagerNeeded to retain pretty much its
previous shape. There's some cosmetic changes too, mostly
expanding comments.

+1 to factoring out the line counting.

While I was looking at this I got annoyed at how many times we call
pg_wcssize. That's not tremendously cheap, and we're actually doing
it twice for every table cell, which is going to add up to a lot for
large tables. (We've had complaints before about the speed of psql
on big query results...) I'm not sure there is any great way to
avoid that altogether, but I did realize that we could skip the vast
majority of that work in the line-counting path if we recognize that
we can stop as soon as we know the table is longer than screen height.
So 0002 attached is a cut at doing that (which now shows why I wanted
the counting to be in its own subroutine).

Yeah, I've noticed those repeated pg_wcssize calls as well but thought
that their overhead might me acceptable given how old the code is.
Limiting that overhead with the threshold parameter is a nifty idea.

I am not entirely sure that we should commit 0002 though; it may be
that the savings is down in the noise anyway once you consider all the
other work that happens while printing a big table. A positive reason
not to take it is something I realized while checking test coverage:
we never execute any of the maybe-use-the-pager branch of PageOutput
in the regression tests, because isatty(stdout) will always fail.
So that lack of coverage would also apply to count_table_lines in this
formulation, which is kind of sad. However, I'm not sure how useful
it really is to have code coverage there, since by this very token
the tests are paying zero attention to the value computed by
count_table_lines. It could be arbitrarily silly and we'd not see
that.

So, while I'm content with v4-0001, I'm not quite convinced about
v4-0002. WDYT?

I only glanced over the patches. Will take a closer look over the
weekend.

--
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:

On 2025-10-02 00:25 +0200, Tom Lane wrote:

I am not entirely sure that we should commit 0002 though; it may be
that the savings is down in the noise anyway once you consider all the
other work that happens while printing a big table. A positive reason
not to take it is something I realized while checking test coverage:
we never execute any of the maybe-use-the-pager branch of PageOutput
in the regression tests, because isatty(stdout) will always fail.

I realized that we could address that if we really wanted to,
using the same infrastructure as for tab-completion testing.
0001 and 0002 attached are the same as before, and then I added
0003 which adds a draft-quality TAP test. Code coverage checks
show that this adds only about 10 more lines of coverage in
psql proper, but in print.c most of the pager-related logic is
now covered.

regards, tom lane

BTW, I see from the cfbot that my v4-0002 patch is causing a
build warning on Windows:

print.c:3445:1: error: ‘count_table_lines’ defined but not used [-Werror=unused-function]

Evidently this is because the one call site is now within
"#ifdef TIOCGWINSZ", and Windows hasn't got that symbol.
So we'll need to likewise qualify count_table_lines.

I don't feel a need to post a new patch, since this is
a trivial fix and besides I'm not sure yet if we want
0002 at all.

regards, tom lane

v4-0001 LGTM. It still gives me the same pager line count as my v2.

On 2025-10-02 23:00 +0200, Tom Lane wrote:

Erik Wienhold <ewie@ewie.name> writes:

On 2025-10-02 00:25 +0200, Tom Lane wrote:

I am not entirely sure that we should commit 0002 though; it may be
that the savings is down in the noise anyway once you consider all
the other work that happens while printing a big table.

I see larger gains for queries that produce cells with many newlines,
benchmarked with a variation of my test-psql-pager.py script from
upthread. It measures the mtime of a file created before running psql
and a second file created by the pager using PAGER='touch somefile'
(that should cover the query execution time plus the formatting
overhead). With that I consistently measure these times for psql's
normal output format:

Query: SELECT repeat(e'foo\n', :n_lines) FROM generate_series(1, :n_rows)

n_lines | n_rows | time[ms] (master) | time[ms] (v4) | diff[%]
---------|----------|---------------------|---------------|---------
1 | 100000 | 30.000 | 26.667 | -11.11
1 | 1000000 | 200.000 | 173.333 | -13.33
1 | 10000000 | 1889.998 | 1613.332 | -14.63
10 | 10000 | 16.667 | 13.333 | -20.00
10 | 100000 | 73.333 | 50.000 | -31.82
10 | 1000000 | 583.333 | 390.000 | -33.14
100 | 1000 | 16.667 | 10.000 | -40.00
100 | 10000 | 60.000 | 36.667 | -38.89
100 | 100000 | 490.000 | 280.000 | -42.86
1000 | 100 | 16.667 | 13.333 | -20.00
1000 | 1000 | 56.667 | 33.333 | -41.18
1000 | 10000 | 483.333 | 260.000 | -46.21

Based on that I think you should push v4-0002.

A positive reason not to take it is something I realized while
checking test coverage: we never execute any of the
maybe-use-the-pager branch of PageOutput in the regression tests,
because isatty(stdout) will always fail.

I realized that we could address that if we really wanted to, using
the same infrastructure as for tab-completion testing. 0001 and 0002
attached are the same as before, and then I added 0003 which adds a
draft-quality TAP test. Code coverage checks show that this adds only
about 10 more lines of coverage in psql proper, but in print.c most of
the pager-related logic is now covered.

+1 on the TAP tests in general. But I'm not too familiar with the TAP
infrastructure to provide any meaningful review, given that you consider
it just draft-quality.

However, I'm not sure how useful it really is to have code coverage
there, since by this very token the tests are paying zero attention
to the value computed by count_table_lines. It could be
arbitrarily silly and we'd not see that.

Can we use something along the lines of my test-psql-pager.py to binary
search the line count at which psql uses the pager. Because it would
only apply to systems that provide termios.h, we might be less
constrained to find a portable solution that tests whether the pager
triggers or not (which you noted in v4-0003.)

--
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:

On 2025-10-02 00:25 +0200, Tom Lane wrote:

I am not entirely sure that we should commit 0002 though; it may be
that the savings is down in the noise anyway once you consider all
the other work that happens while printing a big table.

I see larger gains for queries that produce cells with many newlines,
benchmarked with a variation of my test-psql-pager.py script from
upthread. ...
Based on that I think you should push v4-0002.

OK, we'll keep it.

As I was doing some desultory manual testing, I realized that v4
still has a notable gap: it fails to account for the "(n rows)"
default footer. That's because that is not present in cont->footers
but is manually injected by footers_with_default, and count_table_rows
wasn't dealing with that. It's a little more complicated than just
calling that function, because the various vertical modes don't use
the default footer. After some code study I decided that we could
use the existing "expanded" flag to decide which footer definition
to apply, but I decided to rename it to "vertical" to reduce confusion
with the not-a-boolean cont->opt->expanded field.

Despite all this hackery, the count is still only really accurate
for the "\pset format aligned" modes. I figure we can leave
tidying up the other modes for another day, because I doubt that
anybody cares about interactive output of, say, LaTeX format.
(Perhaps unaligned mode would be the first priority to tidy up.)

Another inaccuracy that I find mildly annoying now that we've got it
mostly right is that we are not accounting for the blank line that
psql likes to print after the query result, so that you can still
end with the first output line scrolled off your screen despite
non-use of the pager. Not sure about a non-kluge way to count that,
though: it's outside the domain of print.c, and other callers might
not have a similar behavior.

I cleaned up the TAP test, too. This version redirects PAGER
to "wc -l", so that it's very obvious from the output whether
or not the pager was used.

regards, tom lane

I wrote:

Another inaccuracy that I find mildly annoying now that we've got it
mostly right is that we are not accounting for the blank line that
psql likes to print after the query result, so that you can still
end with the first output line scrolled off your screen despite
non-use of the pager. Not sure about a non-kluge way to count that,
though: it's outside the domain of print.c, and other callers might
not have a similar behavior.

Oh wait! That line *is* produced within print.c, so there is no
reason we shouldn't account for it. I spent a little extra effort
on unaligned mode too, since the TAP test is checking that case.
I still doubt that anyone's going to care a lot about the other
formats.

regards, tom lane

On 2025-10-07 00:45 +0200, Tom Lane wrote:

I wrote:

Another inaccuracy that I find mildly annoying now that we've got it
mostly right is that we are not accounting for the blank line that
psql likes to print after the query result, so that you can still
end with the first output line scrolled off your screen despite
non-use of the pager. Not sure about a non-kluge way to count that,
though: it's outside the domain of print.c, and other callers might
not have a similar behavior.

Oh wait! That line *is* produced within print.c, so there is no
reason we shouldn't account for it. I spent a little extra effort
on unaligned mode too, since the TAP test is checking that case.

LGTM.

I still doubt that anyone's going to care a lot about the other
formats.

Yup, I think so too.

--
Erik Wienhold

Erik Wienhold <ewie@ewie.name> writes:

On 2025-10-07 00:45 +0200, Tom Lane wrote:

Oh wait! That line *is* produced within print.c, so there is no
reason we shouldn't account for it. I spent a little extra effort
on unaligned mode too, since the TAP test is checking that case.

LGTM.

Pushed, after a tiny bit more comment-burnishing.

I still doubt that anyone's going to care a lot about the other
formats.

Yup, I think so too.

There's always room for a follow-on patch from someone who does
care.

regards, tom lane

On 2025-10-07 17:01 +0200, Tom Lane wrote:

Pushed, after a tiny bit more comment-burnishing.

Thank you for the extensive rework.

--
Erik Wienhold

Dear Team,

I worked on the pager issue in print.c and made the following updates:

-

Added a linecount() helper to correctly count all lines, including
footers.
-

In IsPagerNeeded , replaced the old logic lines++ with lines +=
linecount(f->data).

With this fix, the pager now triggers correctly for long outputs.
Please let me know if this way of fixing looks good, or if you have any
suggestions for improvement.

Best regards,
[lakshmi G]

On Wed, Oct 8, 2025 at 2:40 PM Erik Wienhold <ewie@ewie.name> wrote: