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 <dgrowle...@gmail.com> wrote: > > > > On Thu, 19 Jan 2023 at 00:04, Peter Eisentraut > > <peter.eisentr...@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