MIME-Version: 1.0
References: 
 <CAExHW5vM1bneLYfg0wGeAa=52UiJ3z4vKd3AJ72X8Fw6k3KKrg@mail.gmail.com>
 <cc3ff976-c303-45cd-9a8d-a90483170e37@iki.fi>
 <CA+TgmoauqYXm8iA3FGRAVKxYShUxWBiS_MSLmQpTrmO7wNHamw@mail.gmail.com>
 <113724ab-0028-493f-9605-6e8570f0939f@iki.fi>
 <CAExHW5uTNWOSxJDWQAUnS0tZawob2_J3dRAtc67NHNZ98X4_xA@mail.gmail.com>
 <CAExHW5t439y61YD9bc7d5wZWHp6J=M43Qu3eEZOBPguZML7o2A@mail.gmail.com>
 <CAExHW5v5FVZbsO9sLzztMZ11C3hgGStE=HkkV2bQkCyncess4w@mail.gmail.com>
 <b2451225-0a10-4ded-abef-4e9efbde7a8e@iki.fi>
 <CAExHW5tCC0T1ky=Jnq-AvMxa67Adaw7aQ4iQAO=BSdHcbSNBVg@mail.gmail.com>
 <791c3f18-f4de-4d84-ac6b-c7ccc074dd38@iki.fi>
 <CAExHW5tS7GncN90oJWOSzW_3F1EHL9xwe59L7Req3nUVgmObUw@mail.gmail.com>
 <9d919bd9-94dd-4bda-8ccf-ebced4178c53@iki.fi>
 <CAEze2WhMOHVgH2Xeyzx=VEk-Ta_YnQUqT+TdBiv5Lx8ESn2WZA@mail.gmail.com>
 <CAExHW5s6h=c_q2m72Nvyj1ghMEhPkOBkeN5Htn7YR=1BrNN-Sw@mail.gmail.com>
 <7d3ba240-9350-4dfc-bbe1-be6584aee236@iki.fi>
 <CAEze2WjQZff3znd6CtG-OBzYZMMqy5TyQSoAo=QTFT38tDndeQ@mail.gmail.com>
 <1c3a07a7-158d-4800-927c-2641c73277d8@iki.fi>
In-Reply-To: <1c3a07a7-158d-4800-927c-2641c73277d8@iki.fi>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Sun, 5 Apr 2026 11:18:10 +0530
Message-ID: 
 <CAExHW5twYLhVjtApX3osdaz-4b0Tjvjj_Zr20orUb61qgiJmUw@mail.gmail.com>
Subject: Re: Better shared data structure management and resizable shared data
 structures
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
	chaturvedipalak1911@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAExHW5twYLhVjtApX3osdaz-4b0Tjvjj_Zr20orUb61qgiJmUw%40mail.gmail.com>
Precedence: bulk

