On Thu, Aug 19, 2021 at 3:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
Yes, I agree that it's better no to change it in this patch since %c
is used before the patch. Also I can see some error messages in
walsender.c also use %c. If we conclude that it should use %d instead
of %c, we can change all of them as another patch.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
