Thank you Tom for your review.
On Mon, Sep 6, 2021 at 9:27 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Artur Zakirov <artur.zaki...@adjust.com> writes:
> > I attached the patch which fixes it in a different way. It calls
> > SignalBackends() in AtCommit_Notify(). It is possible to call
> > SignalBackends()
> > outside of ProcessCompletedNotifies() after the commit
> > 51004c7172b5c35afac050f4d5849839a06e8d3b, which removes necessity of
> > checking
> > of SignalBackends()'s result.
>
> Hm. So that forecloses back-patching this to earlier than v13.
> On the other hand, given that we've been ignoring the bug for awhile,
> maybe a fix that only works in v13+ is good enough. (Or maybe by now
> it'd be safe to back-patch the v13-era async.c changes? Don't really
> want to, though.)
I think it would be better to back-patch the fix to older versions
than v13. But considering that the new patch renames
ProcessCompletedNotifies(), it can break existing applications which
use background workers and NOTIFY. Users can be upset with the change,
since they don't face the original bug, they just call
ProcessCompletedNotifies() manually.
In that case v13+ fix can be good enough. But users who use logical
replication and have the NOTIFY bug will have to update to the new
version of Postgres.
> The larger problem with this patch is exactly what Andres said: if
> a replication worker or other background process is sending notifies,
> but no normal backend is listening, then nothing will ever call
> asyncQueueAdvanceTail() so the message queue will bloat until it
> overflows and things start failing. That's not OK. Perhaps it
> could be fixed by moving the "if (backendTryAdvanceTail)" stanza
> into AtCommit_Notify. That's not ideal, because really it's best
> to not do that till after we've read our own notifies, but it might
> be close enough. At that point ProcessCompletedNotifies's only
> responsibility would be to call asyncQueueReadAllNotifications,
> which would allow some simplifications.
Agree, I moved asyncQueueAdvanceTail() to AtCommit_Notify().
> * I think that it's safe to move these actions to AtCommit_Notify,
> given where that is called in the CommitTransaction sequence, but
> there is nothing in xact.c suggesting that that call is in any way
> ordering-critical (because today, it isn't). I think we need some
> comments there to prevent somebody from breaking this in future.
> Maybe about like
I added comments before and after AtCommit_Notify().
> * You need to spend more effort on the comments in async.c too. Some
> of these changes are wrong plus there are places that should be changed
> and weren't. Also, postgres.c's comment about ProcessCompletedNotifies
> is invalidated by this patch.
I fixed these parts. I removed some excessive changes and also fixed
the comment in postgres.c.
> * There is some verbiage about NOTIFY in bgworker.sgml, which looks
> like it may be wrong now, and it certainly will be wrong after this
> patch. We don't want to be encouraging bgworkers to call
> ProcessCompletedNotifies. Maybe we should rename it, to force
> the issue?
I removed this part of documentation and renamed the function to
ProcessBackendNotifies(). It process only self-notifies now, but
ProcessBackendNotifies is good enough to express that the function
processes notifies of the backend.
I also renamed backendTryAdvanceTail to tryAdvanceTail, since it is
used not only by backends.
--
Artur
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index c0811935a1..453063b90d 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -280,15 +280,10 @@ typedef struct BackgroundWorker
</para>
<para>
- If a background worker sends asynchronous notifications with the
- <command>NOTIFY</command> command via the Server Programming Interface
- (<acronym>SPI</acronym>), it should call
- <function>ProcessCompletedNotifies</function> explicitly after committing
- the enclosing transaction so that any notifications can be delivered. If a
- background worker registers to receive asynchronous notifications with
- the <command>LISTEN</command> through <acronym>SPI</acronym>, the worker
- will log those notifications, but there is no programmatic way for the
- worker to intercept and respond to those notifications.
+ If a background worker registers to receive asynchronous notifications with
+ the <command>LISTEN</command> through <acronym>SPI</acronym>, the worker will
+ log those notifications, but there is no programmatic way for the worker to
+ intercept and respond to those notifications.
</para>
<para>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 387f80419a..c0c633cfa4 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2269,7 +2269,16 @@ CommitTransaction(void)
*/
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();
+
+ /*
+ * Everything after this should be purely-internal-to-this-backend cleanup.
+ */
AtEOXact_GUC(true, 1);
AtEOXact_SPI(true);
AtEOXact_Enum();
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 4b16fb5682..2059b07696 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -430,7 +430,7 @@ static bool amRegisteredListener = false;
static bool backendHasSentNotifications = false;
/* have we advanced to a page that's a multiple of QUEUE_CLEANUP_DELAY? */
-static bool backendTryAdvanceTail = false;
+static bool tryAdvanceTail = false;
/* GUC parameter */
bool Trace_notify = false;
@@ -984,7 +984,13 @@ PreCommit_Notify(void)
*
* This is called at transaction commit, after committing to clog.
*
- * Update listenChannels and clear transaction-local state.
+ * Update listenChannels and clear transaction-local state. If we issued
+ * any notifications in the transaction, send signals to other backends to
+ * process them.
+ *
+ * Also, if we filled enough queue pages with new notifies, try to advance
+ * the queue tail pointer. This isn't ideal place for it, because our
+ * backend will send self-notifies after the transaction commited.
*/
void
AtCommit_Notify(void)
@@ -1027,6 +1033,22 @@ AtCommit_Notify(void)
if (amRegisteredListener && listenChannels == NIL)
asyncQueueUnregister();
+ /*
+ * Send signals to other backends. We call SignalBackends() only if there
+ * are pending notifies which are added to the queue by PreCommit_Notify().
+ */
+ if (pendingNotifies != NULL)
+ SignalBackends();
+
+ /*
+ * If it's time to try to advance the global tail pointer, do that.
+ */
+ if (tryAdvanceTail)
+ {
+ tryAdvanceTail = false;
+ asyncQueueAdvanceTail();
+ }
+
/* And clean up */
ClearPendingActionsAndNotifies();
}
@@ -1200,14 +1222,12 @@ Exec_UnlistenAllCommit(void)
}
/*
- * ProcessCompletedNotifies --- send out signals and self-notifies
+ * ProcessBackendNotifies --- send out self-notifies
*
* This is called from postgres.c just before going idle at the completion
* of a transaction. If we issued any notifications in the just-completed
- * transaction, send signals to other backends to process them, and also
- * process the queue ourselves to send messages to our own frontend.
- * Also, if we filled enough queue pages with new notifies, try to advance
- * the queue tail pointer.
+ * transaction, process the queue ourselves to send messages to our own
+ * frontend.
*
* The reason that this is not done in AtCommit_Notify is that there is
* a nonzero chance of errors here (for example, encoding conversion errors
@@ -1216,14 +1236,13 @@ Exec_UnlistenAllCommit(void)
* to ensure that a transaction's self-notifies are delivered to the frontend
* before it gets the terminating ReadyForQuery message.
*
- * Note that we send signals and process the queue even if the transaction
- * eventually aborted. This is because we need to clean out whatever got
- * added to the queue.
+ * Note that we process the queue even if the transaction eventually aborted.
+ * This is because we need to clean out whatever got added to the queue.
*
* NOTE: we are outside of any transaction here.
*/
void
-ProcessCompletedNotifies(void)
+ProcessBackendNotifies(void)
{
MemoryContext caller_context;
@@ -1238,6 +1257,10 @@ ProcessCompletedNotifies(void)
*/
backendHasSentNotifications = false;
+ /* Nothing to do if we are not listening any channels */
+ if (listenChannels == NIL)
+ return;
+
/*
* We must preserve the caller's memory context (probably MessageContext)
* across the transaction we do here.
@@ -1245,7 +1268,7 @@ ProcessCompletedNotifies(void)
caller_context = CurrentMemoryContext;
if (Trace_notify)
- elog(DEBUG1, "ProcessCompletedNotifies");
+ elog(DEBUG1, "ProcessBackendNotifies");
/*
* We must run asyncQueueReadAllNotifications inside a transaction, else
@@ -1253,23 +1276,8 @@ ProcessCompletedNotifies(void)
*/
StartTransactionCommand();
- /* Send signals to other backends */
- SignalBackends();
-
- if (listenChannels != NIL)
- {
- /* Read the queue ourselves, and send relevant stuff to the frontend */
- asyncQueueReadAllNotifications();
- }
-
- /*
- * If it's time to try to advance the global tail pointer, do that.
- */
- if (backendTryAdvanceTail)
- {
- backendTryAdvanceTail = false;
- asyncQueueAdvanceTail();
- }
+ /* Read the queue ourselves, and send relevant stuff to the frontend */
+ asyncQueueReadAllNotifications();
CommitTransactionCommand();
@@ -1543,7 +1551,7 @@ asyncQueueAddEntries(ListCell *nextNotify)
* pointer (we don't want to actually do that right here).
*/
if (QUEUE_POS_PAGE(queue_head) % QUEUE_CLEANUP_DELAY == 0)
- backendTryAdvanceTail = true;
+ tryAdvanceTail = true;
/* And exit the loop */
break;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 58b5960e27..5da52d0590 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4373,8 +4373,8 @@ PostgresMain(int argc, char *argv[],
}
else
{
- /* Send out notify signals and transmit self-notifies */
- ProcessCompletedNotifies();
+ /* Send out self-notifies */
+ ProcessBackendNotifies();
/*
* Also process incoming notifies, if any. This is mostly to
diff --git a/src/include/commands/async.h b/src/include/commands/async.h
index 9217f66b91..aeff4e2689 100644
--- a/src/include/commands/async.h
+++ b/src/include/commands/async.h
@@ -43,7 +43,7 @@ extern void AtAbort_Notify(void);
extern void AtSubCommit_Notify(void);
extern void AtSubAbort_Notify(void);
extern void AtPrepare_Notify(void);
-extern void ProcessCompletedNotifies(void);
+extern void ProcessBackendNotifies(void);
/* signal handler for inbound notifies (PROCSIG_NOTIFY_INTERRUPT) */
extern void HandleNotifyInterrupt(void);
