On Mon, Aug 16, 2021 at 8:33 AM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Aug 16, 2021 at 6:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Therefore, perhaps a message like "... in transaction 740 with commit
> > timestamp 2021-08-10 14:44:38.058174+05:30" is better in terms of
> > consistency with other messages?
> >
>
> Yes, I think that would be more consistent.
>
> On another note, for the 0001 patch, the elog ERROR at the bottom of
> the logicalrep_message_type() function seems to assume that the
> unrecognized "action" is a printable character (with its use of %c)
> and also that the character is meaningful to the user in some way.
> But given that the compiler normally warns of an unhandled enum value
> when switching on an enum, such an error would most likely be when
> action is some int value that wouldn't be meaningful to the user (as
> it wouldn't be one of the LogicalRepMsgType enum values).
> I therefore think it would be better to use %d in that ERROR:
>
> i.e.
>
> + elog(ERROR, "invalid logical replication message type %d", action);
>
> Similar comments apply to the apply_dispatch() function (and I realise
> it used %c before your patch).
>
The action in apply_dispatch is always a single byte so not sure why
we need %d here. Also, if it is used as %c before the patch then I
think it is better not to change it in this patch.
--
With Regards,
Amit Kapila.
