On Wednesday, January 11, 2023 7:45 PM Melih Mutlu <m.melihmutlu@gmail.com> wrote:
> Thanks for your review.
> Done.
Hi, minor comments on v5.
(1) publisher's version check
+ /* If the publisher is v16 or later, copy in binary format.*/
+ server_version = walrcv_server_version(LogRepWorkerWalRcvConn);
+ if (server_version >=160000 && MySubscription->binary)
+ {
+ appendStringInfoString(&cmd, " WITH (FORMAT binary)");
+ options = lappend(options, makeDefElem("format", (Node *) makeString("binary"), -1));
+ }
+
+ elog(LOG, "version: %i, %s", server_version, cmd.data);
(1-1) There is no need to log the version and the query by elog here.
(1-2) Also, I suggest we can remove the server_version variable itself,
because we have only one actual reference for it.
There is a style that we call walrcv_server_version in the
'if condition' directly like existing codes in fetch_remote_table_info().
(1-3) Suggestion to improve comments.
FROM:
/* If the publisher is v16 or later, copy in binary format.*/
TO:
/* If the publisher is v16 or later, copy data in the required data format. */
(2) Minor suggestion for some test code alignment.
$result =
$node_subscriber->safe_psql('postgres',
"SELECT sum(a) FROM tst_dom_constr");
-is($result, '21', 'sql-function constraint on domain');
+is($result, '33', 'sql-function constraint on domain');
+
+$result_binary =
+ $node_subscriber->safe_psql('postgres',
+ "SELECT sum(a) FROM tst_dom_constr");
+is($result_binary, '33', 'sql-function constraint on domain');
I think if we change the order of this part of check like below, then
it would look more aligned with other existing test codes introduced by this patch.
---
my $domain_check = 'SELECT sum(a) FROM tst_dom_constr';
$result = $node_subscriber->safe_psql('postgres', $domain_check);
$result_binary = $node_subscriber->safe_psql('postgres', $domain_check);
is($result, '33', 'sql-function constraint on domain');
is($result_binary, '33', 'sql-function constraint on domain in binary');
---
Best Regards,
Takamichi Osumi
