Re: heapgettup refactoring

On Tue, Jan 24, 2023 at 04:17:23PM -0500, Melanie Plageman wrote:
> Thanks for taking a look!
> 
> On Mon, Jan 23, 2023 at 6:08 AM David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut
> > <peter.eisentraut@enterprisedb.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 don't think we can write a test for this afterall. I've started
another thread on the topic over here:

https://www.postgresql.org/message-id/CAAKRu_bvkhka0CZQun28KTqhuUh5ZqY%3D_T8QEqZqOL02rpi2bw%40mail.gmail.com