Re: BUG #15293: Stored Procedure Triggered by Logical Replication is Unable to use Notification Events

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Artur Zakirov <artur(dot)zakirov(at)adjust(dot)com>
Cc: Sergey Fedchenko <seregayoga(at)bk(dot)ru>, Daniel Danzberger <daniel(at)dd-wrt(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Sergei Kornilov <sk(at)zsrv(dot)org>, pgsql-hackers(at)postgresql(dot)org, Peter Eisentraut <peter_e(at)gmx(dot)net>, Marc Dean <marc(dot)dean(dot)jr(at)gmail(dot)com>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, "michael(dot)paul(dot)powers(at)gmail(dot)com" <michael(dot)paul(dot)powers(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Welin <robert(at)vaion(dot)com>
Subject: Re: BUG #15293: Stored Procedure Triggered by Logical Replication is Unable to use Notification Events
Date: 2021-09-12 17:29:58
Message-ID: 284756.1631467798@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

Artur Zakirov <artur(dot)zakirov(at)adjust(dot)com> writes:
> [ v2-0001-signal-backends-on-commit.patch ]

I had an epiphany while looking at this. Now that PostgresMain
calls ProcessNotifyInterrupt at the same place it calls
ProcessCompletedNotifies (which it does since 790026972), we don't
actually need ProcessCompletedNotifies to read self-notifies either.
If we merely set notifyInterruptPending, ProcessNotifyInterrupt will
handle that just fine. With the other changes already under discussion,
this means ProcessCompletedNotifies can go away entirely.

That's not only less code, but fewer cycles: in cases where we have both
self-notifies and inbound notify signals, the existing code starts two
transactions and runs asyncQueueReadAllNotifications twice, but there's
no need to do it more than once. Self-notifies become less of a special
case on the sending side too, since we can just treat that as signalling
ourselves --- though it still seems worthwhile to optimize that by
setting notifyInterruptPending directly instead of invoking kill().

Hence, I present the attached, which also tweaks things to avoid an
extra pq_flush in the end-of-command code path, and improves the
comments to discuss the issue of NOTIFYs sent by procedures.

There is still a loose end we ought to think about: what to do when
someone issues LISTEN in a background worker. With the code as
it stands, or with this patch, the worker will block cleanout of
the async SLRU since it will never read any messages. (With
the code as it stands, a bgworker author can ameliorate that by
calling ProcessCompletedNotifies, but this patch is going to either
eliminate ProcessCompletedNotifies or turn it into a no-op. In
any case, we still have a problem if an ill-considered trigger
issues LISTEN in a replication worker.)

I'm inclined to think we should flat-out reject LISTEN in any process
that is not attached to a frontend, at least until somebody takes the
trouble to add infrastructure that would let it be useful. I've not
done that here though; I'm not quite sure what we should test for.

regards, tom lane

Attachment Content-Type Size
v3-0001-signal-backends-on-commit.patch text/x-diff 17.0 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Andrew Dunstan 2021-09-12 18:41:23 Re: pg_upgrade test for binary compatibility of core data types
Previous Message Justin Pryzby 2021-09-12 00:51:16 Re: pg_upgrade test for binary compatibility of core data types

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2021-09-12 18:41:23 Re: pg_upgrade test for binary compatibility of core data types
Previous Message Pavel Stehule 2021-09-12 17:15:05 Re: Schema variables - new implementation for Postgres 15