Horton

Thread

Commits

  1. Disallow LISTEN in background workers.

  2. Send NOTIFY signals during CommitTransaction.

  3. Make some efficiency improvements in LISTEN/NOTIFY.

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

    The Post Office <noreply@postgresql.org> — 2018-07-24T12:13:34Z

    The following bug has been logged on the website:

Bug reference:      15293
Logged by:          Michael Powers
Email address:      michael.paul.powers@gmail.com
PostgreSQL version: 10.4
Operating system:   Ubuntu 18.04 Desktop x64 - Linux 4.15.0-29-generic
Description:        

When using logical replication a stored procedure executed on the replica is
unable to use NOTIFY to send messages to other listeners. The stored
procedure does execute as expected but the pg_notify() doesn't appear to
have any effect. If an insert is run on the replica side the trigger
executes the stored procedure as expected and the NOTIFY correctly notifies
listeners.

Steps to Reproduce:
Set up Master:
CREATE TABLE test (id SERIAL PRIMARY KEY, msg TEXT NOT NULL);
CREATE PUBLICATION testpub FOR TABLE test;

Set up Replica:
CREATE TABLE test (id SERIAL PRIMARY KEY, msg TEXT NOT NULL);
CREATE SUBSCRIPTION testsub CONNECTION 'host=192.168.0.136 user=test
password=test' PUBLICATION testpub;
CREATE OR REPLACE FUNCTION notify_channel() RETURNS trigger AS $$
BEGIN
    RAISE LOG 'Notify Triggered';
    PERFORM pg_notify('testchannel', 'Testing');
    RETURN NEW;
END;
$$ LANGUAGE 'plpgsql';
DROP TRIGGER queue_insert ON TEST;
CREATE TRIGGER queue_insert AFTER INSERT ON test FOR EACH ROW EXECUTE
PROCEDURE notify_channel();
ALTER TABLE test ENABLE ALWAYS TRIGGER queue_insert;
LISTEN testchannel;

Run the following insert on the master:
INSERT INTO test (msg) VALUES ('test');

In postgresql-10-main.log I get the following:
2018-07-24 07:45:15.705 EDT [6701] LOG:  00000: Notify Triggered
2018-07-24 07:45:15.705 EDT [6701] CONTEXT:  PL/pgSQL function
notify_channel() line 3 at RAISE
2018-07-24 07:45:15.705 EDT [6701] LOCATION:  exec_stmt_raise,
pl_exec.c:3337

But no listeners receive the message. However if an insert is run directly
on the replica:
# INSERT INTO test VALUES (99999, 'test');
INSERT 0 1
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 6701.
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 6701.
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 6701.
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 6701.
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 6701.
Asynchronous notification "testchannel" with payload "Testing" received from
server process with PID 9992.

Backed up notifications are received for previous NOTIFY's.

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

    Sergei Kornilov <sk@zsrv.org> — 2018-07-24T13:19:45Z

    Hello
Thank you for report

I checked this bug and found reason: we do not notify backends about new events by call ProcessCompletedNotifies from logical worker.
New notify from regular backend does call ProcessCompletedNotifies: send signal to all listen backends and found new events for youself.
But i am not sure where is correct place for call ProcessCompletedNotifies in logical worker src/backend/replication/logical/worker.c and i can not provide patch.

regards, Sergei

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

    Andres Freund <andres@anarazel.de> — 2018-07-24T15:58:29Z

    Hi,

On 2018-07-24 16:19:45 +0300, Sergei Kornilov wrote:
> I checked this bug and found reason: we do not notify backends about new events by call ProcessCompletedNotifies from logical worker.
> New notify from regular backend does call ProcessCompletedNotifies: send signal to all listen backends and found new events for youself.
> But i am not sure where is correct place for call ProcessCompletedNotifies in logical worker src/backend/replication/logical/worker.c and i can not provide patch.

Peter, Petr, this is the second report of this issue. Anything?

Greetings,

Andres Freund

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

    Marc Dean <marc.dean.jr@gmail.com> — 2018-07-24T17:43:30Z

    If Sergei is correct, I would volunteer to work on the patch. I am
completely new to the codebase but this issue affects me. According to the
documentation for `ProcessCompletedNotifies()` it should be called just
before going idle... so perhaps in src/backend/replication/logical/worker.c
at the tail end of `apply_handle_commit`? Again.. just looking at the
codebase today so if this is beyond beginner level I will assist w/ testing
instead.

On Tue, Jul 24, 2018 at 11:58 AM, Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> On 2018-07-24 16:19:45 +0300, Sergei Kornilov wrote:
> > I checked this bug and found reason: we do not notify backends about new
> events by call ProcessCompletedNotifies from logical worker.
> > New notify from regular backend does call ProcessCompletedNotifies: send
> signal to all listen backends and found new events for youself.
> > But i am not sure where is correct place for call
> ProcessCompletedNotifies in logical worker src/backend/replication/logical/worker.c
> and i can not provide patch.
>
> Peter, Petr, this is the second report of this issue. Anything?
>
> Greetings,
>
> Andres Freund
>
>

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

    Sergei Kornilov <sk@zsrv.org> — 2018-07-24T18:22:18Z

    Hello
in fact, I've already tried to build fix. Adding ProcessCompletedNotifies to apply_handle_commit fixed this issue and i think this is right place. In src/backend/tcop/postgres.c we call ProcessCompletedNotifies similar way after commit. This change pass make check-world.
So i attach my two line patch.

regards, Sergei

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

    Andres Freund <andres@anarazel.de> — 2018-07-24T19:27:40Z

    Hi Tom, Peter,

On 2018-07-24 21:22:18 +0300, Sergei Kornilov wrote:
> in fact, I've already tried to build fix. Adding ProcessCompletedNotifies to apply_handle_commit fixed this issue and i think this is right place. In src/backend/tcop/postgres.c we call ProcessCompletedNotifies similar way after commit. This change pass make check-world.
> So i attach my two line patch.

> diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
> index 6ca6cdc..e54bd90 100644
> --- a/src/backend/replication/logical/worker.c
> +++ b/src/backend/replication/logical/worker.c
> @@ -37,6 +37,7 @@
>  
>  #include "commands/tablecmds.h"
>  #include "commands/trigger.h"
> +#include "commands/async.h"
>  
>  #include "executor/executor.h"
>  #include "executor/nodeModifyTable.h"
> @@ -490,6 +491,7 @@ apply_handle_commit(StringInfo s)
>  		replorigin_session_origin_timestamp = commit_data.committime;
>  
>  		CommitTransactionCommand();
> +		ProcessCompletedNotifies();
>  		pgstat_report_stat(false);
>  
>  		store_flush_position(commit_data.end_lsn);

That's probably reasonable for the back branches (although I'd put the
store_flush_position before).

But I wonder if we shouldn't actually move the signalling part of
ProcessCompletedNotifies() into CommitTransactionCommand() in v11. Given
that transactions can now commit without a ready command being sent, due
to the addition of procedures, that kind of seems necessary?

Greetings,

Andres Freund

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-24T21:43:30Z

    Andres Freund <andres@anarazel.de> writes:
> But I wonder if we shouldn't actually move the signalling part of
> ProcessCompletedNotifies() into CommitTransactionCommand() in v11. Given
> that transactions can now commit without a ready command being sent, due
> to the addition of procedures, that kind of seems necessary?

Hrm.  I have a nasty feeling that that code is dependent on being executed
at the outermost logic level.  In particular, ProcessCompletedNotifies
calls CommitTransactionCommand itself, so your proposal will create
infinite recursion.  There may be some other issues too.

Another question that needs consideration is whether an internal commit
should lead to immediate distribution of notifies to our own client.
I think it probably mustn't; from the standpoint of the client, its
originally-asked-for xact is still in progress, and it's not going to
expect any notifies until that ends.  So the proposed change is just
wrong if you ask me.

I agree we need some serious rethinking here.  Maybe the fix will end
up being just a few lines, but it might take significant restructuring
too.

			regards, tom lane

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

    Andres Freund <andres@anarazel.de> — 2018-07-24T21:56:04Z

    Hi,

On 2018-07-24 17:43:30 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > But I wonder if we shouldn't actually move the signalling part of
> > ProcessCompletedNotifies() into CommitTransactionCommand() in v11. Given
> > that transactions can now commit without a ready command being sent, due
> > to the addition of procedures, that kind of seems necessary?
> 
> Hrm.  I have a nasty feeling that that code is dependent on being executed
> at the outermost logic level.  In particular, ProcessCompletedNotifies
> calls CommitTransactionCommand itself, so your proposal will create
> infinite recursion.  There may be some other issues too.

