Julien Demoor <jul...@jdemoor.com> writes:
> [ postgres-notify-all-v8.patch ]
I took a quick look through this. A few comments:
* I find the proposed syntax extension for NOTIFY to be fairly bizarre;
it's unlike the way that we handle options for any other utility
statement. It's also non-orthogonal, since you can't specify a collapse
mode without giving a payload string. I think possibly a better answer
is the way that we've been adding optional parameters to VACUUM and
suchlike recently:
NOTIFY [ (collapse = off/on) ] channel [ , payload ]
This'd be more extensible if we ever find a need for other options,
too.
* I'm also unimpressed with the idea of having a "maybe" collapse mode.
What's that supposed to do? It doesn't appear to be different from
"always", so why not just reduce this to a boolean collapse-or-not
flag?
* The documentation doesn't agree with the code, since it fails to
mention "always" mode.
* I was kind of disappointed to find that the patch doesn't really
do anything to fix the performance problem for duplicate notifies.
The thread title had led me to hope for more ;-). I wonder if we
couldn't do something involving hashing. OTOH, it's certainly
arguable that that would be an independent patch, and that this
one should be seen as a feature patch ("make NOTIFY's behavior
for duplicate notifies more flexible and more clearly specified")
rather than a performance patch.
regards, tom lane
