Re: Listen / Notify - what to do when the queue is full

On Sat, Jan 30, 2010 at 12:02 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> Comments:
>
> * In standard_ProcessUtility(), case NotifyStmt, you add a comment:
>
>    /* XXX the new listen/notify version can be enabled
>     * for Hot Standby */
>
>  but I don't think that's correct. We may be able to support LISTEN
>  on the standby, but not NOTIFY (right?). I don't think we should
>  be adding speculative comments anyway, because there is some work
>  still needed before HS can support LISTEN (notably, WAL-logging
>  NOTIFY).

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? Of course it's not all that useful because all
transactions are read-only but it still allows different clients to
communicate.
Of course listening on the standby server to notifications from the
primary server is not possible currently but in my point of view this
is a completely new use case for LISTEN/NOTIFY as it is not local but
across servers.

> * You have a TODO list as a comment. Can you remove it and explain
>  those items on list if they aren't already?

Sure.

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.

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...

> * You have the comment:
>  That should be written more simply [...]

done

> * You have the Assert:

done

> * ASCII-only is still an open issue.

still an open issue...

> * 2PC is still an open issue (notifications are lost on restart,
>  and there may be other problems, as well). I think it's easy enough
>  to throw an error on PREPARE TRANSACTION if there are any
>  notifications, right?

we could forbid them, yes. Notifications aren't lost on restart
however, they get recorded in the 2PC state files. Currently the patch
works fine with 2PC (including server restart) with the exception that
in the queue full case we might need to roll back the prepared
transaction. 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?

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 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 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? I am not a native English
speaker but to me it sounds strange to send a verb (notify) and I'd
rather prefix the name with pg_...?
Currently the function always returns void. Is this the preferred return type?

Joachim