Yea, I don't think we could do this without separating concerns in
ProcessCompletedNotifies().


> Another question that needs consideration is whether an internal commit
> should lead to immediate distribution of notifies to our own client.
> I think it probably mustn't; from the standpoint of the client, its
> originally-asked-for xact is still in progress, and it's not going to
> expect any notifies until that ends.

Yea, I agree on that.


> So the proposed change is just wrong if you ask me.

I was only proposing to move the signalling part of
ProcessCompletedNotifies() into CommitTransactionCommand(), not the part
that processes notifications for the currentbackend - so I don't think
we actually disagree?


> I agree we need some serious rethinking here.  Maybe the fix will end
> up being just a few lines, but it might take significant restructuring
> too.

Yea :(.  I think we need to separate the SignalBackend() part into
transaction commit, but leave the remainder of
ProcessCompletedNotifies() to be done in outer loops like
PostgresMain().  I'm not quite sure if there's a good way to handle the
fact that currently the asyncQueueAdvanceTail() call depends on
SignalBackend()'s return value.  We probably don't want to do that work
inside the CommitTransactionCommand() - i guess we could move to just
doing it independent of SignalBackend()?

One other thing, somewhat independent, I wonder is if it's actually
problematic that we don't do ProcessCompletedNotifies() in a bunch of
processes, because that means we'll not necessarily call
asyncQueueAdvanceTail().  Perhaps that means we actually *do* need to do
it around CommitTransactionCommand()?

Greetings,

Andres Freund

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-24T22:01:33Z

    Andres Freund <andres@anarazel.de> writes:
> One other thing, somewhat independent, I wonder is if it's actually
> problematic that we don't do ProcessCompletedNotifies() in a bunch of
> processes, because that means we'll not necessarily call
> asyncQueueAdvanceTail().  Perhaps that means we actually *do* need to do
> it around CommitTransactionCommand()?

As far as that goes, we should probably ensure that a process that hasn't
executed any LISTENs is ignored for purposes of whether to advance the
queue tail.  I think it might be like that already.

			regards, tom lane

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

    Andres Freund <andres@anarazel.de> — 2018-07-24T22:06:30Z

    On 2018-07-24 18:01:33 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > One other thing, somewhat independent, I wonder is if it's actually
> > problematic that we don't do ProcessCompletedNotifies() in a bunch of
> > processes, because that means we'll not necessarily call
> > asyncQueueAdvanceTail().  Perhaps that means we actually *do* need to do
> > it around CommitTransactionCommand()?
> 
> As far as that goes, we should probably ensure that a process that hasn't
> executed any LISTENs is ignored for purposes of whether to advance the
> queue tail.  I think it might be like that already.

It indeed is:
	min = QUEUE_HEAD;
	for (i = 1; i <= MaxBackends; i++)
	{
		if (QUEUE_BACKEND_PID(i) != InvalidPid)
			min = QUEUE_POS_MIN(min, QUEUE_BACKEND_POS(i));
	}

what I am wondering is what happens if there's a background worker (like
the replication worker, but it could be other things too) that queues
notifications, but no normal backends are actually listening. As far as
I can tell, in that case we'd continue to queue stuff into the slru, but
wouldn't actually clean things up until a normal session gets around to
it? Which might be a while, on e.g. a logical replica.

Greetings,

Andres Freund

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

    Robert Welin <robert@vaion.com> — 2019-02-22T16:37:50Z

    I have reproduced this bug on PostgreSQL version 10.7 and 11.2 using the
steps described in Michael Powers' original report. The issue also still
seems to be present even with the patch provided by Sergei Kornilov.

Are there plans to address this issue any time soon or is there some way
I can assist in fixing it? It would be great to have notifications from
logical replication.

Regards,
Robert Welin

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

    Marc Dean <marc.dean.jr@gmail.com> — 2019-02-28T01:01:16Z

    If you are trying to get around the issue for now, what my team did was
cron an insert statement on the database server. We have a queue table that
has some of these triggers setup so it was easy to write a no-op row to the
queue. This had the side effect of flushing the notification queue.

We haven't had any issues with this approach.

