Re: heapgettup() with NoMovementScanDirection unused in core?

On Fri, Jan 27, 2023 at 05:05:16PM -0500, Tom Lane wrote:
> Melanie Plageman <melanieplageman(at)gmail(dot)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.

Yes, perhaps I was a bit overzealous putting them in functions called
for every tuple.

I'm not sure where in the executor would make the most sense.
ExecInitSeqScan() comes to mind, but I'm not sure that covers all of the
desired cases.

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

I think you mean the enum value when you say encoding? I actually
started using the ScanDirection value in the refactor of heapgettup()
and heapgettup_pagemode() which I proposed here [1]. Why wouldn't we
want to introduce such a dependency?

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

Oops -- thanks for catching those!

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

Just the one I wrote or any test case?

- Melanie

[1] https://www.postgresql.org/message-id/flat/CAAKRu_ZJg_N7zHtWP%2BJoSY_hrce4%2BGKioL137Y2c2En-kuXQ7g%40mail.gmail.com#8a106c6625bc069cf439230cd9fa1000