At Thu, 22 Oct 2020 12:13:40 +0530, Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com> wrote in > Thanks Andres for your review. Thanks Li, Horiguchi-san and Amit for your > comments. > > On Tue, 20 Oct 2020 at 04:57, Andres Freund <and...@anarazel.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.
We shouldn't have the default: in the switch() block in apply_dispatch(). That prevents compilers from checking completeness. The content of the default: should be moved out to after the switch() block. apply_dispatch() { switch (action) { .... case LOGICAL_REP_MSG_STREAM_COMMIT(s); apply_handle_stream_commit(s); return; } ereport(ERROR, ...); } > 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. pg_send_logicalrep_msg_type() looks somewhat too-much. If we need something like that we shouldn't do this refactoring, I think. pg_get_logicalrep_msg_type() seems doing the same check (that the value is compared aganst every keyword value) with apply_dispatch(). Why do we need that function separately from apply_dispatch()? > 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. regards. -- Kyotaro Horiguchi NTT Open Source Software Center