On Wed, Mar 12, 2025 at 4:28 PM Alena Rybakina
<a.rybakina@postgrespro.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;".
> 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?
--
Peter Geoghegan
