On Thu, Aug 19, 2021 at 3:51 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Aug 16, 2021 at 8:33 AM Greg Nancarrow <gregn4...@gmail.com> wrote: > > > > On Mon, Aug 16, 2021 at 6:24 AM Masahiko Sawada <sawada.m...@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/
