From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Banck <mbanck@gmx.net>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Abhijit Menon-Sen <ams@toroid.org>
Cc: pgsql-hackers@lists.postgresql.org, ζδΉη <zhcheng@ceresdata.com>
Subject: Re: [PATCH] Exponential backoff for auth_delay
Date: Tue, 5 Mar 2024 15:50:44 -0600
Message-ID: <20240305215044.GA3538518@nathanxps13> (raw)
In-Reply-To: <65e62440.170a0220.f6a7c.6f7f@mx.google.com>
References: <658c4eab.050a0220.731ff.011e@mx.google.com>
<65965e9d.050a0220.919a6.f9e1@mx.google.com>
<ZaYimdRlGS5tdzjy@toroid.org>
<65aa8265.170a0220.1ecef.91df@mx.google.com>
<65aa8439.050a0220.31e77.0071@mx.google.com>
<da09dacb-7158-4af1-99f8-6609070b7553@enterprisedb.com>
<65e62440.170a0220.f6a7c.6f7f@mx.google.com>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, mbanck@gmx.net, tomas.vondra@enterprisedb.com, ams@toroid.org, zhcheng@ceresdata.com
Subject: Re: [PATCH] Exponential backoff for auth_delay
In-Reply-To: <20240305215044.GA3538518@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox