Re: Separate GUC for replication origins

On Fri, Jan 24, 2025, at 8:12 PM, Masahiko Sawada wrote:
> Here are some comments on v2 patch:
>
> ---
> /* Report this after the initial starting message for consistency. */
> - if (max_replication_slots == 0)
> + if (max_replication_origins == 0)
> ereport(ERROR,
> (errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
> errmsg("cannot start logical
> replication workers when \"max_replication_slots\"=0")));
>
> Need to update the error message too.

Good catch!

> ---
> + {"max_replication_origins",
> + PGC_POSTMASTER,
> + REPLICATION_SUBSCRIBERS,
> + gettext_noop("Sets the maximum number of
> simultaneously defined replication origins."),
> + NULL
> + },
>
> I think the description is not accurate; this GUC controls the maximum
> number of simultaneous replication origins that can be setup.

Instead of "defined" I use "configured". It seems closer to "setup".

> ---
> Given that max_replication_origins doesn't control the maximum number
> of replication origins that can be defined, probably we need to find a
> better name. As Kuroda-san already mentioned some proposed names,
> max_tracked_replication_origins or max_replication_origin_states seem
> reasonable to me.

The max_replication_origins name is not accurate. I chose it because (a) it is
a runtime limit, (b) it is short and (c) a description can provide the exact
meaning. I think the proposed names don't still reflect the exact meaning. The
"tracked" word provides the meaning that the replication origin is tracked but
in this case it should mean "setup". An existing replication origin that is not
in use is tracked although its information is not available in the
pg_replication_origin_status. The "states" word doesn't make sense in this
context. Do you mean "status" (same as the view name)?

Under reflection, an accurate name is max_replication_origin_session_setup. A
counter argument is that it is a long name (top-5 length).

postgres=# select n, length(n) from (values('max_replication_origins'),
('max_tracked_replication_origins'),('max_replication_origin_states'),
('max_replication_origin_session_setup')) as gucs(n);
n | length
--------------------------------------+--------
max_replication_origins | 23
max_tracked_replication_origins | 31
max_replication_origin_states | 29
max_replication_origin_session_setup | 36
(4 rows)

postgres=# select name, length(name) from pg_settings order by 2 desc
limit 15;
name | length
---------------------------------------------+--------
max_parallel_apply_workers_per_subscription | 43
ssl_passphrase_command_supports_reload | 38
autovacuum_vacuum_insert_scale_factor | 37
autovacuum_multixact_freeze_max_age | 35
debug_logical_replication_streaming | 35
idle_in_transaction_session_timeout | 35
autovacuum_vacuum_insert_threshold | 34
log_parameter_max_length_on_error | 33
vacuum_multixact_freeze_table_age | 33
max_sync_workers_per_subscription | 33
client_connection_check_interval | 32
max_parallel_maintenance_workers | 32
shared_memory_size_in_huge_pages | 32
restrict_nonsystem_relation_kind | 32
autovacuum_analyze_scale_factor | 31
(15 rows)

> ---
> +#include "utils/guc_hooks.h"
>
> I think #include'ing guc.h would be more appropriate.

Fixed.

I also updated the pg_createsubscriber documentation that refers to
max_replication_slots.

Since we don't have an agreement about the name, I still kept
max_replication_origins.

--
Euler Taveira
EDB https://www.enterprisedb.com/