Re: heapgettup() with NoMovementScanDirection unused in core?

On Tue, 31 Jan 2023 at 09:57, Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
> 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.

My thoughts were that we might want to put them
table_scan_getnextslot() and table_scan_getnextslot_tidrange(). My
rationale for that was that it makes it more clear to table AM devs
that they don't need to handle NoMovementScanDirection.

> 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()?

Yeah, this needs to be handled. FETCH can fetch forwards or backwards
from a cursor. The code you have looks fine to me.

> 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.

That's probably slightly better.

The only thing I really have on this is my thoughts on the Asserts
going in tableam.h plus the following comment:

/*
* These enum values were originally int8 values. Using -1, 0, and 1 as their
* values conveniently mirrors their semantic value when used during execution.
*/

I don't really see any reason to keep the historical note here. I
think something like the following might be better:

/*
* Defines the direction for scanning a table or an index. Scans are never
* invoked using NoMovementScanDirectionScans. For convenience, we use the
* values -1 and 1 for backward and forward scans. This allows us to perform
* a few mathematical tricks such as what is done in ScanDirectionCombine.
*/

Also, a nitpick around the inconsistency with the Asserts. In
make_indexscan() and make_indexonlyscan() you're checking you're
getting a forward and backward value, but in heapgettup() and
heapgettup_pagemode() you're checking you don't get
NoMovementScanDirection. I think the != NoMovementScanDirection is
fine for both cases.

Both can be easily fixed, so no need to submit another patch as far as
I'm concerned.

I'll leave this til tomorrow in case Tom wants to have another look too.

David