On Wednesday, January 11, 2023 7:45 PM Melih Mutlu <m.melihmu...@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