Re: Enumize logical replication message actions

On Fri, Oct 30, 2020 at 5:05 PM Ashutosh Bapat
<ashutosh.bapat@2ndquadrant.com> wrote:
>
>
>
> On Fri, 30 Oct 2020 at 09:16, Amit Kapila <amit.kapila16@gmail.com> wrote
>>
>> I think we can simply use 'return apply_handle_begin;' instead of
>> adding return in another line. Again, I think we changed this handling
>> in apply_dispatch() to improve the case where we can detect at the
>> compile time any missing enum but at this stage it is not clear to me
>> if that is true.
>
>
> I don't see much value in writing it like "return apply_handle_begin()"; gives an impression that apply_handle_begin() and apply_dispatch() are returning something which they are not. I would prefer return on separate line unless there's something more than style improvement.
>

Fair enough.

> I have added rationale behind Enum in the commit message as you suggested in one of the later mails.
>
> PFA patch addressing your comments.
>

I don't like the word 'Enumize' in commit message. How about changing
it to something like: (a) Add defines for logical replication protocol
messages, or (b) Associate names with logical replication protocol
messages.

+ 2. It's easy to locate the code handling a given type.

In the above instead of 'type', shouldn't it be 'message'.

Other than that the patch looks good to me.

-- 
With Regards,
Amit Kapila.