Thread

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

Erik Wienhold <ewie@ewie.name> — 2025-10-02T16:53:18Z

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