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
thinkwe should be adding speculative comments anyway, because there is some work still needed before HS can support
LISTEN(notably, WAL-logging NOTIFY).
* You have a TODO list as a comment. Can you remove it and explain those items on list if they aren't already?
* You have the comment:
/* * How long can a payload string possibly be? Actually it needs to be one * byte less to provide space
forthe trailing terminating '\0'. */
That should be written more simply, like "Maximum size of the payload, including terminating NULL."
* You have the Assert:
Assert(strlen(n->payload) <= NOTIFY_PAYLOAD_MAX_LENGTH);
which is inconsistent with the earlier test:
if (stmt->payload && strlen(stmt->payload) > NOTIFY_PAYLOAD_MAX_LENGTH - 1) ereport(ERROR, ...
* ASCII-only is 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'seasy enough to throw an error on PREPARE TRANSACTION if there are any notifications, right?
* There's a bug where an UNLISTEN can abort, and yet you still miss the notification. This is because you unlisten
beforeyou actually commit the transaction, and an error between those times will cause the UNLISTEN to take effect
eventhough the rest of the transaction fails. For example:
-- session 1 LISTEN foo; BEGIN; UNLISTEN foo;
-- session 2 NOTIFY foo;
-- gdb in session 1 (gdb) break AtCommit_NotifyBeforeCommit (gdb) c
-- session 1 COMMIT;
-- gdb in session 1 (gdb) finish (gdb) p op_strict(7654322) (gdb) quit
The notification is missed. It's fixed easily enough by doing the UNLISTEN step in AtCommit_NotifyAfterCommit.
I'm still looking through some of the queue stuff, and I'll send an
update soon. I wanted to give you some feedback now though.
Regards,Jeff Davis
