(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 <drow...@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=_t8qeqzqol02rpi...@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)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3bb1d5cff68..48b4ca45439 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -647,17 +647,6 @@ heapgettup_advance_block(HeapScanDesc scan, BlockNumber block, ScanDirection dir
if (block >= scan->rs_nblocks)
block = 0;
- /* we're done if we're back at where we started */
- if (block == scan->rs_startblock)
- return InvalidBlockNumber;
-
- /* check if the limit imposed by heap_setscanlimits() is met */
- if (scan->rs_numblocks != InvalidBlockNumber)
- {
- if (--scan->rs_numblocks == 0)
- return InvalidBlockNumber;
- }
-
/*
* Report our new scan position for synchronization purposes. We
* don't do that when moving backwards, however. That would just
@@ -673,6 +662,17 @@ heapgettup_advance_block(HeapScanDesc scan, BlockNumber block, ScanDirection dir
if (scan->rs_base.rs_flags & SO_ALLOW_SYNC)
ss_report_location(scan->rs_base.rs_rd, block);
+ /* we're done if we're back at where we started */
+ if (block == scan->rs_startblock)
+ return InvalidBlockNumber;
+
+ /* check if the limit imposed by heap_setscanlimits() is met */
+ if (scan->rs_numblocks != InvalidBlockNumber)
+ {
+ if (--scan->rs_numblocks == 0)
+ return InvalidBlockNumber;
+ }
+
return block;
}
else
