On Wed, 25 Jan 2023 at 13:55, Melanie Plageman
<melanieplageman@gmail.com> wrote:
> David Rowley and I were discussing how to test the
> NoMovementScanDirection case for heapgettup() and heapgettup_pagemode()
> in [1] (since there is not currently coverage). We are actually
> wondering if it is dead code (in core).
Yeah, so I see nothing in core that can cause heapgettup() or
heapgettup_pagemode() to be called with NoMovementScanDirection. I
imagine one possible way to hit it might be in an extension where
someone's written their own ExecutorRun_hook that does not have the
same NoMovementScanDirection check that standard_ExecutorRun() has.
So far my thoughts are that we should just rip it out and see if
anyone complains. If they complain loudly enough, then it's easy
enough to put it back without any compatibility issues. However, if
it's for the ExecutorRun_hook reason, then they'll likely be better to
add the same NoMovementScanDirection as we have in
standard_ExecutorRun().
I'm just not keen on refactoring the code without the means to test
that the new code actually works.
Does anyone know of any reason why we shouldn't ditch the nomovement
code in heapgettup/heapgettup_pagemode?
Maybe we'd also want to Assert that the direction is either forwards
or backwards in table_scan_getnextslot and
table_scan_getnextslot_tidrange. (I see heap_getnextslot_tidrange()
does not have any handling for NoMovementScanDirection, so if this is
not dead code, that probably needs to be fixed)
David