On 5/14/24 19:42, Melanie Plageman wrote: > On Tue, May 14, 2024 at 2:18 AM Michael Paquier <mich...@paquier.xyz> wrote: >> >> On Mon, May 13, 2024 at 10:05:03AM -0400, Melanie Plageman wrote: >>> Remove the assert and reset the field on which it previously asserted to >>> avoid incorrectly emitting NULL-filled tuples from a previous scan on >>> rescan. >> >>> - Assert(scan->rs_empty_tuples_pending == 0); >>> + scan->rs_empty_tuples_pending = 0; >> >> Perhaps this should document the reason why the reset is done in these >> two paths rather than let the reader guess it? And this is about >> avoiding emitting some tuples from a previous scan. > > I've added a comment to heap_rescan() in the attached v5. Doing so > made me realize that we shouldn't bother resetting it in > heap_endscan(). Doing so is perhaps more confusing, because it implies > that field may somehow be used later. I've removed the reset of > rs_empty_tuples_pending from heap_endscan(). >
+1 >>> +SET enable_indexonlyscan = off; >>> +set enable_indexscan = off; >>> +SET enable_seqscan = off; >> >> Nit: adjusting the casing of the second SET here. > > I've fixed this. I've also set enable_material off as I mentioned I > might in my earlier mail. > I'm not sure this (setting more and more GUCs to prevent hypothetical plan changes) is a good practice. Because how do you know the plan does not change for some other unexpected reason, possibly in the future? IMHO if the test requires a specific plan, it's better to do an actual "explain (rows off, costs off)" to check that. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
