On Wed Sep 24, 2025 at 10:59 AM -03, Arseniy Mukhin wrote:
> Thank you! Speaking of the scenario, my understanding is that it's
> impossible as we hold the global lock, so QueuePosition head should
> always be equal to QUEUE_HEAD when we get into At_AbortNotify(), but
> maybe I'm wrong.
>
Let's see if anyone else has any thoughts about this.
> Patch looks great. Some minor points:
>
> I have a warning when using git am with the patch:
> warning: 1 line adds whitespace errors.
>
I don't think that this is a problem? And I don't know how to remove
this warning, I've just created the patch with git format-patch @~1
> There is a comment in the head of the async.c file about some
> listen/notify internals. Maybe it's worth adding a comment about how
> aborted transactions do clean up.
>
Added
> What do you think about a Assert in asyncQueueRollbackNotifications()
> that other backends still see us as 'in progress'? So we can be sure
> that they can't process our notifications before we mark notifications
> as 'committed=false'. Not sure how to do it correctly, maybe
>
> Assert(TransactionIdIsValid(MyProc->xid));
>
> will work? The TAP test that I tried to write also should test it.
>
Sounds resanable to me. I think that we could use the following assert
when iterating over the entries, what do you think?
Assert(TransactionIdIsInProgress(qe->xid));
> There are several comments where the word "crash" is used. What do you
> think about using "abort" instead? "Crash" sounds more like PANIC
> situation where we don't care about notifications because they don't
> survive restart.
>
Good point, fixed
> Thank you!
>
Thanks for reviewing this!
--
Matheus Alcantara
