On Thu, 2010-02-11 at 00:52 +0100, Joachim Wieland wrote: > On Mon, Feb 8, 2010 at 5:16 PM, Alvaro Herrera > <alvhe...@commandprompt.com> wrote: > > 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? > > Attached is a new and hopefully more or less final patch for LISTEN / NOTIFY. > > The following items have been addressed in this patch: > > - only store strlen(channel) instead of NAMEDATALEN bytes on disk > - limit to 7-bit ASCII > - forbid 2PC and LISTEN/NOTIFY for now > - documentation changes > - add missing tab completion for NOTIFY > - fix pg_notify() behavior with respect to NULLs, too long and too > short parameters > - rebased to current HEAD, OID conflicts resolved
Some minor review comments without having taken in much of previous discussion: * Patch doesn't apply cleanly anymore, close. * In async.c you say "new async notification model". Please say "async notification model in 9.0 is". In (2) you say there is a central queue, but don't describe purpose of queue or what it contains. Some other typos in header comments. * There is no mention of what to do with pg_notify at checkpoint. Look at how pg_subtrans handles this. Should pg_notify do the same? * Is there a lazy backend avoidance scheme as exists for relcache invalidation messages? see storage/ipc/sinval... OK, I see asyncQueueFillWarning() but nowhere else says it exists and there aren't any comments in it to say what it does or why * What do you expect the DBA to do when they receive a queue fill warning? Where is that written down? * Not clear of the purpose of backendSendsNotifications. In AtCommit_NotifyAfterCommit() the logic seems strange. Code is /* Allow transactions that have not executed LISTEN/UNLISTEN/NOTIFY to * return as soon as possible */ if (!pendingActions && !backendSendsNotifications) return; but since backendSendsNotifications is set true earlier there is no fast path. * AtSubCommit_Notify doesn't seem to have a fastpath when no notify commands have been executed. * In Send_Notify() you throw ERROR while still holding the lock. It seems better practice to drop the lock then ERROR. * Why is Send_Notify() a separate function? It's only called from one point in the code. * We know that sometimes a FATAL error can occur without properly processing the abort. Do we depend upon this never happening? * Can we make NUM_ASYNC_BUFFERS = 8? 4 just seems too small * backendSendsNotifications is future tense. The variable should be called something like hasSentNotifications. Couple of other variables with similar names Hope that helps -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers