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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

PG Doc comments form
The following bug has been logged on the website:

Bug reference:      15293
Logged by:          Michael Powers
Email address:      [hidden email]
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.

Reply | Threaded
Open this post in threaded view
|

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

Sergei Kornilov
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

Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Marc Dean
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 <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Sergei Kornilov
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

bug15293.patch (914 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Michael Powers
In reply to this post by PG Doc comments form
Hi Everyone,

Thanks for the attention. I've tested Sergei's patch and it does appear to resolve the issue for me.

Thanks!
Michael Powers
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
In reply to this post by Andres Freund
Hi,

On 2018-07-24 17:43:30 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Robert Welin
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
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

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

Alan Kleiman
In reply to this post by Robert Welin

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
[hidden email]