Re: DISCARD ALL failing to acquire locks on pg_listen

Matteo Beccati <php@beccati.com> writes:
> Tom Lane ha scritto:
>> This seems a bit overcomplicated.  I had in mind something like this...

> Much easier indeed... I didn't notice the unlistenExitRegistered variable.

Just for completeness, I attach another form of the patch that I thought
about for a bit.  This adds the ability for UNLISTEN ALL to revert the
backend to the state where subsequent UNLISTENs don't cost anything.
This could be of value in a scenario where you have pooled connections
and just a small fraction of the client threads are using LISTEN.  That
seemed like kind of an unlikely use-case though.  The problem is that
this patch adds some cycles to transaction commit/abort for everyone,
whether they ever use LISTEN/UNLISTEN/DISCARD or not.  It's not a lot of
cycles, but even so I'm thinking it's not a win overall.  Comments?
        regards, tom lane

Index: src/backend/access/transam/xact.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.272
diff -c -r1.272 xact.c
*** src/backend/access/transam/xact.c    20 Jan 2009 18:59:37 -0000    1.272
--- src/backend/access/transam/xact.c    12 Feb 2009 18:24:12 -0000
***************
*** 1703,1708 ****
--- 1703,1709 ----     AtEOXact_SPI(true);     AtEOXact_xml();     AtEOXact_on_commit_actions(true);
+     AtEOXact_Notify(true);     AtEOXact_Namespace(true);     /* smgrcommit already done */     AtEOXact_Files();
***************
*** 1939,1944 ****
--- 1940,1946 ----     AtEOXact_SPI(true);     AtEOXact_xml();     AtEOXact_on_commit_actions(true);
+     AtEOXact_Notify(true);     AtEOXact_Namespace(true);     /* smgrcommit already done */     AtEOXact_Files();
***************
*** 2084,2089 ****
--- 2086,2092 ----     AtEOXact_SPI(false);     AtEOXact_xml();     AtEOXact_on_commit_actions(false);
+     AtEOXact_Notify(false);     AtEOXact_Namespace(false);     AtEOXact_Files();     AtEOXact_ComboCid();
Index: src/backend/commands/async.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/async.c,v
retrieving revision 1.145
diff -c -r1.145 async.c
*** src/backend/commands/async.c    1 Jan 2009 17:23:37 -0000    1.145
--- src/backend/commands/async.c    12 Feb 2009 18:24:13 -0000
***************
*** 167,172 ****
--- 167,178 ---- /* True if we've registered an on_shmem_exit cleanup */ static bool unlistenExitRegistered = false; 
+ /* True if this backend has (or might have) an active LISTEN entry */
+ static bool haveActiveListen = false;
+ 
+ /* True if current transaction is trying to commit an UNLISTEN ALL */
+ static bool committingUnlistenAll = false;
+  bool        Trace_notify = false;  
***************
*** 277,282 ****
--- 283,292 ----     if (Trace_notify)         elog(DEBUG1, "Async_Unlisten(%s,%d)", relname, MyProcPid); 
+     /* If we couldn't possibly be listening, no need to queue anything */
+     if (pendingActions == NIL && !haveActiveListen)
+         return;
+      queue_listen(LISTEN_UNLISTEN, relname); } 
***************
*** 291,296 ****
--- 301,310 ----     if (Trace_notify)         elog(DEBUG1, "Async_UnlistenAll(%d)", MyProcPid); 
+     /* If we couldn't possibly be listening, no need to queue anything */
+     if (pendingActions == NIL && !haveActiveListen)
+         return;
+      queue_listen(LISTEN_UNLISTEN_ALL, ""); } 
***************
*** 493,499 ****     heap_freetuple(tuple);      /*
!      * now that we are listening, make sure we will unlisten before dying.      */     if (!unlistenExitRegistered)
 {
 
--- 507,526 ----     heap_freetuple(tuple);      /*
!      * Remember that this backend has at least one active LISTEN.  Also,
!      * this LISTEN negates the effect of any earlier UNLISTEN ALL in the
!      * same transaction.
!      *
!      * Note: it's still possible for the current transaction to fail before
!      * we reach commit.  In that case haveActiveListen might be uselessly
!      * left true; but that's OK, if not optimal, so we don't expend extra
!      * effort to cover that corner case.
!      */
!     haveActiveListen = true;
!     committingUnlistenAll = false;
! 
!     /*
!      * Now that we are listening, make sure we will unlisten before dying.      */     if (!unlistenExitRegistered)
 {
 
***************
*** 569,574 ****
--- 596,608 ----         simple_heap_delete(lRel, &lTuple->t_self);      heap_endscan(scan);
+ 
+     /*
+      * Remember that we're trying to commit UNLISTEN ALL.  Since we might
+      * still fail before reaching commit, we can't reset haveActiveListen
+      * immediately.
+      */
+     committingUnlistenAll = true; }  /*
***************
*** 675,680 ****
--- 709,730 ---- }  /*
+  * AtEOXact_Notify
+  *
+  *        Clean up post-commit or post-abort.  This is not called until
+  *        we know that we successfully committed (or didn't).
+  */
+ void
+ AtEOXact_Notify(bool isCommit)
+ {
+     /* If we committed UNLISTEN ALL, we can reset haveActiveListen */
+     if (isCommit && committingUnlistenAll)
+         haveActiveListen = false;
+     /* In any case, the next xact starts with clean UNLISTEN ALL slate */
+     committingUnlistenAll = false;
+ }
+ 
+ /*  * AtSubStart_Notify() --- Take care of subtransaction start.  *  * Push empty state for the new subtransaction.
Index: src/include/commands/async.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/async.h,v
retrieving revision 1.37
diff -c -r1.37 async.h
*** src/include/commands/async.h    1 Jan 2009 17:23:58 -0000    1.37
--- src/include/commands/async.h    12 Feb 2009 18:24:13 -0000
***************
*** 28,33 ****
--- 28,34 ---- extern void AtSubCommit_Notify(void); extern void AtSubAbort_Notify(void); extern void
AtPrepare_Notify(void);
+ extern void AtEOXact_Notify(bool isCommit);  /* signal handler for inbound notifies (SIGUSR2) */ extern void
NotifyInterruptHandler(SIGNAL_ARGS);