Commit 0eff10a0 authored by Tom Lane's avatar Tom Lane

Send NOTIFY signals during CommitTransaction.

Formerly, we sent signals for outgoing NOTIFY messages within
ProcessCompletedNotifies, which was also responsible for sending
relevant ones of those messages to our connected client.  It therefore
had to run during the main-loop processing that occurs just before
going idle.  This arrangement had two big disadvantages:

* Now that procedures allow intra-command COMMITs, it would be
useful to send NOTIFYs to other sessions immediately at COMMIT
(though, for reasons of wire-protocol stability, we still shouldn't
forward them to our client until end of command).

* Background processes such as replication workers would not send
NOTIFYs at all, since they never execute the client communication
loop.  We've had requests to allow triggers running in replication
workers to send NOTIFYs, so that's a problem.

To fix these things, move transmission of outgoing NOTIFY signals
into AtCommit_Notify, where it will happen during CommitTransaction.
Also move the possible call of asyncQueueAdvanceTail there, to
ensure we don't bloat the async SLRU if a background worker sends
many NOTIFYs with no one listening.

We can also drop the call of asyncQueueReadAllNotifications,
allowing ProcessCompletedNotifies to go away entirely.  That's
because commit 79002697 added a call of ProcessNotifyInterrupt
adjacent to PostgresMain's call of ProcessCompletedNotifies,
and that does its own call of asyncQueueReadAllNotifications,
meaning that we were uselessly doing two such calls (inside two
separate transactions) whenever inbound notify signals coincided
with an outbound notify.  We need only set notifyInterruptPending
to ensure that ProcessNotifyInterrupt runs, and we're done.

The existing documentation suggests that custom background workers
should call ProcessCompletedNotifies if they want to send NOTIFY
messages.  To avoid an ABI break in the back branches, reduce it
to an empty routine rather than removing it entirely.  Removal
will occur in v15.

Although the problems mentioned above have existed for awhile,
I don't feel comfortable back-patching this any further than v13.
There was quite a bit of churn in adjacent code between 12 and 13.
At minimum we'd have to also backpatch 51004c71, and a good deal
of other adjustment would also be needed, so the benefit-to-risk
ratio doesn't look attractive.

Per bug #15293 from Michael Powers (and similar gripes from others).

Artur Zakirov and Tom Lane

Discussion: https://postgr.es/m/153243441449.1404.2274116228506175596@wrigleys.postgresql.org
parent 29aa0ce3
...@@ -284,15 +284,13 @@ typedef struct BackgroundWorker ...@@ -284,15 +284,13 @@ typedef struct BackgroundWorker
</para> </para>
<para> <para>
If a background worker sends asynchronous notifications with the Background workers can send asynchronous notification messages, either by
<command>NOTIFY</command> command via the Server Programming Interface using the <command>NOTIFY</command> command via <acronym>SPI</acronym>,
(<acronym>SPI</acronym>), it should call or directly via <function>Async_Notify()</function>. Such notifications
<function>ProcessCompletedNotifies</function> explicitly after committing will be sent at transaction commit.
the enclosing transaction so that any notifications can be delivered. If a Background workers should not register to receive asynchronous
background worker registers to receive asynchronous notifications with notifications with the <command>LISTEN</command> command, as there is no
the <command>LISTEN</command> through <acronym>SPI</acronym>, the worker infrastructure for a worker to consume such notifications.
will log those notifications, but there is no programmatic way for the
worker to intercept and respond to those notifications.
</para> </para>
<para> <para>
......
...@@ -2269,7 +2269,17 @@ CommitTransaction(void) ...@@ -2269,7 +2269,17 @@ CommitTransaction(void)
*/ */
smgrDoPendingDeletes(true); smgrDoPendingDeletes(true);
/*
* Send out notification signals to other backends (and do other
* post-commit NOTIFY cleanup). This must not happen until after our
* transaction is fully done from the viewpoint of other backends.
*/
AtCommit_Notify(); AtCommit_Notify();
/*
* Everything after this should be purely internal-to-this-backend
* cleanup.
*/
AtEOXact_GUC(true, 1); AtEOXact_GUC(true, 1);
AtEOXact_SPI(true); AtEOXact_SPI(true);
AtEOXact_Enum(); AtEOXact_Enum();
......
This diff is collapsed.
...@@ -504,7 +504,7 @@ ProcessClientReadInterrupt(bool blocked) ...@@ -504,7 +504,7 @@ ProcessClientReadInterrupt(bool blocked)
/* Process notify interrupts, if any */ /* Process notify interrupts, if any */
if (notifyInterruptPending) if (notifyInterruptPending)
ProcessNotifyInterrupt(); ProcessNotifyInterrupt(true);
} }
else if (ProcDiePending) else if (ProcDiePending)
{ {
...@@ -4371,17 +4371,15 @@ PostgresMain(int argc, char *argv[], ...@@ -4371,17 +4371,15 @@ PostgresMain(int argc, char *argv[],
} }
else else
{ {
/* Send out notify signals and transmit self-notifies */
ProcessCompletedNotifies();
/* /*
* Also process incoming notifies, if any. This is mostly to * Process incoming notifies (including self-notifies), if
* ensure stable behavior in tests: if any notifies were * any, and send relevant messages to the client. Doing it
* received during the just-finished transaction, they'll be * here helps ensure stable behavior in tests: if any notifies
* seen by the client before ReadyForQuery is. * were received during the just-finished transaction, they'll
* be seen by the client before ReadyForQuery is.
*/ */
if (notifyInterruptPending) if (notifyInterruptPending)
ProcessNotifyInterrupt(); ProcessNotifyInterrupt(false);
pgstat_report_stat(false); pgstat_report_stat(false);
......
...@@ -49,6 +49,6 @@ extern void ProcessCompletedNotifies(void); ...@@ -49,6 +49,6 @@ extern void ProcessCompletedNotifies(void);
extern void HandleNotifyInterrupt(void); extern void HandleNotifyInterrupt(void);
/* process interrupts */ /* process interrupts */
extern void ProcessNotifyInterrupt(void); extern void ProcessNotifyInterrupt(bool flush);
#endif /* ASYNC_H */ #endif /* ASYNC_H */
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment