Date: Tue, 5 Mar 2024 15:50:44 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Banck <mbanck@gmx.net>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Abhijit Menon-Sen <ams@toroid.org>,
	pgsql-hackers@lists.postgresql.org,
	=?utf-8?B?5oiQ5LmL54SV?= <zhcheng@ceresdata.com>
Subject: Re: [PATCH] Exponential backoff for auth_delay
Message-ID: <20240305215044.GA3538518@nathanxps13>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <65e62440.170a0220.f6a7c.6f7f@mx.google.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20240305215044.GA3538518%40nathanxps13>
Precedence: bulk

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