On Thu, Mar 16, 2023 at 5:59 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> 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.
>I find this needlessly verbose. > ~~ > > Anyway, the shortened comment as in the latest v15 patch is fine by me too. > Okay, then let's go with that. -- With Regards, Amit Kapila.
