On Fri, Jul 1, 2022 at 12:13 PM Peter Smith <smithpb2...@gmail.com> wrote: > > ====== > > 1.2 doc/src/sgml/protocol.sgml - Protocol constants > > Previously I wrote that since there are protocol changes here, > shouldn’t there also be some corresponding LOGICALREP_PROTO_XXX > constants and special checking added in the worker.c? > > But you said [1 comment #6] you think it is OK because... > > IMO, I still disagree with the reply. The fact is that the protocol > *has* been changed, so IIUC that is precisely the reason for having > those protocol constants. > > e.g I am guessing you might assign the new one somewhere here: > -- > server_version = walrcv_server_version(LogRepWorkerWalRcvConn); > options.proto.logical.proto_version = > server_version >= 150000 ? LOGICALREP_PROTO_TWOPHASE_VERSION_NUM : > server_version >= 140000 ? LOGICALREP_PROTO_STREAM_VERSION_NUM : > LOGICALREP_PROTO_VERSION_NUM; > -- > > And then later you would refer to this new protocol version (instead > of the server version) when calling to the apply_handle_stream_abort > function. > > ====== >
One point related to this that occurred to me is how it will behave if the publisher is of version >=16 whereas the subscriber is of versions <=15? Won't in that case publisher sends the new fields but subscribers won't be reading those which may cause some problems. > ====== > > 1.5 src/backend/commands/subscriptioncmds.c > > + /* > + * If no parameter given, assume "true" is meant. > + */ > > Previously I suggested an update for this comment, but it was rejected > [1 comment #12] saying you wanted consistency with defGetBoolean. > > Sure, that is one point of view. Another one is that "two wrongs don't > make a right". IIUC that comment as it currently stands is incorrect > because in this case there *is* a parameter given - it is just the > parameter *value* that is missing. > You have a point but if we see this function in the vicinity then the proposed comment also makes sense. -- With Regards, Amit Kapila.
