Обсуждение: One-off failure in "cluster" test

Hi,

I wonder what caused this[1] one-off failure to see tuples in clustered order:

diff -U3 /home/pgbfarm/buildroot/REL_13_STABLE/pgsql.build/src/test/regress/expected/cluster.out
/home/pgbfarm/buildroot/REL_13_STABLE/pgsql.build/src/test/regress/results/cluster.out
--- /home/pgbfarm/buildroot/REL_13_STABLE/pgsql.build/src/test/regress/expected/cluster.out
2020-06-11 07:58:23.738084255 +0300
+++ /home/pgbfarm/buildroot/REL_13_STABLE/pgsql.build/src/test/regress/results/cluster.out
2020-07-05 02:35:06.396023210 +0300
@@ -462,7 +462,8 @@
 where row(hundred, thousand, tenthous) <= row(lhundred, lthousand, ltenthous);
  hundred | lhundred | thousand | lthousand | tenthous | ltenthous
 ---------+----------+----------+-----------+----------+-----------
-(0 rows)
+       0 |       99 |        0 |       999 |        0 |      9999
+(1 row)

I guess a synchronised scan could cause that, but I wouldn't expect one here.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=chipmunk&dt=2020-07-04%2023:10:22

Thomas Munro <thomas.munro@gmail.com> writes:
> I wonder what caused this[1] one-off failure to see tuples in clustered order:
> ...
> I guess a synchronised scan could cause that, but I wouldn't expect one here.

Looking at its configuration, chipmunk uses

 'extra_config' => {
 ...
                                                      'shared_buffers = 10MB',

which I think means that clstr_4 would be large enough to trigger a
syncscan.  Ordinarily that's not a problem since no other session would
be touching clstr_4 ... but I wonder whether (a) autovacuum had decided
to look at clstr_4 and (b) syncscan can trigger on vacuum-driven scans.
(a) would explain the non-reproducibility.

I kinda think that (b), if true, is a bad idea and should be suppressed.
autovacuum would typically fail to keep up with other syncscans thanks
to vacuum delay settings, so letting it participate seems unhelpful.

            regards, tom lane

On Mon, Aug 17, 2020 at 1:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > I wonder what caused this[1] one-off failure to see tuples in clustered order:
> > ...
> > I guess a synchronised scan could cause that, but I wouldn't expect one here.
>
> Looking at its configuration, chipmunk uses
>
>  'extra_config' => {
>  ...
>                                                       'shared_buffers = 10MB',
>
> which I think means that clstr_4 would be large enough to trigger a
> syncscan.  Ordinarily that's not a problem since no other session would
> be touching clstr_4 ... but I wonder whether (a) autovacuum had decided
> to look at clstr_4 and (b) syncscan can trigger on vacuum-driven scans.
> (a) would explain the non-reproducibility.
>
> I kinda think that (b), if true, is a bad idea and should be suppressed.
> autovacuum would typically fail to keep up with other syncscans thanks
> to vacuum delay settings, so letting it participate seems unhelpful.

Yeah, I wondered that as well and found my way to historical
discussions concluding that autovacuum should not participate in sync
scans.  Now I'm wondering if either table AM refactoring or parallel
vacuum refactoring might have inadvertently caused that to become a
possibility in REL_13_STABLE.

On Mon, Aug 17, 2020 at 1:27 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Mon, Aug 17, 2020 at 1:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Thomas Munro <thomas.munro@gmail.com> writes:
> > > I wonder what caused this[1] one-off failure to see tuples in clustered order:
> > > ...
> > > I guess a synchronised scan could cause that, but I wouldn't expect one here.
> >
> > Looking at its configuration, chipmunk uses
> >
> >  'extra_config' => {
> >  ...
> >                                                       'shared_buffers = 10MB',

Ahh, I see what's happening.  You don't need a concurrent process
scanning *your* table for scan order to be nondeterministic.  The
preceding CLUSTER command can leave the start block anywhere if its
call to ss_report_location() fails to acquire SyncScanLock
conditionally.  So I think we just need to disable that for this test,
like in the attached.

Thomas Munro <thomas.munro@gmail.com> writes:
> Ahh, I see what's happening.  You don't need a concurrent process
> scanning *your* table for scan order to be nondeterministic.  The
> preceding CLUSTER command can leave the start block anywhere if its
> call to ss_report_location() fails to acquire SyncScanLock
> conditionally.  So I think we just need to disable that for this test,
> like in the attached.

Hmm.  I'm not terribly thrilled about band-aiding one unstable test
case at a time.

heapgettup makes a point of ensuring that its scan end position
gets reported:

            page++;
            if (page >= scan->rs_nblocks)
                page = 0;
            finished = (page == scan->rs_startblock) ||
                (scan->rs_numblocks != InvalidBlockNumber ? --scan->rs_numblocks == 0 : false);

            /*
             * 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, page);

Seems like the conditional LWLockAcquire is pissing away that attempt
at stability.  Maybe we should adjust things so that the final
location report isn't done conditionally.

            regards, tom lane