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

Melanie Plageman <melanieplageman@gmail.com> writes:
> I have written the patch to remove the unreachable code in
> heapgettup_pagemode]().

A few thoughts ...

1. Do we really need quite so many Asserts?  I'd kind of lean
to just having one, at some higher level of the executor.

2. I'm not sure if we want to do this:

-    direction = estate->es_direction;
-    /* flip direction if this is an overall backward scan */
-    if (ScanDirectionIsBackward(((IndexScan *) node->ss.ps.plan)->indexorderdir))
-    {
-        if (ScanDirectionIsForward(direction))
-            direction = BackwardScanDirection;
-        else if (ScanDirectionIsBackward(direction))
-            direction = ForwardScanDirection;
-    }
+    direction = estate->es_direction * ((IndexScan *) node->ss.ps.plan)->indexorderdir;

AFAIR, there is noplace today that depends on the exact encoding
of ForwardScanDirection and BackwardScanDirection, and I'm not
sure that we want to introduce such a dependency.  If we do it
at least deserves a comment here, and you probably ought to adjust
the wishy-washy comment in sdir.h as well.  Taking out the existing
comment explaining what this code is doing is not an improvement
either.

3. You didn't update the header comment for heapgettup, nor the
one in pathnodes.h for IndexPath.indexscandir.

4. I don't think the proposed test case is worth the cycles.

            regards, tom lane