On 12.03.2025 23:50, Peter Geoghegan wrote:
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;".
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
