Re: psql: Count all table footer lines in pager setup

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

From 0b6780211acafa3504b47692f87b24f891dbefe0 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 1 Oct 2025 17:25:00 -0400
Subject: [PATCH v4 1/2] Improve psql's ability to select pager mode
 accurately.

The code in print.c that's concerned with counting the number of lines
that will be needed missed a lot of edge cases:

* While plain aligned mode accounted for embedded newlines in column
headers and table cells, unaligned and expanded output modes did not.

* In particular, since expanded mode repeats the headers for each
record, we need to account for embedded newlines in the headers for
each record.

* Multi-line table titles were not accounted for.

* tuples_only mode (where headers aren't printed) wasn't accounted
for.

* Footers were accounted for as one line per footer, again missing
the possibility of multi-line footers.  (In some cases such as
"\d+" on a view, there can be many lines in a footer.)

To fix, move the entire responsibility for counting lines into
IsPagerNeeded (or actually, into a new subroutine count_table_lines),
and then expand the logic as appropriate.  Also restructure to make
it perhaps a bit easier to follow.

Author: Erik Wienhold <ewie@ewie.name>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/2dd2430f-dd20-4c89-97fd-242616a3d768@ewie.name
---
 src/fe_utils/print.c | 199 ++++++++++++++++++++++++++++++-------------
 1 file changed, 138 insertions(+), 61 deletions(-)

diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 4af0f32f2fc..71bc14d499b 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -266,8 +266,13 @@ static const unicodeStyleFormat unicode_style = {

 /* Local functions */
 static int    strlen_max_width(unsigned char *str, int *target_width, int encoding);
-static void IsPagerNeeded(const printTableContent *cont, int extra_lines, bool expanded,
+static void IsPagerNeeded(const printTableContent *cont,
+                          const unsigned int *width_wrap,
+                          bool expanded,
                           FILE **fout, bool *is_pager);
+static int    count_table_lines(const printTableContent *cont,
+                              const unsigned int *width_wrap,
+                              bool expanded);

 static void print_aligned_vertical(const printTableContent *cont,
                                    FILE *fout, bool is_pager);
@@ -656,8 +661,6 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
     unsigned char **format_buf;
     unsigned int width_total;
     unsigned int total_header_width;
-    unsigned int extra_row_output_lines = 0;
-    unsigned int extra_output_lines = 0;

     const char *const *ptr;

@@ -722,14 +725,9 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
             max_nl_lines[i] = nl_lines;
         if (bytes_required > max_bytes[i])
             max_bytes[i] = bytes_required;
-        if (nl_lines > extra_row_output_lines)
-            extra_row_output_lines = nl_lines;

         width_header[i] = width;
     }
-    /* Add height of tallest header column */
-    extra_output_lines += extra_row_output_lines;
-    extra_row_output_lines = 0;

     /* scan all cells, find maximum width, compute cell_count */
     for (i = 0, ptr = cont->cells; *ptr; ptr++, i++, cell_count++)
@@ -889,43 +887,10 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
         is_pager = is_local_pager = true;
     }

-    /* Check if newlines or our wrapping now need the pager */
-    if (!is_pager && fout == stdout)
+    /* Check if there are enough lines to require the pager */
+    if (!is_pager)
     {
-        /* scan all cells, find maximum width, compute cell_count */
-        for (i = 0, ptr = cont->cells; *ptr; ptr++, cell_count++)
-        {
-            int            width,
-                        nl_lines,
-                        bytes_required;
-
-            pg_wcssize((const unsigned char *) *ptr, strlen(*ptr), encoding,
-                       &width, &nl_lines, &bytes_required);
-
-            /*
-             * A row can have both wrapping and newlines that cause it to
-             * display across multiple lines.  We check for both cases below.
-             */
-            if (width > 0 && width_wrap[i])
-            {
-                unsigned int extra_lines;
-
-                /* don't count the first line of nl_lines - it's not "extra" */
-                extra_lines = ((width - 1) / width_wrap[i]) + nl_lines - 1;
-                if (extra_lines > extra_row_output_lines)
-                    extra_row_output_lines = extra_lines;
-            }
-
-            /* i is the current column number: increment with wrap */
-            if (++i >= col_count)
-            {
-                i = 0;
-                /* At last column of each row, add tallest column height */
-                extra_output_lines += extra_row_output_lines;
-                extra_row_output_lines = 0;
-            }
-        }
-        IsPagerNeeded(cont, extra_output_lines, false, &fout, &is_pager);
+        IsPagerNeeded(cont, width_wrap, false, &fout, &is_pager);
         is_local_pager = is_pager;
     }

@@ -1376,7 +1341,7 @@ print_aligned_vertical(const printTableContent *cont,
      */
     if (!is_pager)
     {
-        IsPagerNeeded(cont, 0, true, &fout, &is_pager);
+        IsPagerNeeded(cont, NULL, true, &fout, &is_pager);
         is_local_pager = is_pager;
     }

@@ -3398,37 +3363,149 @@ printTableCleanup(printTableContent *const content)
  * IsPagerNeeded
  *
  * Setup pager if required
+ *
+ * cont: table data to be printed
+ * width_wrap[]: per-column maximum width, or NULL if caller will not wrap
+ * expanded: expanded mode?
+ * fout: where to print to (in/out argument)
+ * is_pager: output argument
+ *
+ * If we decide pager is needed, *fout is modified and *is_pager is set true
  */
 static void
-IsPagerNeeded(const printTableContent *cont, int extra_lines, bool expanded,
+IsPagerNeeded(const printTableContent *cont, const unsigned int *width_wrap,
+              bool expanded,
               FILE **fout, bool *is_pager)
 {
     if (*fout == stdout)
     {
         int            lines;

+        lines = count_table_lines(cont, width_wrap, expanded);
+
+        *fout = PageOutput(lines, cont->opt);
+        *is_pager = (*fout != stdout);
+    }
+    else
+        *is_pager = false;
+}
+
+/*
+ * Count the number of lines needed to print the given table.
+ *
+ * cont: table data to be printed
+ * width_wrap[]: per-column maximum width, or NULL if caller will not wrap
+ * expanded: expanded mode?
+ *
+ * We currently handle only regular and expanded modes here.
+ */
+static int
+count_table_lines(const printTableContent *cont,
+                  const unsigned int *width_wrap,
+                  bool expanded)
+{
+    int           *header_height;
+    int            lines,
+                max_lines = 0,
+                nl_lines,
+                i;
+    int            encoding = cont->opt->encoding;
+    const char *const *cell;
+
+    /*
+     * Scan all column headers and determine their heights.  Cache the values
+     * since expanded mode repeats the headers for every record.
+     */
+    header_height = (int *) pg_malloc(cont->ncolumns * sizeof(int));
+    for (i = 0; i < cont->ncolumns; i++)
+    {
+        pg_wcssize((const unsigned char *) cont->headers[i],
+                   strlen(cont->headers[i]), encoding,
+                   NULL, &header_height[i], NULL);
+    }
+
+    /*
+     * Expanded mode writes one separator line per record.  Normal mode writes
+     * a single separator line between header and rows.
+     */
+    lines = expanded ? cont->nrows : 1;
+
+    /* Scan all cells to count their lines */
+    for (i = 0, cell = cont->cells; *cell; cell++)
+    {
+        int            width;
+
+        /* Count the original line breaks */
+        pg_wcssize((const unsigned char *) *cell, strlen(*cell), encoding,
+                   &width, &nl_lines, NULL);
+
+        /* Count extra lines due to wrapping */
+        if (width > 0 && width_wrap && width_wrap[i])
+            nl_lines += (width - 1) / width_wrap[i];
+
         if (expanded)
-            lines = (cont->ncolumns + 1) * cont->nrows;
+        {
+            /* Pick the height of the header or cell, whichever is taller */
+            if (nl_lines > header_height[i])
+                lines += nl_lines;
+            else
+                lines += header_height[i];
+        }
         else
-            lines = cont->nrows + 1;
+        {
+            /* Remember max height in the current row */
+            if (nl_lines > max_lines)
+                max_lines = nl_lines;
+        }

-        if (!cont->opt->tuples_only)
+        /* i is the current column number: increment with wrap */
+        if (++i >= cont->ncolumns)
         {
-            printTableFooter *f;
+            i = 0;
+            if (!expanded)
+            {
+                /* At last column of each row, add tallest column height */
+                lines += max_lines;
+                max_lines = 0;
+            }
+        }
+    }

-            /*
-             * FIXME -- this is slightly bogus: it counts the number of
-             * footers, not the number of lines in them.
-             */
-            for (f = cont->footers; f; f = f->next)
-                lines++;
+    /* Account for header and footer decoration */
+    if (!cont->opt->tuples_only)
+    {
+        if (cont->title)
+        {
+            /* Add height of title */
+            pg_wcssize((const unsigned char *) cont->title, strlen(cont->title),
+                       encoding, NULL, &nl_lines, NULL);
+            lines += nl_lines;
         }

-        *fout = PageOutput(lines + extra_lines, cont->opt);
-        *is_pager = (*fout != stdout);
+        if (!expanded)
+        {
+            /* Add height of tallest header column */
+            max_lines = 0;
+            for (i = 0; i < cont->ncolumns; i++)
+            {
+                if (header_height[i] > max_lines)
+                    max_lines = header_height[i];
+            }
+            lines += max_lines;
+        }
+
+        /* Add all footer lines */
+        for (printTableFooter *f = cont->footers; f; f = f->next)
+        {
+            pg_wcssize((const unsigned char *) f->data, strlen(f->data),
+                       encoding, NULL, &nl_lines, NULL);
+            lines += nl_lines;
+        }
     }
-    else
-        *is_pager = false;
+
+    free(header_height);
+
+    return lines;
 }

 /*
@@ -3456,7 +3533,7 @@ printTable(const printTableContent *cont,
         cont->opt->format != PRINT_ALIGNED &&
         cont->opt->format != PRINT_WRAPPED)
     {
-        IsPagerNeeded(cont, 0, (cont->opt->expanded == 1), &fout, &is_pager);
+        IsPagerNeeded(cont, NULL, (cont->opt->expanded == 1), &fout, &is_pager);
         is_local_pager = is_pager;
     }

--
2.43.7

From d4fe1827f338e38ed5dbc2042ab199fb09954fbd Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 1 Oct 2025 18:04:02 -0400
Subject: [PATCH v4 2/2] Make some minor performance improvements in psql's
 line-counting.

Arrange to not run count_table_lines at all unless we will use
its result, and teach it to quit early as soon as it's proven
that the output is long enough to require use of the pager.
When dealing with large tables this should save a noticeable
amount of time, since pg_wcssize() isn't exactly cheap.

In passing, fix print_aligned_text to avoid using repeated
"i % col_count" when it could just make the value of i wrap
around.  In a quick check with the version of gcc I'm using,
the compiler is smart enough to recognize the common subexpressions
and issue only one integer divide per loop, but it's not smart enough
to avoid the integer divide altogether.  In any case, this coding
was randomly unlike the way it's done in count_table_lines.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/2dd2430f-dd20-4c89-97fd-242616a3d768@ewie.name
---
 src/fe_utils/print.c | 97 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 24 deletions(-)

diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 71bc14d499b..d8c8d7d2a3b 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -266,13 +266,18 @@ static const unicodeStyleFormat unicode_style = {

 /* Local functions */
 static int    strlen_max_width(unsigned char *str, int *target_width, int encoding);
+static FILE *PageOutputInternal(int lines, const printTableOpt *topt,
+                                const printTableContent *cont,
+                                const unsigned int *width_wrap,
+                                bool expanded);
 static void IsPagerNeeded(const printTableContent *cont,
                           const unsigned int *width_wrap,
                           bool expanded,
                           FILE **fout, bool *is_pager);
 static int    count_table_lines(const printTableContent *cont,
                               const unsigned int *width_wrap,
-                              bool expanded);
+                              bool expanded,
+                              int threshold);

 static void print_aligned_vertical(const printTableContent *cont,
                                    FILE *fout, bool is_pager);
@@ -730,7 +735,7 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
     }

     /* scan all cells, find maximum width, compute cell_count */
-    for (i = 0, ptr = cont->cells; *ptr; ptr++, i++, cell_count++)
+    for (i = 0, ptr = cont->cells; *ptr; ptr++, cell_count++)
     {
         int            width,
                     nl_lines,
@@ -739,14 +744,18 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
         pg_wcssize((const unsigned char *) *ptr, strlen(*ptr), encoding,
                    &width, &nl_lines, &bytes_required);

-        if (width > max_width[i % col_count])
-            max_width[i % col_count] = width;
-        if (nl_lines > max_nl_lines[i % col_count])
-            max_nl_lines[i % col_count] = nl_lines;
-        if (bytes_required > max_bytes[i % col_count])
-            max_bytes[i % col_count] = bytes_required;
+        if (width > max_width[i])
+            max_width[i] = width;
+        if (nl_lines > max_nl_lines[i])
+            max_nl_lines[i] = nl_lines;
+        if (bytes_required > max_bytes[i])
+            max_bytes[i] = bytes_required;
+
+        width_average[i] += width;

-        width_average[i % col_count] += width;
+        /* i is the current column number: increment with wrap */
+        if (++i >= col_count)
+            i = 0;
     }

     /* If we have rows, compute average */
@@ -3046,12 +3055,31 @@ set_sigpipe_trap_state(bool ignore)
 /*
  * PageOutput
  *
- * Tests if pager is needed and returns appropriate FILE pointer.
+ * Tests if pager is needed and returns appropriate FILE pointer
+ * (either a pipe, or stdout if we don't need the pager).
+ *
+ * lines: number of lines that will be printed
+ * topt: print formatting options
  *
  * If the topt argument is NULL no pager is used.
  */
 FILE *
 PageOutput(int lines, const printTableOpt *topt)
+{
+    return PageOutputInternal(lines, topt, NULL, NULL, false);
+}
+
+/*
+ * Private version that allows for line-counting to be avoided when
+ * not needed.  If "cont" is not null then the input value of "lines"
+ * is ignored and we count lines based on cont + width_wrap + expanded
+ * (see count_table_lines).
+ */
+static FILE *
+PageOutputInternal(int lines, const printTableOpt *topt,
+                   const printTableContent *cont,
+                   const unsigned int *width_wrap,
+                   bool expanded)
 {
     /* check whether we need / can / are supposed to use pager */
     if (topt && topt->pager && isatty(fileno(stdin)) && isatty(fileno(stdout)))
@@ -3059,15 +3087,29 @@ PageOutput(int lines, const printTableOpt *topt)
 #ifdef TIOCGWINSZ
         unsigned short int pager = topt->pager;
         int            min_lines = topt->pager_min_lines;
-        int            result;
-        struct winsize screen_size;

-        result = ioctl(fileno(stdout), TIOCGWINSZ, &screen_size);
+        if (pager == 1)
+        {
+            int            result;
+            struct winsize screen_size;

-        /* >= accounts for a one-line prompt */
-        if (result == -1
-            || (lines >= screen_size.ws_row && lines >= min_lines)
-            || pager > 1)
+            result = ioctl(fileno(stdout), TIOCGWINSZ, &screen_size);
+            if (result < 0)
+                pager = 2;        /* force use of pager */
+            else
+            {
+                int            threshold = Max(screen_size.ws_row, min_lines);
+
+                if (cont)        /* caller wants us to calculate lines */
+                    lines = count_table_lines(cont, width_wrap, expanded,
+                                              threshold);
+                /* >= accounts for a one-line prompt */
+                if (lines >= threshold)
+                    pager = 2;
+            }
+        }
+
+        if (pager > 1)
 #endif
         {
             const char *pagerprog;
@@ -3379,11 +3421,7 @@ IsPagerNeeded(const printTableContent *cont, const unsigned int *width_wrap,
 {
     if (*fout == stdout)
     {
-        int            lines;
-
-        lines = count_table_lines(cont, width_wrap, expanded);
-
-        *fout = PageOutput(lines, cont->opt);
+        *fout = PageOutputInternal(0, cont->opt, cont, width_wrap, expanded);
         *is_pager = (*fout != stdout);
     }
     else
@@ -3396,13 +3434,18 @@ IsPagerNeeded(const printTableContent *cont, const unsigned int *width_wrap,
  * cont: table data to be printed
  * width_wrap[]: per-column maximum width, or NULL if caller will not wrap
  * expanded: expanded mode?
+ * threshold: we can stop counting once we pass threshold
  *
  * We currently handle only regular and expanded modes here.
+ *
+ * The value of the threshold parameter is that when the table is very
+ * long, we'll typically be able to stop scanning after not many rows.
  */
 static int
 count_table_lines(const printTableContent *cont,
                   const unsigned int *width_wrap,
-                  bool expanded)
+                  bool expanded,
+                  int threshold)
 {
     int           *header_height;
     int            lines,
@@ -3468,11 +3511,14 @@ count_table_lines(const printTableContent *cont,
                 lines += max_lines;
                 max_lines = 0;
             }
+            /* Stop scanning table body once we pass threshold */
+            if (lines > threshold)
+                break;
         }
     }

     /* Account for header and footer decoration */
-    if (!cont->opt->tuples_only)
+    if (!cont->opt->tuples_only && lines <= threshold)
     {
         if (cont->title)
         {
@@ -3500,6 +3546,9 @@ count_table_lines(const printTableContent *cont,
             pg_wcssize((const unsigned char *) f->data, strlen(f->data),
                        encoding, NULL, &nl_lines, NULL);
             lines += nl_lines;
+            /* Stop scanning footers once we pass threshold */
+            if (lines > threshold)
+                break;
         }
     }

--
2.43.7

From 8b7a5e79d98ebe7d7e31e200d66fb249816fc17e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 2 Oct 2025 16:54:27 -0400
Subject: [PATCH v4 3/3] Add a TAP test to exercise psql's use of PAGER.

Up to now, we have had zero test coverage of these code paths,
because they aren't reached unless isatty(stdout).  We do have
the test infrastructure to improve that situation, though.
Following the lead of 010_tab_completion.pl, set up an
interactive psql and feed it some cases that should make it
decide it needs to use the pager.

It's not at all clear how portable this is, though, so I
await the results of CI with interest.

Another issue is that as written, the test cases should cause
use of the pager, but the test will not reveal whether they
did or not.  (Code coverage testing shows that the right
lines of code are reached now, but that's not really a good
answer.)  Doing better seems to require use of something
besides "cat" as $PAGER, which adds more portability questions.
---
 src/bin/psql/meson.build    |  1 +
 src/bin/psql/t/030_pager.pl | 84 +++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 src/bin/psql/t/030_pager.pl

diff --git a/src/bin/psql/meson.build b/src/bin/psql/meson.build
index f795ff28271..d344053c23b 100644
--- a/src/bin/psql/meson.build
+++ b/src/bin/psql/meson.build
@@ -77,6 +77,7 @@ tests += {
       't/001_basic.pl',
       't/010_tab_completion.pl',
       't/020_cancel.pl',
+      't/030_pager.pl',
     ],
   },
 }
diff --git a/src/bin/psql/t/030_pager.pl b/src/bin/psql/t/030_pager.pl
new file mode 100644
index 00000000000..4bd6759a32a
--- /dev/null
+++ b/src/bin/psql/t/030_pager.pl
@@ -0,0 +1,84 @@
+
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+use Data::Dumper;
+
+# If we don't have IO::Pty, forget it, because IPC::Run depends on that
+# to support pty connections
+eval { require IO::Pty; };
+if ($@)
+{
+    plan skip_all => 'IO::Pty is needed to run this test';
+}
+
+# start a new server
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->start;
+
+# we don't really care about the pager's behavior, so just use cat
+$ENV{PAGER} = "cat";
+
+# fire up an interactive psql session
+my $h = $node->interactive_psql('postgres');
+
+# set the pty's window size to known values
+# (requires undesirable chumminess with the innards of IPC::Run)
+for my $pty (values %{ $h->{run}->{PTYS} })
+{
+    $pty->set_winsize(24, 80);
+}
+
+# Simple test case: type something and see if psql responds as expected
+sub do_command
+{
+    my ($send, $pattern, $annotation) = @_;
+
+    # report test failures from caller location
+    local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+    # restart per-command timer
+    $h->{timeout}->start($PostgreSQL::Test::Utils::timeout_default);
+
+    # send the data to be sent and wait for its result
+    my $out = $h->query_until($pattern, $send);
+    my $okay = ($out =~ $pattern && !$h->{timeout}->is_expired);
+    ok($okay, $annotation);
+    # for debugging, log actual output if it didn't match
+    local $Data::Dumper::Terse = 1;
+    local $Data::Dumper::Useqq = 1;
+    diag 'Actual output was ' . Dumper($out) . "Did not match \"$pattern\"\n"
+      if !$okay;
+    return;
+}
+
+# Test invocation of the pager
+# (Note that interactive_psql starts psql with --no-align --tuples-only)
+
+do_command(
+    "SELECT generate_series(1,10*10) as g;\n",
+    qr/100/,
+    "execute SELECT query that needs pagination");
+
+do_command(
+    "\\pset expanded\nSELECT generate_series(1,20) as g;\n",
+    qr/g\|20/,
+    "execute SELECT query that needs pagination in expanded mode");
+
+do_command(
+    "\\pset tuples_only off\n\\d+ information_schema.referential_constraints\n",
+    qr/has_any_column_privilege/,
+    "execute command with footer that needs pagination");
+
+# send psql an explicit \q to shut it down, else pty won't close properly
+$h->quit or die "psql returned $?";
+
+# done
+$node->stop;
+done_testing();
--
2.43.7