At Thu, 22 Oct 2020 12:13:40 +0530, Ashutosh Bapat <ashutosh.bapat@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 <andres@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
