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

thanks and sorry for the issues, please find attached updated version.

On Sat, Nov 1, 2025 at 8:36 AM Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:

> >> 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
>

--
Nadav Shatz
Tailor Brands | CTO