Re: Sync scan & regression tests

On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas <hlinnaka(at)iki(dot)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