Re: heapgettup() with NoMovementScanDirection unused in core? - Mailing list pgsql-hackers

On Tue, Jan 31, 2023 at 11:46:05PM +1300, David Rowley wrote:
> On Tue, 31 Jan 2023 at 09:57, Melanie Plageman
> <melanieplageman@gmail.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.

I previously had the asserts here, but I thought perhaps we shouldn't
restrict table AMs from using NoMovementScanDirection in whatever way
they'd like. We care about protecting heapgettup() and
heapgettup_pagemode(). We could put a comment in the table AM API about
NoMovementScanDirection not necessarily making sense for a next() type
function and informing table AMs that they need not support it.

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

This comment looks good to me.

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

Yes, I thought about it being weird that they are different. Perhaps we
should check in both places that it is forward or backward. In
heapgettup[_pagemode()] there is if/else -- so if the assert is only for
NoMovementScanDirection and a new scan direction is added, it would fall
through to the else.

In planner, it is not that we are not "handling" NoMovementScanDirection
(like in heapgettup) but rather that we are only passing Forward and
Backward scan directions when creating the path nodes, so the Assert
would be mainly to remind the developer that if they are creating a plan
with a different scan direction that they should be intentional about
it.

So, I would favor having both asserts check that the direction is one of
forward or backward.
 
> Both can be easily fixed, so no need to submit another patch as far as
> I'm concerned.

I realized I forgot a commit message in the second version. Patch v1 has
one.

- Melanie