Re: Sync scan & regression tests

Hi,

On 2023-09-18 13:49:24 +0300, Heikki Linnakangas wrote:
> On 05/09/2023 06:16, Tom Lane wrote:
> > Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > > With shared_buffers='20MB', the tests passed. I'm going to change it
> > > back to 10MB now, so that we continue to cover that case.
> > 
> > So chipmunk is getting through the core tests now, but instead it
> > is failing in contrib/pg_visibility [1]:
> > 
> > diff -U3 /home/pgbfarm/buildroot/HEAD/pgsql.build/contrib/pg_visibility/expected/pg_visibility.out /home/pgbfarm/buildroot/HEAD/pgsql.build/contrib/pg_visibility/results/pg_visibility.out
> > --- /home/pgbfarm/buildroot/HEAD/pgsql.build/contrib/pg_visibility/expected/pg_visibility.out	2022-10-08 19:00:15.905074105 +0300
> > +++ /home/pgbfarm/buildroot/HEAD/pgsql.build/contrib/pg_visibility/results/pg_visibility.out	2023-09-02 00:25:51.814148116 +0300
> > @@ -218,7 +218,8 @@
> >        0 | t           | t
> >        1 | t           | t
> >        2 | t           | t
> > -(3 rows)
> > +     3 | f           | f
> > +(4 rows)
> >   select * from pg_check_frozen('copyfreeze');
> >    t_ctid
> > 
> > I find this easily reproducible by setting shared_buffers=10MB.
> > But I'm confused about why, because the affected test case
> > dates to Tomas' commit 7db0cd214 of 2021-01-17, and chipmunk
> > passed many times after that.  Might be worth bisecting in
> > the interval where chipmunk wasn't reporting?
> 
> I bisected it to this:
> 
> commit 82a4edabd272f70d044faec8cf7fd1eab92d9991 (HEAD)
> Author: Andres Freund <andres@anarazel.de>
> Date:   Mon Aug 14 09:54:03 2023 -0700
> 
>     hio: Take number of prior relation extensions into account
> 
>     The new relation extension logic, introduced in 00d1e02be24, could lead
> to
>     slowdowns in some scenarios. E.g., when loading narrow rows into a table
> using
>     COPY, the caller of RelationGetBufferForTuple() will only request a
> small
>     number of pages. Without concurrency, we just extended using pwritev()
> in that
>     case. However, if there is *some* concurrency, we switched between
> extending
>     by a small number of pages and a larger number of pages, depending on
> the
>     number of waiters for the relation extension logic.  However, some
>     filesystems, XFS in particular, do not perform well when switching
> between
>     extending files using fallocate() and pwritev().
> 
>     To avoid that issue, remember the number of prior relation extensions in
>     BulkInsertState and extend more aggressively if there were prior
> relation
>     extensions. That not just avoids the aforementioned slowdown, but also
> leads
>     to noticeable performance gains in other situations, primarily due to
>     extending more aggressively when there is no concurrency. I should have
> done
>     it this way from the get go.
> 
>     Reported-by: Masahiko Sawada <sawada.mshk@gmail.com>
>     Author: Andres Freund <andres@anarazel.de>
>     Discussion: https://postgr.es/m/CAD21AoDvDmUQeJtZrau1ovnT_smN940=Kp6mszNGK3bq9yRN6g@mail.gmail.com
>     Backpatch: 16-, where the new relation extension code was added

This issue is also discussed at:

https://www.postgresql.org/message-id/20230916000011.2ugpkkkp7bpp4cfh%40awork3.anarazel.de


> Before this patch, the test table was 3 pages long, now it is 4 pages with a
> small shared_buffers setting.
> 
> In this test, the relation needs to be at least 3 pages long to hold all the
> COPYed rows. With a larger shared_buffers, the table is extended to three
> pages in a single call to heap_multi_insert(). With shared_buffers='10 MB',
> the table is extended twice, because LimitAdditionalPins() restricts how
> many pages are extended in one go to two pages. With the logic that that
> commit added, we first extend the table with 2 pages, then with 2 pages
> again.
> 
> I think the behavior is fine. The reasons given in the commit message make
> sense. But it would be nice to silence the test failure. Some alternatives:
> 
> a) Add an alternative expected output file
> 
> b) Change the pg_visibilitymap query so that it passes even if the table has
> four pages. "select * from pg_visibility_map('copyfreeze') where blkno <=
> 3";

I think the test already encodes the tuple and page size too much, this'd go
further down that road.

It's too bad we don't have an easy way for testing if a page is empty - if we
did, it'd be simple to write this in a robust way. Instead of the query I came
up with in the other thread:
select *
from pg_visibility_map('copyfreeze')
where
  (not all_visible or not all_frozen)
  -- deal with trailing empty pages due to potentially bulk-extending too aggressively
  and exists(SELECT * FROM copyfreeze WHERE ctid >= ('('||blkno||', 0)')::tid)
;


> c) Change the extension logic so that we don't extend so much when the table
> is small. The efficiency of bulk extension doesn't matter when the table is
> tiny, so arguably we should rather try to minimize the table size. If you
> have millions of tiny tables, allocating one extra block on each adds up.

We could also adjust LimitAdditionalPins() to be a bit more aggressive, by
actually counting instead of using REFCOUNT_ARRAY_ENTRIES (only when it might
matter, for efficiency reasons).



> d) Copy fewer rows to the table in the test. If we copy only 6 rows, for
> example, the table will have only two pages, regardless of shared_buffers.
> 
> I'm leaning towards d). The whole test is a little fragile, it will also
> fail with a non-default block size, for example. But c) seems like a simple
> fix and wouldn't look too out of place in the test.

Hm, what do you mean with the last sentence? Oh, is the test you're
referencing the relation-extension logic?

Greetings,

Andres Freund