Re: heapgettup refactoring

Thanks for taking a look!

On Mon, Jan 23, 2023 at 6:08 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> > In your v2 patch, you remove these assertions:
> >
> > - /* check that rs_cindex is in sync */
> > - Assert(scan->rs_cindex < scan->rs_ntuples);
> > - Assert(lineoff == scan->rs_vistuples[scan->rs_cindex]);
> >
> > Is that intentional?
> >
> > I don't see any explanation, or some other equivalent code appearing
> > elsewhere to replace this.
>
> I guess it's because those asserts are not relevant unless
> heapgettup_no_movement() is being called from heapgettup_pagemode().
> Maybe they can be put back along the lines of:
>
> Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 ||
> scan->rs_cindex < scan->rs_ntuples);
> Assert((scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) == 0 || lineoff ==
> scan->rs_vistuples[scan->rs_cindex]);
>
> but it probably would be cleaner to just do an: if
> (scan->rs_base.rs_flags & SO_ALLOW_PAGEMODE) { Assert(...);
> Assert(...}; }

I prefer the first method and have implemented that in attached v6.

> The only issue I see with that is that we don't seem to have anywhere
> in the regression tests that call heapgettup_no_movement() when
> rs_flags have SO_ALLOW_PAGEMODE. At least, adding an elog(NOTICE) to
> heapgettup() just before calling heapgettup_no_movement() does not
> seem to cause make check to fail. I wonder if any series of SQL
> commands would allow us to call heapgettup_no_movement() from
> heapgettup()?

So, the places in which we set scan direction to no movement include:
- explain analyze on a ctas with no data
EXPLAIN ANALYZE CREATE TABLE foo AS SELECT 1 WITH NO DATA;
However, in standard_ExecutorRun() we only call ExecutePlan() if the
ScanDirection is not no movement, so this wouldn't hit our code
- PortalRunSelect
- PersistHoldablePortal()

I can't say I know enough about portals currently to design a test that
will hit this code, but I will poke around some more.

> I think heapgettup_no_movement() also needs a header comment more
> along the lines of:
>
> /*
> * heapgettup_no_movement
> * Helper function for NoMovementScanDirection direction for
> heapgettup() and
> * heapgettup_pagemode.
> */

I've added a comment but I didn't include the function name in it -- I
find it repetitive when the comments above functions do that -- however,
I'm not strongly attached to that.

> I pushed the pgindent stuff that v5-0001 did along with some additions
> to typedefs.list so that further runs could be done more easily as
> changes are made to these patches.

Cool!

- Melanie