Listen / Notify - what to do when the queue is full
in [PgSql]
|
From: Jeff Davis on 2 Feb 2010 20:05 On Wed, 2010-02-03 at 00:10 +0100, Joachim Wieland wrote: > I admit that it was not clear what I meant. The comment should only > address LISTEN / NOTIFY on the standby server. Do you see any problems > allowing it? The original comment was a part of the NotifyStmt case, and I don't think we can support NOTIFY issued on a standby system -- surely there's no way for the standby to communicate the notification to the master. Anyway, this is getting a little sidetracked; I don't think we need to worry about HS right now. > I was wondering if we should have a hard limit on the maximal number > of notifications per transaction. You can now easily fill up your > backend's memory with notifications. However we had the same problem > with the old implementation already and nobody complained. The > difference is just that now you can fill it up a lot faster because > you can send a large payload. I don't see a need for that. The only way that is likely to happen is with triggers, and there are other ways to fill up the memory using triggers. Even if the option is there, all we can do is abort. > The second doubt I had is about the truncation behavior of slru. ISTM > that it doesn't truncate at the end of the page range once the head > pointer has already wrapped around. > There is the following comment in slru.c describing this fact: > > /* > * While we are holding the lock, make an important safety check: the > * planned cutoff point must be <= the current endpoint page. Otherwise we > * have already wrapped around, and proceeding with the truncation would > * risk removing the current segment. > */ > > I wanted to check if we can do anything about it and if we need to do > anything at all... I'll have to take a look in more detail. > Now the question is, should we forbid NOTIFY for 2PC > altogether only because in the unlikely event of a full queue we > cannot guarantee that we can commit the transaction? I believe that was the consensus in the thread. The people who use 2PC are the people that want the most rock-solid guarantees. I don't like forbidding feature combinations, but I don't see a good alternative. > One solution is to treat a 2PC transaction like a backend with its own > pointer to the queue. As long as the prepared transaction is not > committed, its pointer does not move and so we don't move forward the > global tail pointer. Here the NOTIFYs sent by the 2PC transaction are > already in the queue and the transaction can always commit. The > drawback of this idea is that if you forget to commit the prepared > transaction and leave it around uncommitted, your queue will fill up > inevitably because you do not truncate anymore... There's also a problem if the power goes out, you restart, and then the queue fills up before you COMMIT PREPARED. > > * There's a bug where an UNLISTEN can abort, and yet you still miss > > the notification. > > [...] > > The notification is missed. It's fixed easily enough by doing the > > UNLISTEN step in AtCommit_NotifyAfterCommit. > > Thanks, very well spotted... Actually the same is true for LISTEN... I > have reworked the patch to do the changes to listenChannels only in > the post-commit functions. I'm worried that this creates the opposite problem: that a LISTEN transaction might commit before a NOTIFY transaction, and yet miss the notification. It seems safest to me to add a backend (LISTEN) to the list before commit, and remove a backend (UNLISTEN) after commit. That way we are sure to only receive spurious notifications, and can't miss any. > I have also included Arnaud Betremieux's send_notify function. Should > we really call the function send_notify? What about > pg_send_notification or just pg_notify? Agreed: pg_notify sounds good to me. > Is [void] the preferred return type? I can't think of anything else that it might return. NOTIFY doesn't return much ;) Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Joachim Wieland on 3 Feb 2010 04:34 On Wed, Feb 3, 2010 at 2:05 AM, Jeff Davis <pgsql(a)j-davis.com> wrote: >> Thanks, very well spotted... Actually the same is true for LISTEN... I >> have reworked the patch to do the changes to listenChannels only in >> the post-commit functions. > > I'm worried that this creates the opposite problem: that a LISTEN > transaction might commit before a NOTIFY transaction, and yet miss the > notification. See the following comment and let me know if you agree... ! /* ! * Exec_ListenBeforeCommit --- subroutine for AtCommit_NotifyBeforeCommit ! * ! * Note that we do only set our pointer here and do not yet add the channel to ! * listenChannels. Since our transaction could still roll back we do this only ! * after commit. We know that our tail pointer won't move between here and ! * directly after commit, so we won't miss a notification. ! */ However this introduces a new problem when an initial LISTEN aborts: Then we are not listening to anything but for other backends it looks like we were. This is tracked by the boolean variable backendExecutesInitialListen and gets cleaned up in AtAbort_Notify(). > It seems safest to me to add a backend (LISTEN) to the list before > commit, and remove a backend (UNLISTEN) after commit. That way we are > sure to only receive spurious notifications, and can't miss any. If a LISTEN aborted we would not only receive a few spurious notifications from it but would receive notifications on this channel forever even though we have never executed LISTEN on it successfully. Joachim -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 7 Feb 2010 00:18 On Wed, Feb 3, 2010 at 4:34 AM, Joachim Wieland <joe(a)mcknight.de> wrote: > On Wed, Feb 3, 2010 at 2:05 AM, Jeff Davis <pgsql(a)j-davis.com> wrote: >>> Thanks, very well spotted... Actually the same is true for LISTEN... I >>> have reworked the patch to do the changes to listenChannels only in >>> the post-commit functions. >> >> I'm worried that this creates the opposite problem: that a LISTEN >> transaction might commit before a NOTIFY transaction, and yet miss the >> notification. > > See the following comment and let me know if you agree... > > ! /* > ! * Exec_ListenBeforeCommit --- subroutine for AtCommit_NotifyBeforeCommit > ! * > ! * Note that we do only set our pointer here and do not yet add the channel to > ! * listenChannels. Since our transaction could still roll back we do this only > ! * after commit. We know that our tail pointer won't move between here and > ! * directly after commit, so we won't miss a notification. > ! */ > > However this introduces a new problem when an initial LISTEN aborts: > Then we are not listening to anything but for other backends it looks > like we were. This is tracked by the boolean variable > backendExecutesInitialListen and gets cleaned up in AtAbort_Notify(). > > >> It seems safest to me to add a backend (LISTEN) to the list before >> commit, and remove a backend (UNLISTEN) after commit. That way we are >> sure to only receive spurious notifications, and can't miss any. > > If a LISTEN aborted we would not only receive a few spurious > notifications from it but would receive notifications on this channel > forever even though we have never executed LISTEN on it successfully. Jeff, do you think this patch is ready for committer? If so, please mark it as such on commitfest.postgresql.org - otherwise, please clarify what you think the action items are. Thanks, ....Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Jeff Davis on 7 Feb 2010 01:44 On Sun, 2010-02-07 at 00:18 -0500, Robert Haas wrote: > Jeff, do you think this patch is ready for committer? If so, please > mark it as such on commitfest.postgresql.org - otherwise, please > clarify what you think the action items are. I'll post an update tomorrow. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Alvaro Herrera on 8 Feb 2010 11:16
Joachim Wieland wrote: > + typedef struct AsyncQueueEntry > + { > + /* > + * this record has the maximal length, but usually we limit it to > + * AsyncQueueEntryEmptySize + strlen(payload). > + */ > + Size length; > + Oid dboid; > + TransactionId xid; > + int32 srcPid; > + char channel[NAMEDATALEN]; > + char payload[NOTIFY_PAYLOAD_MAX_LENGTH]; > + } AsyncQueueEntry; > + #define AsyncQueueEntryEmptySize \ > + (sizeof(AsyncQueueEntry) - NOTIFY_PAYLOAD_MAX_LENGTH + 1) These are the on-disk notifications, right? It seems to me a bit wasteful to store channel name always as NAMEDATALEN bytes. Can we truncate it at its strlen? I realize that this would cause the struct definition to be uglier (you will no longer be able to have both channel and payload pointers, only a char[1] pointer to a data area to which you write both). Typical channel names should be short, so IMHO this is worthwhile. Besides, I think the uglification of code this causes should be fairly contained ... -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |