Hi,
Thank you for the new version! There are several points I noticed:
1)
On Tue, Sep 9, 2025 at 3:08 AM Rishu Bagga <rishu.postgres@gmail.com> wrote:
>
> ...
> On Sat, Sep 6, 2025 at 7:52 AM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
>
> > Your patch already aims to fix the issue? On [2] I implemented a TAP
> > test that reproduce the issue and I tried to execute using your patch
> > and I still see the error. I'm attaching the TAP test isolated and
> > maybe we could incorporate into your patch series to ensure that the
> > issue is fixed? What do you think?
>
> I wasn’t able to run the TAP tests; however, in the updated patch, we
> can be confident that entries in the queue are from committed
> transactions. If there is a crash before committing and after writing to
> the queue, this would be within the critical section, so a notification
> from an uncommitted transaction would never be read in the queue.
> That allows us to remove the XidInMVCCSnapshot and
> TransactionIdDidCommit check.
>
I agree that listeners don't need to check if the notify transaction
was committed or not anymore, but it seems that listeners still need
to wait until the notify transaction is completed before sending its
notifications. I believe we should continue using XidInMVCCSnapshot as
the current version does.
2) Now we have two calls to SignalBackends (is it intentional?). The
first in AtCommit_Notify() which seems correct to me. The second in
XactLogCommitRecord() seems too early, because other backends can't
process new notifications at this point (if the point about
XidInMVCCSnapshot is correct). And there is Assert failure (Matheus'
report is about it).
3) I think we still need to check if the queue is full or not while
adding new entries and ereport if it is (entries are smaller now, but
we still can run out of space). And it seems to me that we should do
this check before entering the critical section, because in the
critical section it will result in PANIC. Master results in ERROR in
case of the full queue, so should we do the same here?
4) I don't know, but It seems to me that XactLogCommitRecord is not
the right place for adding listen/notify logic, it seems to be only
about wal stuff.
5) Do we want to use NotifyQueueLock as a lock that provides the
commit order? Maybe we can introduce another lock to avoid unnecessary
contantion? For example, if we use NotifyQueueLock, we block listeners
that want to read new notifications while we are inserting a commit
record, which seems unnecessary.
6) We have fixed size queue entries, so I think we don't need this
"padding" logic at the end of the page anymore, because we know how
many entries we can have on each page.
BTW, what do you think about creating a separate thread for the patch?
The current thread's subject seems a bit irrelevant.
Best regards,
Arseniy Mukhin
