Hi,
On 2019-01-10 15:56:18 -0500, Tom Lane wrote:
> 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.
Given there's been no movement since this review, I'm marking this patch
as returned with feedback. Please resubmit once updated.
Greetings,
Andres Freund