On Sat, Apr 4, 2026 at 11:02=E2=80=AFPM Heikki Linnakangas <hlinnaka@iki.fi=
> wrote:
>
> On 04/04/2026 15:00, Matthias van de Meent wrote:
> > On Sat, 4 Apr 2026 at 02:45, Heikki Linnakangas <hlinnaka@iki.fi> wrote=
:
> >>>> I don't understand the use of ShmemStructDesc. They generally/always
> >>>> are private to request_fn(), and their fields are used exclusively
> >>>> inside the shmem mechanisms, with no reads of its fields that can't
> >>>> already be deduced from context. Why do we need that struct
> >>>> everywhere?
> >>>
> >>> My resizable shared memory structure patches use it as a handle to th=
e
> >>> structure to be resized.
> >>
> >> Right. And hash tables and SLRUs use a desc-like object already, so fo=
r
> >> symmetry it feels natural to have it for plain structs too.
> >> I wonder if we should make it optional though, for the common case tha=
t
> >> you have no intention of doing anything more with the shmem region tha=
t
> >> you'd need a desc for. I'm thinking you could just pass NULL for the
> >> desc pointer:
> >>
> >>       ShmemRequestStruct(NULL,
> >>           .name =3D "pg_stat_statements",
> >>           .size =3D sizeof(pgssSharedState),
> >>           .ptr =3D (void **) &pgss,
> >>       };
> >
> > That would help, though I'd still wonder why we'd have separate Opts
> > and Desc structs. IIUC, they generally carry (exactly) the same data.
> >
> > Maybe moving it into a `.handle` or `.desc` field in Shmem*Opts could
> > make that part of the code a bit cleaner; as it'd further clarify that
> > it's very much an optional field.
>
> Yeah. OTOH, I'd like to separate the options from what's effectively a
> return value. But maybe you're right and it's nevertheless better that wa=
y.
>
> Some options on this:
>
> a) What's in the patch now
>
>      static ShmemStructDesc pgssSharedStateDesc;
>
>      ShmemRequestStruct(&pgssSharedStateDesc,
>                         .name =3D "pg_stat_statements",
>                         .size =3D sizeof(pgssSharedState),
>                         .ptr =3D (void **) &pgss);
>
> b) Allow passing NULL for the desc
>
>      ShmemRequestStruct(NULL,
>                         .name =3D "pg_stat_statements",
>                         .size =3D sizeof(pgssSharedState),
>                         .ptr =3D (void **) &pgss);
>
> c) Return the Desc as a return value
>
>      static ShmemStructDesc *pgssSharedStateDesc;
>
>      pgssSharedStateDesc =3D
>          ShmemRequestStruct(.name =3D "pg_stat_statements",
>                             .size =3D sizeof(pgssSharedState),
>                             .ptr =3D (void **) &pgss);
>
> In option c) you can just throw away the result if you don't need it. I
> kind of like this as a notational thing. However it has some downsides:
>
> This changes the return value to be a pointer. I'm thinking that
> ShmemRequestStruct() palloc's the descriptor struct in TopMemoryContext.
> This is a little ugly because the descriptor struct is leaked if the
> caller throws it away. It's not a lot of memory, but still.
>
> I'm also not sure how well this fits in with the SLRU code. On 'master',
> you already have SlruCtlData which is like the "desc" struct. Would we
> turn that into a pointer too, adding one indirection to all the SLRU
> calls. It's probably fine from a performance point of view, but it feels
> like it's going in the wrong direction.
>
> d) Make it part of Opts, as you suggested
>
>      static ShmemStructDesc pgssSharedStateDesc;
>
>      ShmemRequestStruct(.name =3D "pg_stat_statements",
>                         .size =3D sizeof(pgssSharedState),
>                         .ptr =3D (void **) &pgss,
>                         .desc =3D &pgssSharedStateDesc);
>
> In the attached new version, though, I stepped back and decided to
> remove the whole ShmemStructDesc after all. I still think having a
> handle like that is a good idea, and the follow-up patches for resizing
> need it. However, with option d) it can easily be added later. With
> option d), it seems silly to have it be part of the patch now, when the
> desc struct doesn't really do anything. SLRU's still have a similar
> SlruDesc struct, however. For SLRUs it's essentially the same as the old
> SlruCtlData struct before these patches.
>
> The Desc structs were being used for one thing though: I used the 'size'
> from the Desc struct in ProcGlobalShmemInit() to get the allocated size
> of each shmem area. The size computation there is complicated enough
> that I'd rather not repeat it, and avoiding the repeated size
> calculation was the raison d'=C3=AAtre for these patches. I replaced it w=
ith
> global variables to hold the sizes from the ShmemRequest() step to
> ShmemInit(). But that would be one case where having the desc would
> already be useful. Then again, I'm not sure we want to expose the 'size'
> in the descriptor like that anyway, because as soon as we make shmem
> regions resizable, we might not be able to keep the size in the
> descriptor up-to-date. The size of these structs won't change, but we
> might not want to expose the information because it would be confusing
> for other structs where it can change to show outdated information.
>
> On a related note, when we add back the ".desc" concept later, is
> ".desc" a good name, or ".handle" as you also suggested? More widely, do
> we call the concept and the struct a "handle" or "descriptor" or what?
> Or if we follow the precedence with the existing SlruCtlData struct, it
> could be ".ctl". I'm not a fan of the "Ctl" naming though, because we
> already have a lot of structs with "Ctl" in the name and it's not always
> clear whether a "Ctl" struct refers to the shared memory parts or the
> handle to it. Now that the "desc" structs are not part of these patches
> anymore, however, we can punt on that decision.

Resizing patches can do without Desc, they use name has the handle
instead. I was not comfortable with current state of Desc either
because they are not opaque as I had pointed out earlier. A caller can
scribble on them. There is not need to decide on the handle decision
right now, even for resizing patches. If we decide to add a handle, I
would like it to be opaque. I thought about using ShmemIndexEnt *
itself as the opaque pointer; we shouldn't expose it to the users of
shmem.c that it's ShmemIndexEnt * though. There is downside that we
are giving a much riskier handle in the shmem.c users' hands - they
can now corrupt shared memory itself. We could encapsulate the
ShmemIndexEntry * like how HTAB encapsulates HASHHDR if needed.
Advantage of this approach is that ShmemResizeStruct() or any shmem.c
API accepting the handle doesn't need to perform a ShmemIndex lookup.
Just ideas, nothing required right now.

>
> On 02/04/2026 09:58, Ashutosh Bapat wrote:
> >>
> >> I renamed it to AttachOrInitShmemIndexEntry, and the args to 'may_init=
'
> >> and 'may_attach'. But more importantly I added comments to explain the
> >> different usages. Hope that helps..
> >
> > The explanation in the prologue looks good. But the function is still
> > confusing. Instead of if ... else fi ... chain, I feel organizing this
> > as below would make it more readable. (this was part of one of my
> > earlier edit patches).
> > if (found)
> > ...
> > else
> > {
> >  if (!may_init)
> >  error
> > if (!index_entry)
> >  error
> >
> > ... rest of the code to initialize and attach
> > }
> >
> > But other than that I don't have any other brilliant ideas.
>
> I did another refactoring in this area: I split
> AttachOrInitShmemIndexEntry() into separate AttachShmemIndexEntry() and
> InitShmemIndexEntry functions again. There's a little bit of repetition
> that way, but IMO it makes it much clearer overall.
>

Yes.

I will post my resizable shmem structures patch in a separate email in
this thread but continue to review your patches.

--=20
Best Wishes,
Ashutosh Bapat