Re: Enumize logical replication message actions

On Thu, 22 Oct 2020 at 14:46, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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