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