Re: heapgettup() with NoMovementScanDirection unused in core?

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?

AFAICS, the remaining actual use-case for NoMovementScanDirection
is that defined by ExecutorRun:

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

The heapgettup definition is thus not only unreachable, but confusingly
inconsistent with this meaning.

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

While that comment's claim is plausible, I think it's been wrong for
years. AFAICS indxpath.c makes pathkeys before it ever does this:

index_is_ordered ?
ForwardScanDirection :
NoMovementScanDirection,

and nothing depends on that later either. So I think we could
simplify this to something like "indexscandir is either
ForwardScanDirection or BackwardScanDirection. (Unordered
index types need not support BackwardScanDirection.)"

regards, tom lane