Horton

Thread

Commits

  1. Avoid edge case in pg_visibility test with small shared_buffers

  2. Report syncscan position at end of scan.

  3. hio: Take number of prior relation extensions into account

  4. Reduce code duplication between heapgettup and heapgettup_pagemode

  5. Fix CLUSTER tuplesorts on abbreviated expressions.

  6. Set PD_ALL_VISIBLE and visibility map bits in COPY FREEZE

  1. Sync scan & regression tests

    Konstantin Knizhnik <knizhnik@garret.ru> — 2023-08-06T19:20:57Z

    Hi hackers,

Is it is ok, that regression tests do not pass with small value of 
shared buffers (for example 1Mb)?

Two tests are failed because of sync scan - this tests cluster.sql and 
portals.sql perform seqscan without explicit order by and expect that 
data will be returned in particular order. But because of sync scan it 
doesn't happen. Small shared buffers are needed to satisfy seqscan 
criteria in heapam.c: `scan->rs_nblocks > NBuffers / 4` for tenk1 table.

More general question - is it really good that in situation when there 
is actually no concurrent queries, seqscan is started not from the first 
page?

  2. Re: Sync scan & regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-06T22:30:49Z

    Konstantin Knizhnik <knizhnik@garret.ru> writes:
> Is it is ok, that regression tests do not pass with small value of 
> shared buffers (for example 1Mb)?

There are quite a few GUC settings with which you can break the
regression tests.  I'm not especially bothered by this one.

> More general question - is it really good that in situation when there 
> is actually no concurrent queries, seqscan is started not from the first 
> page?

You're going to have race-condition issues if you try to make that
(not) happen, I should think.

			regards, tom lane

  3. Re: Sync scan & regression tests

    Thomas Munro <thomas.munro@gmail.com> — 2023-08-06T23:29:58Z

    On Mon, Aug 7, 2023 at 7:21 AM Konstantin Knizhnik <knizhnik@garret.ru> wrote:
> Two tests are failed because of sync scan - this tests cluster.sql and
> portals.sql perform seqscan without explicit order by and expect that
> data will be returned in particular order. But because of sync scan it
> doesn't happen. Small shared buffers are needed to satisfy seqscan
> criteria in heapam.c: `scan->rs_nblocks > NBuffers / 4` for tenk1 table.

I wondered the same thing while working on the tests in commit
8ab0ebb9a84, which explicitly care about physical order, so they *say
so* with ORDER BY ctid.  But the problem seems quite widespread, so I
didn't volunteer to try to do something like that everywhere, when Tom
committed cbf4177f for 027_stream_regress.pl.

FWIW here's another discussion of that cluster test, in which I was
still figuring out some surprising ways this feature can introduce
non-determinism even without concurrent access to the same table.

https://www.postgresql.org/message-id/flat/CA%2BhUKGLTK6ZuEkpeJ05-MEmvmgZveCh%2B_w013m7%2ByKWFSmRcDA%40mail.gmail.com

  4. Re: Sync scan & regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-08-07T00:55:36Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Mon, Aug 7, 2023 at 7:21 AM Konstantin Knizhnik <knizhnik@garret.ru> wrote:
>> Two tests are failed because of sync scan - this tests cluster.sql and
>> portals.sql perform seqscan without explicit order by and expect that
>> data will be returned in particular order. But because of sync scan it
>> doesn't happen. Small shared buffers are needed to satisfy seqscan
>> criteria in heapam.c: `scan->rs_nblocks > NBuffers / 4` for tenk1 table.

> I wondered the same thing while working on the tests in commit
> 8ab0ebb9a84, which explicitly care about physical order, so they *say
> so* with ORDER BY ctid.  But the problem seems quite widespread, so I
> didn't volunteer to try to do something like that everywhere, when Tom
> committed cbf4177f for 027_stream_regress.pl.

Our customary theory about that is explained in regress.sgml:

  You might wonder why we don't order all the regression test queries explicitly
  to get rid of this issue once and for all.  The reason is that that would
  make the regression tests less useful, not more, since they'd tend
  to exercise query plan types that produce ordered results to the
  exclusion of those that don't.

Having said that ... I just noticed that chipmunk, which I'd been
ignoring because it had been having configuration-related failures
ever since it came back to life about three months ago, has gotten
past those problems and is now failing with what looks suspiciously
like syncscan problems:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=chipmunk&dt=2023-08-06%2008%3A20%3A21

This is possibly explained by the fact that it uses (per its
extra_config)
                  'shared_buffers = 10MB',
although it's done that for a long time and portals.out hasn't changed
since before chipmunk's latest success.  Perhaps some change in an
earlier test script affected this?

I think it'd be worth running to ground exactly what is causing these
failures and when they started.  But assuming that they are triggered
by syncscan, my first thought about dealing with it is to disable
syncscan within the affected tests.  However ... do we exercise the
syncscan logic at all within the regression tests, normally?  Is there
a coverage issue to be dealt with?

			regards, tom lane

  5. Re: Sync scan & regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-08-29T10:35:44Z

    (noticed this thread just now)

