Re: Sync scan & regression tests

On Wed, Aug 30, 2023 at 5:15 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 29 Aug 2023 at 22:35, Heikki Linnakangas <hlinnaka@iki.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.