On Sat Oct 18, 2025 at 2:43 AM -03, Joel Jacobson wrote:
> On Fri, Oct 17, 2025, at 22:50, Arseniy Mukhin wrote:
> What a funny coincidence that the approach in this patch,
> has one similarity with the "Direct advancement" approach
> in the patch in the "Optimize LISTEN/NOTIFY" [1] thread,
> namely that we're both interested in QUEUE_HEAD before/after
> we push the notifications into the queue, in PreCommit_Notify().
>
> It looks to me like our new data structures are Interchangeable,
> so I guess we probably want both patches to eventually settle
> on one and the same?
>
> The differences I note between our queue head before/after code are:
>
> - In this patch, you are palloc'ing a struct with two fields.
> In [1], we're using two separate static QueuePosition variables.
>
> - In this patch, you are taking/releasing a shared lock before/after
> the loop to read QUEUE_HEAD and set previousHead/head.
> In [1], we avoid the need of the shared lock, by doing the reads
> within the existing exclusive lock inside the loop, but instead
> therefore need a firstIteration bool, to know which is the first
> iteration, and need to overwrite the after-var in each iteration.
>
> I don't think the noted differences above matter, both seems fine.
>
Yeah, I also think that both approach seems fine. I keep the v8 version
with the palloc, if someone has any concern about this I'm open to
switch to another approach.
> Another thing I noticed in your patch that made me wonder,
> is the naming of the new AsyncQueueEntry bool field,
> which is given the name "committed".
>
> I think this name is not entirely faithful, since when set to true,
> the entry has not been committed yet.
>
> How about negating the meaning of this boolean field?
> To instead indicate when the entry has been rollbacked.
> Then, it would clearly communicate just that.
>
> Maybe naming it something like "rollbacked" or "aborted"?
>
Good point. I've renamed this field on the attached v8 version.
--
Matheus Alcantara