I can also volunteer to help test out patches.

Thanks,

-mdj

On Fri, Feb 22, 2019 at 11:37 AM Robert Welin <robert@vaion.com> wrote:

> I have reproduced this bug on PostgreSQL version 10.7 and 11.2 using the
> steps described in Michael Powers' original report. The issue also still
> seems to be present even with the patch provided by Sergei Kornilov.
>
> Are there plans to address this issue any time soon or is there some way
> I can assist in fixing it? It would be great to have notifications from
> logical replication.
>
> Regards,
> Robert Welin
>
>

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-06-28T21:22:24Z

    On 2019-Feb-22, Robert Welin wrote:

> I have reproduced this bug on PostgreSQL version 10.7 and 11.2 using the
> steps described in Michael Powers' original report. The issue also still
> seems to be present even with the patch provided by Sergei Kornilov.
> 
> Are there plans to address this issue any time soon or is there some way
> I can assist in fixing it? It would be great to have notifications from
> logical replication.

This issue seems largely forgotten about :-(

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Alan Kleiman <alan.kleiman@ifood.com.br> — 2019-08-29T15:03:22Z

    On Fri, Feb 22, 2019 at 11:37 AM Robert Welin <robert(at)vaion(dot)com>
wrote:

> I have reproduced this bug on PostgreSQL version 10.7 and 11.2 using the
> steps described in Michael Powers' original report. The issue also still
> seems to be present even with the patch provided by Sergei Kornilov.
>
> Are there plans to address this issue any time soon or is there some way
> I can assist in fixing it? It would be great to have notifications from
> logical replication.
>
> Same, I've reproduced this on 11.4 and 10.9. Are there any plans to
address this?
If there's nothing in the short-term, can we get a caveat in the
documentation for notify/logical
replication explaining this shortcoming?


-- 
Alan Kleiman
alan.kleiman@ifood.com.br

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

    Daniel Danzberger <daniel@dd-wrt.com> — 2020-06-23T09:40:53Z

    On Fri, 2019-06-28 at 17:22 -0400, Alvaro Herrera wrote:
> On 2019-Feb-22, Robert Welin wrote:
> 
> > I have reproduced this bug on PostgreSQL version 10.7 and 11.2
> > using the
> > steps described in Michael Powers' original report. The issue also
> > still
> > seems to be present even with the patch provided by Sergei
> > Kornilov.
> > 
> > Are there plans to address this issue any time soon or is there
> > some way
> > I can assist in fixing it? It would be great to have notifications
> > from
> > logical replication.
> 
> This issue seems largely forgotten about :-(
> 

Are there any news to this issue ?
I can reproduce this bug up to now with version 12.

-- 
Regards

Daniel Danzberger
embeDD GmbH, Alter Postplatz 2, CH-6370 Stans

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

    Sergey Fedchenko <seregayoga@bk.ru> — 2021-06-14T11:17:24Z

    Hi all! I still can reproduce it on 14beta1 version. I adapted a patch I found in this thread  https://github.com/seregayoga/postgres/commit/338bc33f2cf77edde7c45bfdfb9f39a92ec57eb8 . It solved this bug for me (tested with simple Go program using  https://github.com/lib/pq ). It would be nice to have this bug fixed. I’m not so familiar with postgres code base, but would glad to help with testing.

  
>Monday, June 14, 2021 12:39 PM +03:00 from Daniel Danzberger < daniel@dd-wrt.com >:
> 
>On Fri, 2019-06-28 at 17:22 -0400, Alvaro Herrera wrote:
>> On 2019-Feb-22, Robert Welin wrote:
>>
>> > I have reproduced this bug on PostgreSQL version 10.7 and 11.2
>> > using the
>> > steps described in Michael Powers' original report. The issue also
>> > still
>> > seems to be present even with the patch provided by Sergei
>> > Kornilov.
>> >
>> > Are there plans to address this issue any time soon or is there
>> > some way
>> > I can assist in fixing it? It would be great to have notifications
>> > from
>> > logical replication.
>>
>> This issue seems largely forgotten about :-(
>>
>Are there any news to this issue ?
>I can reproduce this bug up to now with version 12.
>
>--
>Regards
>
>Daniel Danzberger
>embeDD GmbH, Alter Postplatz 2, CH-6370 Stans
>
>
>
>  
 
 
Best regards, Sergey Fedchenko.
 
 

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

    Artur Zakirov <artur.zakirov@adjust.com> — 2021-07-22T09:04:07Z

    Hello hackers,

On Wed, Jul 14, 2021 at 11:30 AM Sergey Fedchenko <seregayoga@bk.ru> wrote:
>
> Hi all! I still can reproduce it on 14beta1 version. I adapted a patch I found in this thread https://github.com/seregayoga/postgres/commit/338bc33f2cf77edde7c45bfdfb9f39a92ec57eb8 . It solved this bug for me (tested with simple Go program using https://github.com/lib/pq). It would be nice to have this bug fixed. I’m not so familiar with postgres code base, but would glad to help with testing.

In our company in one of our projects we ran into this bug too.

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.

Moving SignalBackends() to AtCommit_Notify() makes it possible to signal
backends by "non-normal" backends including logical replication workers. It
seems it is safe to call SignalBackends() in AtCommit_Notify() since
SignalBackends() doesn't raise any error except OOM.

Regarding Andres concern:
> what I am wondering is what happens if there's a background worker (like
> the replication worker, but it could be other things too) that queues
> notifications, but no normal backends are actually listening. As far as
> I can tell, in that case we'd continue to queue stuff into the slru, but
> wouldn't actually clean things up until a normal session gets around to
> it? Which might be a while, on e.g. a logical replica.

A backend which raises notifications calls asyncQueueAdvanceTail() sometimes,
which truncates pages up to new tail, which is equal to head if there are no
listening backends. But there will be a problem if there is a backend which
is listening but it doesn't process incoming notifications and doesn't update
its queue position. In that case asyncQueueAdvanceTail() is able to advance
tail only up to that backend's queue position.

--
Artur Zakirov
PostgreSQL Developer at Adjust

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-06T19:27:32Z

    Artur Zakirov <artur.zakirov@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.)

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.

There are some sort-of-cosmetic-but-important-for-future-proofing
issues too:

* 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

	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);


