Re: [PATCH] Exponential backoff for auth_delay

On Mon, Mar 04, 2024 at 08:42:55PM +0100, Michael Banck wrote:
> On Wed, Feb 21, 2024 at 10:26:11PM +0100, Tomas Vondra wrote:
>> I think it'd be good to consider:
>>
>> - Making the acr_array a hash table, and larger than 50 entries (I
>> wonder if that should be dynamic / customizable by GUC?).
>
> I would say a GUC should be overkill for this as this would mostly be an
> implementation detail.
>
> More generally, I think now that entries are expired (see below), this
> should be less of a concern, so I have not changed this to a hash table
> for now but doubled MAX_CONN_RECORDS to 100 entries.

I don't have a strong opinion about making this configurable, but I do
think we should consider making this a hash table so that we can set
MAX_CONN_RECORDS much higher.

Also, since these records are stored in shared memory, don't we need to
lock them when searching/updating?

> +static void
> +auth_delay_init_state(void *ptr)
> +{
> + Size shm_size;
> + AuthConnRecord *array = (AuthConnRecord *) ptr;
> +
> + shm_size = sizeof(AuthConnRecord) * MAX_CONN_RECORDS;
> +
> + memset(array, 0, shm_size);
> +}
> +
> +static void
> +auth_delay_shmem_startup(void)
> +{
> + bool found;
> + Size shm_size;
> +
> + if (shmem_startup_next)
> + shmem_startup_next();
> +
> + shm_size = sizeof(AuthConnRecord) * MAX_CONN_RECORDS;
> + acr_array = GetNamedDSMSegment("auth_delay", shm_size, auth_delay_init_state, &found);
> +}

Great to see the DSM registry getting some use. This example makes me
wonder whether the size of the segment should be passed to the
init_callback.

> /*
> * Module Load Callback
> */
> void
> _PG_init(void)
> {
> + if (!process_shared_preload_libraries_in_progress)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("auth_delay must be loaded via shared_preload_libraries")));
> +

This change seems like a good idea independent of this feature.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com