On Monday, February 20, 2023 8:47 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> Thanks for letting me know.
> Attached the fixed version of the patch.
Hi, Melih
Thanks for updating the patch. Minor comments on v9.
(1) commit message
"The patch introduces a new parameter, copy_format, to CREATE SUBSCRIPTION to allow to choose
the format used in initial table synchronization."
This patch introduces the new parameter not only to CREATE SUBSCRIPTION and ALTER SUBSCRIPTION, then this description
shouldbe more general, something like below.
"The patch introduces a new parameter, copy_format, as subscription option to
allow user to choose the format of initial table synchronization."
(2) copy_table
We don't need to check the publisher's version below.
+
+ /* If the publisher is v16 or later, specify the format to copy data. */
+ if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 160000)
+ {
+ char *format = MySubscription->copyformat == LOGICALREP_COPY_AS_BINARY ? "binary" : "text";
+ appendStringInfo(&cmd, " WITH (FORMAT %s)", format);
+ options = lappend(options, makeDefElem("format", (Node *) makeString(format), -1));
+ }
+
We don't have this kind of check for "binary" option and it seems this is user's responsibility to avoid any errors
duringreplication. If we want to add this kind of check, then we can add checks for both "binary" and "copy_format"
optiontogether as an independent patch.
(3) subscription.sql/out
The other existing other subscription options check the invalid input for newly created option (e.g. "foo" for
disable_on_error, streaming mode). So, I think we can add this type of test for this feature.
Best Regards,
Takamichi Osumi
