On Monday, January 30, 2023 7:50 PM Melih Mutlu <m.melihmu...@gmail.com> wrote:
> Thanks for reviewing.
...
> Please see [1] and you'll get the following error in your case:
> "ERROR: incorrect binary data format in logical replication column 1"
Hi, thanks for sharing v7.
(1) general comment
I wondered if the addition of the new option/parameter can introduce some
confusion to the users.
case 1. When binary = true and copy_format = text, the table sync is conducted
by text.
case 2. When binary = false and copy_format = binary, the table sync is
conducted by binary.
(Note that the case 2 can be accepted after addressing the 3rd comment of
Bharath-san in [1].
I agree with the 3rd comment by itself.)
The name of the new subscription parameter looks confusing.
How about "init_sync_format" or something ?
(2) The commit message doesn't get updated.
The commit message needs to mention the new subscription option.
(3) whitespace errors.
$ git am v7-0001-Allow-logical-replication-to-copy-table-in-binary.patch
Applying: Allow logical replication to copy table in binary
.git/rebase-apply/patch:95: trailing whitespace.
copied to the subscriber. Supported formats are
.git/rebase-apply/patch:101: trailing whitespace.
that data will not be copied if <literal>copy_data = false</literal>.
warning: 2 lines add whitespace errors.
(4) pg_dump.c
if (fout->remoteVersion >= 160000)
- appendPQExpBufferStr(query, " s.suborigin\n");
+ appendPQExpBufferStr(query, " s.suborigin,\n");
else
- appendPQExpBuffer(query, " '%s' AS suborigin\n",
LOGICALREP_ORIGIN_ANY);
+ appendPQExpBuffer(query, " '%s' AS suborigin,\n",
LOGICALREP_ORIGIN_ANY);
+
+ if (fout->remoteVersion >= 160000)
+ appendPQExpBufferStr(query, " s.subcopyformat\n");
+ else
+ appendPQExpBuffer(query, " '%c' AS subcopyformat\n",
LOGICALREP_COPY_AS_TEXT);
This new branch for v16 can be made together with the previous same condition.
(5) describe.c
+
+ /* Copy format is only supported in v16 and higher */
+ if (pset.sversion >= 160000)
+ appendPQExpBuffer(&buf,
+ ", subcopyformat AS
\"%s\"\n",
+ gettext_noop("Copy
Format"));
Similarly to (4), this creates a new branch for v16. Please see the above codes
of this part.
(6)
+ * Extract the copy format value from a DefElem.
+ */
+char
+defGetCopyFormat(DefElem *def)
Shouldn't this function be static and remove the change of subscriptioncmds.h ?
(7) catalogs.sgml
The subcopyformat should be mentioned here and the current description for
subbinary
should be removed.
(8) create_subscription.sgml
+ <literal>text</literal>.
+
+ <literal>binary</literal> format can be selected only if
Unnecessary blank line.
[1] -
https://www.postgresql.org/message-id/CALj2ACW5Oa7_v25iZb326UXvtM_tjQfw0Tc3hPJ8zN4FZqc9cw%40mail.gmail.com
Best Regards,
Takamichi Osumi
