Re: DISCARD ALL failing to acquire locks on pg_listen

Matteo Beccati <php(at)beccati(dot)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);