On Wed, Mar 15, 2023 at 5:31 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Wed, Mar 15, 2023 at 11:52 AM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > ======
> > src/backend/replication/logical/tablesync.c
> >
> > 5.
> > +
> > + /*
> > + * If the publisher version is earlier than v14, it COPY command doesn't
> > + * support the binary option.
> > + */
> > + if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 140000 &&
> > + MySubscription->binary)
> > + {
> > + appendStringInfo(&cmd, " WITH (FORMAT binary)");
> > + options = lappend(options, makeDefElem("format", (Node *)
> > makeString("binary"), -1));
> > + }
> >
> > Sorry, I gave a poor review comment for this previously. Now I have
> > revisited all the thread discussions about version checking. I feel
> > that some explanation should be given in the code comment so that
> > future readers of this code can understand why you decided to use v14
> > checking.
> >
> > Something like this:
> >
> > SUGGESTION
> > If the publisher version is earlier than v14, we use text format COPY.
> >
>
> I think this isn't explicit that we supported the binary format since
> v14. So, I would prefer my version of the comment as suggested in the
> previous email.
>Hmm, but my *full* suggestion was bigger than what is misquoted above, and it certainly did say " logical replication binary mode transfer was not introduced until v14". SUGGESTION If the publisher version is earlier than v14, we use text format COPY. Note - In fact COPY syntax "WITH (FORMAT binary)" has existed since v9, but since the logical replication binary mode transfer was not introduced until v14 it was decided to check using the later version. ~~ Anyway, the shortened comment as in the latest v15 patch is fine by me too. ------ Kind Regards, Peter Smith. Fujitsu Australia
