Feedback-ID: id4a34324:Fastmail
Date: Fri, 30 Jun 2023 18:50:07 -0700
From: Andres Freund <andres@anarazel.de>
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Extensible storage manager API - SMGR hook Redux
Message-ID: <20230701015007.fu5nsjaxoird3ejx@awork3.anarazel.de>
References: <CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK=PCNWEMrA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK=PCNWEMrA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230701015007.fu5nsjaxoird3ejx%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-06-30 14:26:44 +0200, Matthias van de Meent wrote:
> diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
> index 4c49393fc5..8685b9fde6 100644
> --- a/src/backend/postmaster/postmaster.c
> +++ b/src/backend/postmaster/postmaster.c
> @@ -1002,6 +1002,11 @@ PostmasterMain(int argc, char *argv[])
>  	 */
>  	ApplyLauncherRegister();
>  
> +	/*
> +	 * Register built-in managers that are not part of static arrays
> +	 */
> +	register_builtin_dynamic_managers();
> +
>  	/*
>  	 * process any libraries that should be preloaded at postmaster start
>  	 */

That doesn't strike me as a good place to initialize this, we'll need it in
multiple places that way.  How about putting it into BaseInit()?


> -static const f_smgr smgrsw[] = {
> +static f_smgr *smgrsw;

This adds another level of indirection. I would rather limit the number of
registerable smgrs than do that.


> +SMgrId
> +smgr_register(const f_smgr *smgr, Size smgrrelation_size)
> +{

> +	MemoryContextSwitchTo(old);
> +
> +	pg_compiler_barrier();

Huh, what's that about?


> @@ -59,14 +63,8 @@ typedef struct SMgrRelationData
>  	 * Fields below here are intended to be private to smgr.c and its
>  	 * submodules.  Do not touch them from elsewhere.
>  	 */
> -	int			smgr_which;		/* storage manager selector */
> -
> -	/*
> -	 * for md.c; per-fork arrays of the number of open segments
> -	 * (md_num_open_segs) and the segments themselves (md_seg_fds).
> -	 */
> -	int			md_num_open_segs[MAX_FORKNUM + 1];
> -	struct _MdfdVec *md_seg_fds[MAX_FORKNUM + 1];
> +	SMgrId		smgr_which;		/* storage manager selector */
> +	int			smgrrelation_size;	/* size of this struct, incl. smgr-specific data */

It looked to me like you determined this globally - why do we need it in every
entry then?

Greetings,

Andres Freund