Re: BitmapHeapScan streaming read user and prelim refactoring

On 5/14/24 19:42, Melanie Plageman wrote:
> On Tue, May 14, 2024 at 2:18 AM Michael Paquier <michael(at)paquier(dot)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