Re: Adding skip scan (including MDAM style range skip scan) to nbtree

On 18.03.2025 13:54, Alena Rybakina wrote:
>
>
> On 12.03.2025 23:50, Peter Geoghegan wrote:
>> On Wed, Mar 12, 2025 at 4:28 PM Alena Rybakina
>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>> Thank you for the explanation!
>>>
>>> Now I see why these changes were made.
>>>
>>> After your additional explanations, everything really became clear and I
>>> fully agree with the current code regarding this part.
>> Cool.
>>
>>> However I did not see an explanation to the commit regarding this place,
>>> as well as a comment next to the assert and the parallel_aware check and
>>> why BitmapIndexScanState was added in the ExecParallelReInitializeDSM.
>> I added BitmapIndexScanState to the switch statement in
>> ExecParallelReInitializeDSM because it is in the category of
>> planstates that never need their shared memory reinitialized -- that's
>> just how we represent such a plan state there.
>>
>> I think that this is supposed to serve as a kind of documentation,
>> since it doesn't really affect how things behave. That is, it wouldn't
>> actually affect anything if I had forgotten to add
>> BitmapIndexScanState to the ExecParallelReInitializeDSM switch
>> statement "case" that represents that it is in this "plan state
>> category": the switch ends with catch-all "default: break;".
> Agree.
>>> In my opinion, there is not enough additional explanation about this in
>>> the form of comments, although I think that it has already been
>>> explained here enough for someone who will look at this code.
>> What can be done to improve the situation? For example, would adding a
>> comment next to the new assertions recently added to
>> ExecIndexScanReInitializeDSM and ExecIndexOnlyScanReInitializeDSM be
>> an improvement? And if so, what would the comment say?
>>
> After reviewing the logic again, I realized that I was confused
> precisely in the reinitialization of memory for IndexScanState and
> IndexOnlyScanState.
>
> As far as I can see, either assert is not needed here, the functions
> ExecIndexScanReInitializeDSM and ExecIndexScanReInitializeDSM can be
> called only if parallel_aware is positive, or it makes sense that
> reinitialization is needed only if parallel_aware is positive, then
> the condition noted above is not needed. According to your letter (0),
> the check should be removed there too, but I got confused in the
> comment. We do not need to reinitialize memory because DSM is
> instrumentation state only, but it turns out that we are
> reinitializing the memory, so we don't do it at all?
>
> I attached a diff file to the letter with the comment.
>
> [0]
> https://www.postgresql.org/message-id/CAH2-WzkMpFsE_hM9-5tecF22jVJSGtKMFMsYqMa-uo73MOxsWw%40mail.gmail.com
>
>
Sorry, I figured it out. The Assert was added to avoid misuse of the
function to reinitialize memory and to ensure that it happens when
parallel_aware is positive.

--
Regards,
Alena Rybakina
Postgres Professional