Re: Sync scan & regression tests

(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)