Horton

Thread

Commits

  1. Rename ShmemVariableCache to TransamVariables

  2. Initialize ShmemVariableCache like other shmem areas

  1. Rename ShmemVariableCache and initialize it in more standard way

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-12-04T12:49:57Z

    This came up in the "Refactoring backend fork+exec code" thread recently 
[0], but is independent of that work:

On 11/07/2023 01:50, Andres Freund wrote:
>> --- a/src/backend/storage/ipc/shmem.c
>> +++ b/src/backend/storage/ipc/shmem.c
>> @@ -144,6 +144,8 @@ InitShmemAllocation(void)
>>  	/*
>>  	 * Initialize ShmemVariableCache for transaction manager. (This doesn't
>>  	 * really belong here, but not worth moving.)
>> +	 *
>> +	 * XXX: we really should move this
>>  	 */
>>  	ShmemVariableCache = (VariableCache)
>>  		ShmemAlloc(sizeof(*ShmemVariableCache));
> 
> Heh. Indeed. And probably just rename it to something less insane.

Here's a patch to allocate and initialize it with a pair of ShmemSize 
and ShmemInit functions, like all other shared memory structs.

+1 on renaming it too. It's not a cache, the values tracked in 
ShmemVariableCache are the authoritative source. Attached patch renames 
it to "TransamVariables", but I'm all ears for other suggestions.

[0] 
https://www.postgresql.org/message-id/20230710225043.svl7fqxecwshwc7a@awork3.anarazel.de

-- 
Heikki Linnakangas
Neon (https://neon.tech)

  2. Re: Rename ShmemVariableCache and initialize it in more standard way

    Tristan Partin <tristan@neon.tech> — 2023-12-04T16:31:36Z

    On Mon Dec 4, 2023 at 6:49 AM CST, Heikki Linnakangas wrote:
> This came up in the "Refactoring backend fork+exec code" thread recently 
> [0], but is independent of that work:
>
> On 11/07/2023 01:50, Andres Freund wrote:
> >> --- a/src/backend/storage/ipc/shmem.c
> >> +++ b/src/backend/storage/ipc/shmem.c
> >> @@ -144,6 +144,8 @@ InitShmemAllocation(void)
> >>  	/*
> >>  	 * Initialize ShmemVariableCache for transaction manager. (This doesn't
> >>  	 * really belong here, but not worth moving.)
> >> +	 *
> >> +	 * XXX: we really should move this
> >>  	 */
> >>  	ShmemVariableCache = (VariableCache)
> >>  		ShmemAlloc(sizeof(*ShmemVariableCache));
> > 
> > Heh. Indeed. And probably just rename it to something less insane.
>
> Here's a patch to allocate and initialize it with a pair of ShmemSize 
> and ShmemInit functions, like all other shared memory structs.
>

>  +        if (!IsUnderPostmaster)
>  +        {
>  +                Assert(!found);
>  +                memset(ShmemVariableCache, 0, sizeof(VariableCacheData));
>  +        }
>  +        else
>  +                Assert(found);

Should the else branch instead be a fatal log?

Patches look good to me.
-- 
Tristan Partin
Neon (https://neon.tech)

  3. Re: Rename ShmemVariableCache and initialize it in more standard way

    Richard Guo <guofenglinux@gmail.com> — 2023-12-05T03:40:53Z

    On Tue, Dec 5, 2023 at 12:31 AM Tristan Partin <tristan@neon.tech> wrote:

> On Mon Dec 4, 2023 at 6:49 AM CST, Heikki Linnakangas wrote:
> > This came up in the "Refactoring backend fork+exec code" thread recently
> > [0], but is independent of that work:
> >
> > Here's a patch to allocate and initialize it with a pair of ShmemSize
> > and ShmemInit functions, like all other shared memory structs.
> >
> >  +        if (!IsUnderPostmaster)
> >  +        {
> >  +                Assert(!found);
> >  +                memset(ShmemVariableCache, 0,
> sizeof(VariableCacheData));
> >  +        }
> >  +        else
> >  +                Assert(found);
>
> Should the else branch instead be a fatal log?


The Assert here seems OK to me.  We do the same when initializing
commitTsShared/MultiXactState.  I think it would be preferable to adhere
to this convention.


> Patches look good to me.


Also +1 to the patches.

Thanks
Richard

  4. Re: Rename ShmemVariableCache and initialize it in more standard way

    Heikki Linnakangas <hlinnaka@iki.fi> — 2023-12-08T07:51:11Z

    On 05/12/2023 05:40, Richard Guo wrote:
> On Tue, Dec 5, 2023 at 12:31 AM Tristan Partin <tristan@neon.tech> wrote:
>     On Mon Dec 4, 2023 at 6:49 AM CST, Heikki Linnakangas wrote:
>      > Here's a patch to allocate and initialize it with a pair of
>     ShmemSize
>      > and ShmemInit functions, like all other shared memory structs.
>      >
>      >  +        if (!IsUnderPostmaster)
>      >  +        {
>      >  +                Assert(!found);
>      >  +                memset(ShmemVariableCache, 0,
>     sizeof(VariableCacheData));
>      >  +        }
>      >  +        else
>      >  +                Assert(found);
> 
>> Should the else branch instead be a fatal log?
> 
> The Assert here seems OK to me.  We do the same when initializing
> commitTsShared/MultiXactState.  I think it would be preferable to adhere
> to this convention.

Right. I'm not 100% happy with that pattern either, but better be 
consistent.

There's a brief comment about this in CreateOrAttachShmemStructs():

>  * This is called by the postmaster or by a standalone backend.
>  * It is also called by a backend forked from the postmaster in the
>  * EXEC_BACKEND case.  In the latter case, the shared memory segment
>  * already exists and has been physically attached to, but we have to
>  * initialize pointers in local memory that reference the shared structures,
>  * because we didn't inherit the correct pointer values from the postmaster
>  * as we do in the fork() scenario.  The easiest way to do that is to run
>  * through the same code as before.  (Note that the called routines mostly
>  * check IsUnderPostmaster, rather than EXEC_BACKEND, to detect this case.
>  * This is a bit code-wasteful and could be cleaned up.)

The last sentence refers to this pattern.

>> Patches look good to me.
> 
> Also +1 to the patches.

Committed, thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)