public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tomas Vondra <[email protected]>
To: Melanie Plageman <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: Nazir Bilal Yavuz <[email protected]>
Subject: Re: BitmapHeapScan streaming read user and prelim refactoring
Date: Tue, 14 May 2024 20:33:32 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAKRu_ZwjB7b+_FS3h1_bdr4wY9xGcy47iE3o7kdHzOh3ALPfA@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAAKRu_YA2YBQNr+qE_hq-a4ohqDx+dD2wWekzOhmMUmYcJ0Knw@mail.gmail.com>
<CAAKRu_YVQdGVYFAu7ng84GvHWXoSvpBtz+xktm1nUB7H48Pj-Q@mail.gmail.com>
<[email protected]>
<CAAKRu_ZkL=BUB4FWx+P3Wcf-JiDYSf-NkuPm3Z94G3So+S=R0w@mail.gmail.com>
<[email protected]>
<CAAKRu_amE9MxcXSmHfjLQkx4CtoqHyAJPvv3_iY3gfR6ZHZedg@mail.gmail.com>
<[email protected]>
<CAAKRu_a+5foybidkmh8FpFAV7iegxetPyPXQ5=++kqZ+ZDEUcg@mail.gmail.com>
<[email protected]>
<CAAKRu_ZwjB7b+_FS3h1_bdr4wY9xGcy47iE3o7kdHzOh3ALPfA@mail.gmail.com>
On 5/14/24 19:42, Melanie Plageman wrote:
> On Tue, May 14, 2024 at 2:18 AM Michael Paquier <[email protected]> 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
view thread (27+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BitmapHeapScan streaming read user and prelim refactoring
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox