Re: Enumize logical replication message actions

Thanks Andres for your review. Thanks Li, Horiguchi-san and Amit for your
comments.

On Tue, 20 Oct 2020 at 04:57, Andres Freund <andres(at)anarazel(dot)de> wrote:

> Hi,
>
> On 2020-10-16 12:55:26 +0530, Ashutosh Bapat wrote:
> > Here's a patch simplifying that for top level logical replication
> > messages.
>
> I think that's a good plan. One big benefit for me is that it's much
> easier to search for an enum than for a single letter
> constant. Including searching for all the places that deal with any sort
> of logical rep message type.

>
> > void
> > logicalrep_write_begin(StringInfo out, ReorderBufferTXN *txn)
> > {
> > - pq_sendbyte(out, 'B'); /* BEGIN */
> > + pq_sendbyte(out, LOGICAL_REP_MSG_BEGIN); /* BEGIN */
>
> I think if we have the LOGICAL_REP_MSG_BEGIN we don't need the /* BEGIN */.
>

Yes. Fixed all places.

I have attached two places - 0001 which is previous 0001 patch with your
comments addressed.

0002 adds wrappers on top of pq_sendbyte() and pq_getmsgbyte() to send and
receive a logical replication message type respectively. These wrappers add
more protection to make sure that the enum definitions fit one byte. This
also removes the default case from apply_dispatch() so that we can detect
any LogicalRepMsgType not handled by that function.

These two patches are intended to be committed together as a single commit.
For now the second one is separate so that it's easy to remove the changes
if they are not acceptable.

--
Best Wishes,
Ashutosh