On 10/10/2018 19:42, Catalin Iacob wrote:
> On Tue, Oct 9, 2018 at 2:17 PM <julien@jdemoor.com> wrote:
>> I just caught an error in my patch, it's fixed in the attachment. The
>> 'never' and 'maybe' collapse modes were mixed up in one location.
>
> Here's a partial review of this version, did not read the doc part
> very carefully.
>
> First of all, I agree that this is a desirable feature as, for a large
> number of notiifications, the O(n^2) overhead quickly becomes very
> noticeable.
>
> I would expect the collapse mode to be an enum which is created from
> the string early on during parsing and used for the rest of the code.
> Instead the string is used all the way leading to string comparisons
> in the notification dispatcher and to the need of hardcoding special
> strings in various places, including the contrib module.
>
> This comment in the beginning of async.c should also be updated:
> * Duplicate notifications from the same transaction are sent out as one
> * notification only. This is done to save work when for example a trigger
>
> pg_notify_3args duplicates pg_notify, I would expect a helper function
> to be extracted and called from both.
>
> There are braces placed on the same line as the if, for example if
> (strlen(collapse_mode) != 0) { which seems to not be the project's
> style.
Thank you for the review. I've addressed all your points in the attached
patch. The patch was made against release 11.1.
I couldn't find a way to make a good helper function for pg_notify_3args
and pg_notify, I hope my proposed solution is acceptable.