Re: Extensible storage manager API - SMGR hook Redux

> Subject: [PATCH v1 1/2] Expose f_smgr to extensions for manual implementation

From what I can see, all the md* APIs that were exposed in md.h can now
be made static in md.c. The only other references to those APIs were in
smgr.c.

> Subject: [PATCH v1 2/2] Prototype: Allow tablespaces to specify which SMGR
> they use

> -typedef uint8 SMgrId;
> +/*
> + * volatile ID of the smgr. Across various configurations IDs may vary,
> + * true identity is the name of each smgr.
> + */
> +typedef int SMgrId;
>
> -#define MaxSMgrId UINT8_MAX
> +#define MaxSMgrId INT_MAX

In a future revision of this patch, seems worthwhile to just start as
int instead of a uint8 to avoid this song and dance. Maybe int8 instead
of int?

> +static SMgrId recent_smgrid = -1;

You could use InvalidSmgrId here.

> +void smgrvalidatetspopts(const char *smgrname, List *opts)
> +{
> + SMgrId smgrid = get_smgr_by_name(smgrname, false);
> +
> + smgrsw[smgrid].smgr_validate_tspopts(opts);
> +}
> +
> +void smgrcreatetsp(const char *smgrname, Oid tsp, List *opts, bool isredo)
> +{
> + SMgrId smgrid = get_smgr_by_name(smgrname, false);
> +
> + smgrsw[smgrid].smgr_create_tsp(tsp, opts, isredo);
> +}
> +
> +void smgrdroptsp(const char *smgrname, Oid tsp, bool isredo)
> +{
> + SMgrId smgrid = get_smgr_by_name(smgrname, false);
> +
> + smgrsw[smgrid].smgr_drop_tsp(tsp, isredo);
> +}

Do you not need to check if smgrid is the InvalidSmgrId? I didn't see
any other validation anywhere.

> + char *smgr;
> + List *smgropts; /* list of DefElem nodes */

smgrname would probably work better alongside tablespacename in that
struct.

> @@ -221,7 +229,7 @@ mdexists(SMgrRelation reln, ForkNumber forknum)
> if (!InRecovery)
> mdclose(reln, forknum);
>
> - return (mdopenfork(reln, forknum, EXTENSION_RETURN_NULL) != NULL);
> + return (mdopenfork(mdreln, forknum, EXTENSION_RETURN_NULL) != NULL);
> }

Was this a victim of a bad rebase? Seems like it belongs in the previous
patch.

> +void mddroptsp(Oid tsp, bool isredo)
> +{
> +
> +}

Some functions in this file have the return type on the previous line.

This is a pretty slick patchset. Excited to read more dicussion and how
it evolves.

--
Tristan Partin
Neon (https://neon.tech)