On Sat, Aug 7, 2021 at 2:01 PM Bossart, Nathan <bossa...@amazon.com> wrote:
> Here is a rebased version of the patch.
>
Giving this a review.
Patch applies cleanly and `make check` works as of
e12694523e7e4482a052236f12d3d8b58be9a22c
Overall looks very nice and tucks MaxBackends safely away.
I have a few suggestions:
> + size = add_size(size, mul_size(GetMaxBackends(0),
sizeof(BTOneVacInfo)));
The use of GetMaxBackends(0) looks weird - can we add another constant in
there
for the "default" case? Or just have GetMaxBackends() work?
> + GMB_MAX_PREPARED_XACTS = 1 << 0, /* include max_prepared_xacts */
s/include/add in/
> +typedef enum GMBOption
> +{
> + GMB_MAX_PREPARED_XACTS = 1 << 0, /* include max_prepared_xacts */
> + GMB_NUM_AUXILIARY_PROCS = 1 << 1 /* include NUM_AUXILIARY_PROCS */
> +} GMBOption;
Is a typedef enum really needed? As opposed to something like this style:
#define WL_LATCH_SET (1 << 0)
#define WL_SOCKET_READABLE (1 << 1)
#define WL_SOCKET_WRITEABLE (1 << 2)
> - (MaxBackends + max_prepared_xacts + 1));
> + (GetMaxBackends(GMB_MAX_PREPARED_XACTS) + 1));
This is a little confusing - there is no indication to the reader that this
is an additive function. Perhaps a little more intuitive name:
> + (GetMaxBackends(GMB_PLUS_MAX_PREPARED_XACTS) + 1));
However, the more I went through this patch, the more the GetMaxBackends(0)
nagged at me. The vast majority of the calls are with "0". I'd argue for
just having no arguments at all, which removes a bit of code and actually
makes things like this easier to read:
Original change:
> - uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS +
max_prepared_xacts;
> + uint32 TotalProcs = GetMaxBackends(GMB_NUM_AUXILIARY_PROCS |
GMB_MAX_PREPARED_XACTS);
Versus:
> - uint32 TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS +
max_prepared_xacts;
> + uint32 TotalProcs = GetMaxBackends() + NUM_AUXILIARY_PROCS +
max_prepared_xacts;
> + * This must be called after modules have had the change to alter GUCs in
> + * shared_preload_libraries, and before shared memory size is determined.
s/change/chance/;
> +void
> +SetMaxBackends(int max_backends)
> +{
> + if (MaxBackendsInitialized)
> + elog(ERROR, "MaxBackends already initialized");
Is this going to get tripped by a call from restore_backend_variables?
Cheers,
Greg
