Re: heapgettup() with NoMovementScanDirection unused in core?

Hi,

On 2023-01-25 10:02:28 -0500, Tom Lane wrote:
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > Does anyone know of any reason why we shouldn't ditch the nomovement
> > code in heapgettup/heapgettup_pagemode?

+1

Because I dug it up yesterday. There used to be callers of heap* with
NoMovement. But they were unused themselves:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=adbfab119b308a7e0e6b1305de9be222cfd5c85b

> * If direction is NoMovementScanDirection then nothing is done
> * except to start up/shut down the destination. Otherwise,
> * we retrieve up to 'count' tuples in the specified direction.
> *
> * Note: count = 0 is interpreted as no portal limit, i.e., run to
> * completion.
>
> We must have the NoMovementScanDirection option because count = 0
> does not mean "do nothing", and I noted at least two call sites
> that require it.

I wonder if we'd be better off removing NoMovementScanDirection, and using
count == (uint64)-1 for what NoMovementScanDirection is currently used for as
an ExecutorRun parameter. Seems less confusing to me - right now we have two
parameters with non-obbvious meanings and interactions.

> I wonder if we couldn't also get rid of this confusingly-inconsistent
> alternative usage in the planner:
>
> * 'indexscandir' is one of:
> * ForwardScanDirection: forward scan of an ordered index
> * BackwardScanDirection: backward scan of an ordered index
> * NoMovementScanDirection: scan of an unordered index, or don't care
> * (The executor doesn't care whether it gets ForwardScanDirection or
> * NoMovementScanDirection for an indexscan, but the planner wants to
> * distinguish ordered from unordered indexes for building pathkeys.)

+1

Certainly seems confusing to me.

Greetings,

Andres Freund