On Fri, Oct 17, 2025, at 22:50, Arseniy Mukhin wrote:
> On Fri, Oct 17, 2025 at 7:31 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> I have the impression that this thread has lost focus on the idea of
>> producing a backpatchable bugfix. The last proposed patch has a lot of
>> new mechanism that doesn't seem suitable for backpatch. I could be
>> wrong of course.
>>
>
> Oops, I guess the TAP test that I added in the last version and that
> uses injection points is one of those things. PFA the new version
> without it. I also noticed that CheckSharedObjectLockedByMe() which
> was introduced in one of previous versions is unused, so I removed it
> too.
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.
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"?
/Joel
[1] https://www.postgresql.org/message-id/flat/6899c044-4a82-49be-8117-e6f669765f7e%40app.fastmail.com
