Re: Sync scan & regression tests

On Wed, Aug 30, 2023 at 5:15 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> 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.

Yes, thanks for catching my mistake.
LGTM.