Re: Enumize logical replication message actions

At Fri, 23 Oct 2020 10:08:44 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Thu, 22 Oct 2020 16:37:18 +0530, Ashutosh Bapat <ashutosh(dot)bapat(at)2ndquadrant(dot)com> wrote in
> > On Thu, 22 Oct 2020 at 14:46, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> > wrote:
> > 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.
>
> Even if that enum contains out-of-range values, that "command" is sent
> having truncated to uint8 and on the receiver side apply_dispatch()
> doesn't identify the command and raises an error. That is equivalent
> to what pq_send_logicalrep_msg_type() does. (Also equivalent on the
> point that symbols that are not used in regression are not checked.)

Sorry, this is about pg_send_logicalrep_msg_type(), not
pg_get..(). And I forgot to mention pg_get_logicalrep_msg_type().

For the pg_get_logicalrep_msg_type(), It is just a repetion of what
apply_displatch() does in switch().

If I flattened the code, it looks like:

apply_dispatch(s)
{
LogicalRepMsgType msgtype = pq_getmsgtype(s);
bool pass = false;

switch (msgtype)
{
case LOGICAL_REP_MSG_BEGIN:
...
case LOGICAL_REP_MSG_STREAM_COMMIT:
pass = true;
}
if (!pass)
ereport(ERROR, (errmsg("invalid logical replication message type"..

switch (msgtype)
{
case LOGICAL_REP_MSG_BEGIN:
apply_handle_begin();
break;
...
case LOGICAL_REP_MSG_STREAM_COMMIT:
apply_handle_begin();
break;
}
}

Those two switch()es are apparently redundant. That code is exactly
equivalent to:

apply_dispatch(s)
{
LogicalRepMsgType msgtype = pq_getmsgtype(s);

switch (msgtype)
{
case LOGICAL_REP_MSG_BEGIN:
apply_handle_begin();
! return;
...
case LOGICAL_REP_MSG_STREAM_COMMIT:
apply_handle_begin();
! return;
}

ereport(ERROR, (errmsg("invalid logical replication message type"..
}

which is smaller and fast.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center