On Thu, 22 Oct 2020 at 14:46, Kyotaro Horiguchi <horikyota....@gmail.com> wrote:
> > > 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. > Enum is an integer, and we want to send byte. The function asserts that the enum fits a byte. If there's a way to declare byte long enums I would use that. But I didn't find a way to do that. 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()? > > The second patch removes the default case you quoted above. I think that's important to detect any unhandled case at compile time rather than at run time. But we need some way to detect whether the values we get from wire are legit. pg_get_logicalrep_msg_type() does that. Further that function can be used at places other than apply_dispatch() if required without each of those places having their own validation. -- Best Wishes, Ashutosh