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