* 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.

* 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?

			regards, tom lane

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

    Artur Zakirov <artur.zakirov@adjust.com> — 2021-09-11T19:55:16Z

    Thank you Tom for your review.

On Mon, Sep 6, 2021 at 9:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Artur Zakirov <artur.zakirov@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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-11T22:16:09Z

    Artur Zakirov <artur.zakirov@adjust.com> writes:
> On Mon, Sep 6, 2021 at 9:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

I've not looked at your new patch yet, but I did spend a little time
checking into whether it'd be sane to backpatch 51004c717, which
changed SignalBackends' API.  (We'd also need the non-catalog parts
of 8b7ae5a82, presumably.)  I came away feeling that the benefit
isn't worth the effort and risk.  There was an awful lot of churn
in async.c since v12, and to make matters worse, some of it was
back-patched already while some wasn't.  So 51004c717 doesn't apply
to v12 at all cleanly, and some of the merge problems are due to
code that's older than that commit but others are due to code that's
newer.  It would take some work just to get a patch that applies,
and I would not have a lot of faith in the result.  I think we're
better off just settling for a back-patch to v13.  The delta in
async.c between v13 and HEAD is not very large, so we'd not be
taking too much risk in going that far back.

As far as ProcessCompletedNotifies is concerned, I think what we
could do in the back branches is leave it in place as an empty,
no-op function, so as not to break extensions that are following
the existing documentation by calling it.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-12T17:29:58Z

    Artur Zakirov <artur.zakirov@adjust.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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-14T21:56:58Z

    I wrote:
> 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.

Hearing no comments, I pushed that.

> 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.

After a bit of looking around, it seems that MyBackendType addresses
that problem nicely, so I propose the attached.

			regards, tom lane

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

    Artur Zakirov <artur.zakirov@adjust.com> — 2021-09-15T12:03:25Z

    On Wed, Sep 15, 2021 at 2:57 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hearing no comments, I pushed that.

Thank you!

> > 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.
>
> After a bit of looking around, it seems that MyBackendType addresses
> that problem nicely, so I propose the attached.

Indeed, it seems only regular backends can send out notifies to
frontends. In that case there is no point in supporting LISTEN to
other processes. I guess a background worker can try it with SPI, but
with no luck.
+1 from my sight.

-- 
Artur