Hi, On 2023-09-18 13:49:24 +0300, Heikki Linnakangas wrote: > On 05/09/2023 06:16, Tom Lane wrote: > > Heikki Linnakangas <hlinn...@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 <and...@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.m...@gmail.com> > Author: Andres Freund <and...@anarazel.de> > Discussion: > https://postgr.es/m/CAD21AoDvDmUQeJtZrau1ovnT_smN940=kp6mszngk3bq9yr...@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