On Thu, Mar 16, 2023 at 8:27 AM Euler Taveira <euler@eulerto.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.
> IMO it should be >= 16 and documentation should explain that v14/v15 uses
> text format during initial table synchronization even if binary = true.
>
Yeah, if we change the version then the corresponding text in the
patch should also be changed.
> Should there be a fallback mode (text) if initial table synchronization failed
> because of the binary option? Maybe a different setting (auto) to support such
> behavior.
>
I think the workaround is that the user disables binary mode for the
time of initial sync. I think if we want to extend and add a fallback
(text) mode then it is better to keep it as default behavior rather
than introducing a new setting like 'auto'. Personally, I feel it can
be added later after doing some more study.
--
With Regards,
Amit Kapila.