On 07/08/2023 03:55, Tom Lane wrote:
> Having said that ... I just noticed that chipmunk, which I'd been
> ignoring because it had been having configuration-related failures
> ever since it came back to life about three months ago, has gotten
> past those problems

Yes, I finally got around to fix the configuration...

> and is now failing with what looks suspiciously like syncscan
> problems:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=chipmunk&dt=2023-08-06%2008%3A20%3A21
> 
> This is possibly explained by the fact that it uses (per its
> extra_config)
>                    'shared_buffers = 10MB',
> although it's done that for a long time and portals.out hasn't changed
> since before chipmunk's latest success.  Perhaps some change in an
> earlier test script affected this?

I changed the config yesterday to 'shared_buffers = 20MB', before seeing 
this thread. If we expect the regression tests to work with 
'shared_buffers=10MB' - and I think that would be nice - I'll change it 
back. But let's see what happens with 'shared_buffers=20MB'. It will 
take a few days for the tests to complete.

> I think it'd be worth running to ground exactly what is causing these
> failures and when they started.

I bisected it to this commit:

commit 7ae0ab0ad9704b10400a611a9af56c63304c27a5
Author: David Rowley <drowley@postgresql.org>
Date:   Fri Feb 3 16:20:43 2023 +1300

     Reduce code duplication between heapgettup and heapgettup_pagemode

     The code to get the next block number was exactly the same between 
these
     two functions, so let's just put it into a helper function and call 
that
     from both locations.

     Author: Melanie Plageman
     Reviewed-by: Andres Freund, David Rowley
     Discussion: 
https://postgr.es/m/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY=_T8QEqZqOL02rpi2bw@mail.gmail.com

 From that description, that was supposed to be just code refactoring, 
with no change in behavior.

Looking the new heapgettup_advance_block() function and the code that it 
replaced, it's now skipping this ss_report_location() on the last call, 
when it has reached the end of the scan:

> 
> 	/*
> 	 * Report our new scan position for synchronization purposes. We
> 	 * don't do that when moving backwards, however. That would just
> 	 * mess up any other forward-moving scanners.
> 	 *
> 	 * Note: we do this before checking for end of scan so that the
> 	 * final state of the position hint is back at the start of the
> 	 * rel.  That's not strictly necessary, but otherwise when you run
> 	 * the same query multiple times the starting position would shift
> 	 * a little bit backwards on every invocation, which is confusing.
> 	 * We don't guarantee any specific ordering in general, though.
> 	 */
> 	if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
> 		ss_report_location(scan->rs_base.rs_rd, block);

The comment explicitly says that we do that before checking for 
end-of-scan, but that commit moved it to after. I propose the attached 
to move it back to before the end-of-scan checks.

Melanie, David, any thoughts?

> But assuming that they are triggered by syncscan, my first thought
> about dealing with it is to disable syncscan within the affected
> tests.  However ... do we exercise the syncscan logic at all within
> the regression tests, normally?  Is there a coverage issue to be
> dealt with?
Good question..

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  6. Re: Sync scan & regression tests

    David Rowley <dgrowleyml@gmail.com> — 2023-08-30T21:15:03Z

    On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Looking the new heapgettup_advance_block() function and the code that it
> replaced, it's now skipping this ss_report_location() on the last call,
> when it has reached the end of the scan:
>
> >
> >       /*
> >        * Report our new scan position for synchronization purposes. We
> >        * don't do that when moving backwards, however. That would just
> >        * mess up any other forward-moving scanners.
> >        *
> >        * Note: we do this before checking for end of scan so that the
> >        * final state of the position hint is back at the start of the
> >        * rel.  That's not strictly necessary, but otherwise when you run
> >        * the same query multiple times the starting position would shift
> >        * a little bit backwards on every invocation, which is confusing.
> >        * We don't guarantee any specific ordering in general, though.
> >        */
> >       if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
> >               ss_report_location(scan->rs_base.rs_rd, block);
>
> The comment explicitly says that we do that before checking for
> end-of-scan, but that commit moved it to after. I propose the attached
> to move it back to before the end-of-scan checks.
>
> Melanie, David, any thoughts?

I just looked at v15's code and I agree that the ss_report_location()
would be called even when the scan is finished.  It wasn't intentional
that that was changed in v16, so I'm happy for your patch to be
applied and backpatched to 16.  Thanks for digging into that.

David

  7. Re: Sync scan & regression tests

    Melanie Plageman <melanieplageman@gmail.com> — 2023-08-30T23:37:18Z

    On Wed, Aug 30, 2023 at 5:15 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > Looking the new heapgettup_advance_block() function and the code that it
