Re: Connection slots reserved for replication

Hello,

> On 24. Jan 2019, at 13:47, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

Thank you for the review.I took a liberty to address it with v9.

>
> Documentation looks fine for me. By the way, a comment for the
> caller-site of CheckRequreParameterValues() in xlog.c looks
> somewhat stale.
>
>> /* Check to see if any changes to max_connections give problems */
>> CheckRequiredParameterValues();
>
> may be better something like this?:
>
>> /* Check to see if any parameter change gives a problem on replication */

I changed it to "Check if any parameter change gives a problem on recovery”, as I think it is independent of the [streaming] replication, but I still don’t like the wording, as it just duplicate the comment at the definition of CheckRequiredParameterValues. Maybe remove the comment altogether?

>
>
> In postinit.c:
>> /*
>> * The last few connection slots are reserved for superusers.
>> */
>> if ((!am_superuser && !am_walsender) &&
>> ReservedBackends > 0 &&
>
> This is forgetting about explaing about walsenders.
>
>> The last few connection slots are reserved for
>> superusers. Replication connections don't share the same slot
>> pool.
>
> Or something?

I changed it to

+ * The last few connection slots are reserved for superusers.
+ * Replication connections are drawn from a separate pool and
+ * not limited by max_connections or superuser_reserved_connections.

>
> And the parentheses enclosing "!am_superuser..walsender" seems no
> longer needed.
>
>
> In guc.c:
> - /* see max_connections and max_wal_senders */
> + /* see max_connections */
>
> I don't understand for what reason we should see max_connections
> just above. (Or even max_wal_senders) Do we need this comment?
> (There're some other instances, but they wont'be for this patch.)

I don’t understand what is it pointing to as well, so I’ve removed it.

>
>
> In pg_controldata.c:
> + printf(_("max_wal_senders setting: %d\n"),
> + ControlFile->max_wal_senders);
> printf(_("max_worker_processes setting: %d\n"),
> ControlFile->max_worker_processes);
> printf(_("max_prepared_xacts setting: %d\n"),
>
> The added item seems to want some additional spaces.

Good catch, fixed.

Attached is v9. I also bumped up the PG_CONTROL_VERSION to 1200 per the prior comment by Robert.

Cheers,
Oleksii