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

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