Re: Enumize logical replication message actions

At Fri, 23 Oct 2020 10:08:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> At Thu, 22 Oct 2020 16:37:18 +0530, Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com> wrote in 
> > On Thu, 22 Oct 2020 at 14:46, Kyotaro Horiguchi <horikyota.ntt@gmail.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