David Rowley <dgrowle...@gmail.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