public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tristan Partin <[email protected]>
To: Matthias van de Meent <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: Re: Extensible storage manager API - SMGR hook Redux
Date: Tue, 11 Jul 2023 15:57:34 -0500
Message-ID: <CTZN6R9A57SL.25NQGHTWUO7EA@gonk> (raw)
In-Reply-To: <CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK=PCNWEMrA@mail.gmail.com>
References: <CAEze2WgMySu2suO_TLvFyGY3URa4mAx22WeoEicnK=PCNWEMrA@mail.gmail.com>

> 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)

view thread (9+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Extensible storage manager API - SMGR hook Redux
  In-Reply-To: <CTZN6R9A57SL.25NQGHTWUO7EA@gonk>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox