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

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

--
Regards,
Alena Rybakina
Postgres Professional