> > replaced, it's now skipping this ss_report_location() on the last call,
> > when it has reached the end of the scan:
> >
> > >
> > >       /*
> > >        * Report our new scan position for synchronization purposes. We
> > >        * don't do that when moving backwards, however. That would just
> > >        * mess up any other forward-moving scanners.
> > >        *
> > >        * Note: we do this before checking for end of scan so that the
> > >        * final state of the position hint is back at the start of the
> > >        * rel.  That's not strictly necessary, but otherwise when you run
> > >        * the same query multiple times the starting position would shift
> > >        * a little bit backwards on every invocation, which is confusing.
> > >        * We don't guarantee any specific ordering in general, though.
> > >        */
> > >       if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
> > >               ss_report_location(scan->rs_base.rs_rd, block);
> >
> > The comment explicitly says that we do that before checking for
> > end-of-scan, but that commit moved it to after. I propose the attached
> > to move it back to before the end-of-scan checks.
> >
> > Melanie, David, any thoughts?
>
> I just looked at v15's code and I agree that the ss_report_location()
> would be called even when the scan is finished.  It wasn't intentional
> that that was changed in v16, so I'm happy for your patch to be
> applied and backpatched to 16.  Thanks for digging into that.

Yes, thanks for catching my mistake.
LGTM.

  8. Re: Sync scan & regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-08-31T09:45:34Z

    On 29/08/2023 13:35, Heikki Linnakangas wrote:
> On 07/08/2023 03:55, Tom Lane wrote:
>> This is possibly explained by the fact that it uses (per its
>> extra_config)
>>                     'shared_buffers = 10MB',
>> although it's done that for a long time and portals.out hasn't changed
>> since before chipmunk's latest success.  Perhaps some change in an
>> earlier test script affected this?
> 
> I changed the config yesterday to 'shared_buffers = 20MB', before seeing
> this thread. If we expect the regression tests to work with
> 'shared_buffers=10MB' - and I think that would be nice - I'll change it
> back. But let's see what happens with 'shared_buffers=20MB'. It will
> take a few days for the tests to complete.

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.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  9. Re: Sync scan & regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-08-31T10:12:42Z

    On 31/08/2023 02:37, Melanie Plageman wrote:
> On Wed, Aug 30, 2023 at 5:15 PM David Rowley <dgrowleyml@gmail.com> wrote:
>>
>> I just looked at v15's code and I agree that the ss_report_location()
>> would be called even when the scan is finished.  It wasn't intentional
>> that that was changed in v16, so I'm happy for your patch to be
>> applied and backpatched to 16.  Thanks for digging into that.
> 
> Yes, thanks for catching my mistake.
> LGTM.

Pushed, thanks for the reviews!

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  10. Re: Sync scan & regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-05T03:16:48Z

    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?

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=chipmunk&dt=2023-09-01%2015%3A15%3A56

  11. Re: Sync scan & regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-09-18T10:49:24Z

    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


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";

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.

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.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  12. Re: Sync scan & regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-09-18T13:45:19Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 05/09/2023 06:16, Tom Lane wrote:
>> So chipmunk is getting through the core tests now, but instead it
>> is failing in contrib/pg_visibility [1]:

> 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

Yeah, I came to the same conclusion after discovering that I could
reproduce it locally with small shared_buffers.

> 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:
> ...
> 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.

I think your alternative (c) is plenty attractive.  IIUC, the current
change has at one stroke doubled the amount of disk space eaten by
a table that formerly needed two pages.  I don't think we should be
adding more than one page at a time until the table size reaches
perhaps 10 pages.

			regards, tom lane

  13. Re: Sync scan & regression tests

    Andres Freund <andres@anarazel.de> — 2023-09-18T22:57:03Z

    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

  14. Re: Sync scan & regression tests

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-09-19T09:06:57Z

    On 19/09/2023 01:57, Andres Freund wrote:
> On 2023-09-18 13:49:24 +0300, Heikki Linnakangas wrote:
>> 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?

Sorry, I said "c) seems like a simple fix ...", but I meant "d) seems 
like a simple fix ..."

I meant the attached.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  15. Re: Sync scan & regression tests

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-24T15:28:12Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 19/09/2023 01:57, Andres Freund wrote:
>> On 2023-09-18 13:49:24 +0300, Heikki Linnakangas wrote:
>>> 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?

> Sorry, I said "c) seems like a simple fix ...", but I meant "d) seems 
> like a simple fix ..."
> I meant the attached.

This thread stalled out months ago, but chipmunk is still failing in
HEAD and v16.  Can we please have a fix?  I'm good with Heikki's
adjustment to the pg_visibility test case.

			regards, tom lane

  16. Re: Sync scan & regression tests

    Andres Freund <andres@anarazel.de> — 2024-03-26T03:28:34Z

    Hi,

On 2024-03-24 11:28:12 -0400, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
> > On 19/09/2023 01:57, Andres Freund wrote:
> >> On 2023-09-18 13:49:24 +0300, Heikki Linnakangas wrote:
> >>> 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?
>
> > Sorry, I said "c) seems like a simple fix ...", but I meant "d) seems
> > like a simple fix ..."
> > I meant the attached.
>
> This thread stalled out months ago, but chipmunk is still failing in
> HEAD and v16.  Can we please have a fix?  I'm good with Heikki's
> adjustment to the pg_visibility test case.

I pushed Heikki's adjustment. Thanks for the "fix" and the reminder.

Greetings,

Andres Freund