Re: Allow logical replication to copy tables in binary format

On Thu, Mar 16, 2023 at 6:59 PM Melih Mutlu <m(dot)melihmutlu(at)gmail(dot)com> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, 16 Mar 2023 Per, 06:25 tarihinde şunu yazdı:
>>
>> On Thu, Mar 16, 2023 at 8:27 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>> >
>> > On Wed, Mar 8, 2023, at 11:50 PM, Amit Kapila wrote:
>> >
>> > It is not clear to me which version check you wanted to add because we
>> > seem to have a binary option in COPY from the time prior to logical
>> > replication. I feel we need a publisher version 14 check as that is
>> > where we start to support binary mode transfer in logical replication.
>> > See the check in function libpqrcv_startstreaming().
>> >
>> > ... then you are breaking existent cases. Even if you have a convincing
>> > argument, you are introducing a behavior change in prior versions (commit
>> > messages should always indicate that you are breaking backward compatibility).
>> >
>> > +
>> > + /*
>> > + * The binary option for replication is supported since v14
>> > + */
>> > + if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 140000 &&
>> > + MySubscription->binary)
>> > + {
>> > + appendStringInfo(&cmd, " WITH (FORMAT binary)");
>> > + options = lappend(options, makeDefElem("format", (Node *) makeString("binary"), -1));
>> > + }
>> > +
>> >
>> > What are the arguments to support since v14 instead of the to-be-released
>> > version? I read the thread but it is not clear. It was said about the
>> > restrictive nature of this feature and it will be frustrating to see that the
>> > same setup (with the same commands) works with v14 and v15 but it doesn't with
>> > v16.
>> >
>>
>> If the failure has to happen it will anyway happen later when the
>> publisher will be upgraded to v16. The reason for the version checks
>> as v14 was to allow the initial sync from the same version where the
>> binary mode for replication was introduced. However, if we expect
>> failures in the existing setup, I am fine with supporting this for >=
>> v16.
>
>
> Upgrading the subscriber to v16 and keeping the subscriber in v14 could break existing subscriptions. I don't know how likely such a case is.
>
> I don't have a strong preference on this. What do you think? Should we change it >=v16 or keep it as it is?
>

I think to reduce the risk of breakage, let's change the check to
>=v16. Also, accordingly, update the doc and commit message.

--
With Regards,
Amit Kapila.