Re: Proposal: recent access based routing for primary-replica setups

>> Hi,
>>
>> I'm back at work - wdyt of this version?
>
> Thanks for the patch! I will look into it weekend.

Here are review comments.

1. git apply complains trailing whitespace and new blank line.

$ git apply ~/0001-external-replication-delay-implementation.patch
/home/t-ishii/0001-external-replication-delay-implementation.patch:225: trailing whitespace.

/home/t-ishii/0001-external-replication-delay-implementation.patch:232: trailing whitespace.

/home/t-ishii/0001-external-replication-delay-implementation.patch:246: trailing whitespace.

warning: 3 lines add whitespace errors.
$ git apply ~/0002-external-replication-delay-tests-and-docs.patch
/home/t-ishii/0002-external-replication-delay-tests-and-docs.patch:639: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

2. You can use psprintf() instead of palloc() + snprintf() to make the code simpler.

+ if (!bi || bi->backend_hostname[0] == '\0' || bi->backend_port <= 0)
+ {
+ /* Fallback if hostname or port is not set */
+ out = palloc(32);
+ snprintf(out, 32, "unknown_node_%d", node_id);
+ return out;
+ }

3. Ditto as above.

+ /* Use hostname:port format */
+ hlen = strlen(bi->backend_hostname);
+ /* max port chars ~5, plus colon and NUL */
+ out_len = hlen + 1 + 5 + 1;
+ out = palloc(out_len);
+ snprintf(out, out_len, "%s:%d", bi->backend_hostname, bi->backend_port);
+ return out;

4. There are a few compiler warnings.

streaming_replication/pool_worker_child.c: In function ‘do_worker_child’:
streaming_replication/pool_worker_child.c:269:33: warning: this ‘else’ clause does not guard... [-Wmisleading-indentation]
269 | else
| ^~~~
streaming_replication/pool_worker_child.c:273:41: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘else’
273 | node_status = verify_backend_node_status(slots);
| ^~~~~~~~~~~

5. 041.external_replication_delay failed.

timeout: failed to run command './test.sh': Permission denied

After running "chmod 755" to fix the issue, still the test fails. From
src/test/regression/log/041.external_replication_delay:

./test.sh: line 45: syntax error near unexpected token `('
./test.sh: line 45: `echo === Test0: External command receives replica identifiers only (primary omitted) ==='

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp