On Monday, February 20, 2023 8:47 PM Melih Mutlu <m.melihmu...@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 should be 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 during replication. If we want to add
this kind of check, then we can add checks for both "binary" and "copy_format"
option together 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
