Re: heapgettup() with NoMovementScanDirection unused in core?

v2 attached

On Fri, Jan 27, 2023 at 6:28 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Sat, 28 Jan 2023 at 12:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > /*
> > * Determine the net effect of two direction specifications.
> > * This relies on having ForwardScanDirection = +1, BackwardScanDirection = -1,
> > * and will probably not do what you want if applied to any other values.
> > */
> > #define CombineScanDirections(a, b) ((a) * (b))
> >
> > The main thing this'd buy us is being able to grep for uses of the
> > trick. If it's written as just multiplication, good luck being
> > able to find what's depending on that, should you ever need to.
>
> Yeah, I think the multiplication macro is a good way of doing it.
> Having the definition of it close to the ScanDirection enum's
> definition is likely a very good idea so that anyone adjusting the
> enum values is more likely to notice that it'll cause an issue. A
> small note on the enum declaration about the -1, +1 values being
> exploited in various places might be a good idea too. I see v6-0006 in
> [1] further exploits this, so that's further reason to document that.
>
> My personal preference would have been to call it
> ScanDirectionCombine, so the naming is more aligned to the 4 other
> macro names that start with ScanDirection in sdir.h, but I'm not going
> to fuss over it.

I've gone with this macro name.
I've also updated comments Tom mentioned and removed the test.

As for the asserts, I was at a bit of a loss as to where to put an
assert which will make it clear that heapgettup() and
heapgettup_pagemode() do not handle NoMovementScanDirection but was
at a higher level of the executor. Do we not have to accommodate the
direction changing from tuple to tuple? If we don't expect the plan node
direction to change during execution, then why recalculate
estate->es_direction for each invocation of Index/SeqNext()?

As such, in this version I've put the asserts in heapgettup() and
heapgettup_pagemode().

I also realized that it doesn't really make sense to assert about the
index scan direction in ExecInitIndexOnlyScan() and ExecInitIndexScan()
-- so I've moved the assertion to planner when we make the index plan
from the path. I'm not sure if it is needed.

- Melanie