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. -- With Regards, Amit Kapila.