Date: Tue, 24 Jul 2018 12:27:40 -0700
From: Andres Freund <andres@anarazel.de>
To: Sergei Kornilov <sk@zsrv.org>, pgsql-hackers@postgresql.org,
	Peter Eisentraut <peter_e@gmx.net>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Marc Dean <marc.dean.jr@gmail.com>, Petr Jelinek <petr@2ndquadrant.com>,
	"michael.paul.powers@gmail.com" <michael.paul.powers@gmail.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15293: Stored Procedure Triggered by Logical Replication is
 Unable to use Notification Events
Message-ID: <20180724192740.6yn2xregaz6yhpc2@alap3.anarazel.de>
References: <153243441449.1404.2274116228506175596@wrigleys.postgresql.org>
 <45136061532438385@iva3-a2a8aac410a1.qloud-c.yandex.net>
 <20180724155829.zq22vov6d7erhxsx@alap3.anarazel.de>
 <CAEU75wAdO7_t6MzSqWxua4Cca+KKpuHYYz6txz=VgETX--yYvw@mail.gmail.com>
 <648481532456538@myt5-0fa3e541b8b9.qloud-c.yandex.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <648481532456538@myt5-0fa3e541b8b9.qloud-c.yandex.net>
Precedence: bulk

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