Обсуждение: Patch to address creation of PgStat* contexts with null parent context

Hi,

There are instances where pgstat_setup_memcxt() and
pgstat_prep_pending_entry() are invoked before the CacheMemoryContext
has been created.  This results in PgStat* contexts being created
without a parent context.  Most easily reproduced/seen in autovacuum
worker via pgstat_setup_memcxt().

Attached is a patch to address this.

To see the issue one can add a line similar to this to the top of 
MemoryContextCreate() in mcxt.c
fprintf(stderr, "pid: %d ctxname: %s parent is %s CacheMemoryContext is %s\n", MyProcPid, name, parent == NULL ? "NULL"
:"not NULL", CacheMemoryContext == NULL ? "NULL" : "Not NULL") 
and startup postgres and grep for the above after autovacuum workers
have been invoked

...snip...
pid: 1427756 ctxname: PgStat Pending parent is NULL CacheMemoryContext is NULL                            
pid: 1427756 ctxname: PgStat Shared Ref parent is NULL CacheMemoryContext is NULL
...snip...

or

startup postgres, attach gdb to postgres following child, break at
pgstat_setup_memcxt and wait for autovacuum worker to start...

...snip...
─── Source
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 384  AllocSetContextCreateInternal(MemoryContext parent,
 385                                const char *name,
 386                                Size minContextSize,
 387                                Size initBlockSize,
 388                                Size maxBlockSize)
 389  {
 390      int            freeListIndex;
 391      Size        firstBlockSize;
 392      AllocSet    set;
 393      AllocBlock    block;
─── Stack
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
[0] from 0x000055b7e4088b40 in AllocSetContextCreateInternal+0 at
/home/rthompso/src/git/postgres/src/backend/utils/mmgr/aset.c:389
[1] from 0x000055b7e3f41c88 in pgstat_setup_memcxt+2544 at
/home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:979
[2] from 0x000055b7e3f41c88 in pgstat_get_entry_ref+2648 at
/home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:410
[3] from 0x000055b7e3f420ea in pgstat_get_entry_ref_locked+26 at
/home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_shmem.c:598
[4] from 0x000055b7e3f3e2c4 in pgstat_report_autovac+36 at
/home/rthompso/src/git/postgres/src/backend/utils/activity/pgstat_database.c:68
[5] from 0x000055b7e3e7f267 in AutoVacWorkerMain+807 at
/home/rthompso/src/git/postgres/src/backend/postmaster/autovacuum.c:1694
[6] from 0x000055b7e3e7f435 in StartAutoVacWorker+133 at
/home/rthompso/src/git/postgres/src/backend/postmaster/autovacuum.c:1493
[7] from 0x000055b7e3e87367 in StartAutovacuumWorker+557 at
/home/rthompso/src/git/postgres/src/backend/postmaster/postmaster.c:5539
[8] from 0x000055b7e3e87367 in sigusr1_handler+935 at
/home/rthompso/src/git/postgres/src/backend/postmaster/postmaster.c:5244
[9] from 0x00007fb02bca7420 in __restore_rt
[+]
─── Threads
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
[1] id 1174088 name postgres from 0x000055b7e4088b40 in AllocSetContextCreateInternal+0 at
/home/rthompso/src/git/postgres/src/backend/utils/mmgr/aset.c:389
─── Variables
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
arg parent = 0x0, name = 0x55b7e416f179 "PgStat Shared Ref": 80 'P', minContextSize = 0, initBlockSize = 1024,
maxBlockSize= 8192 
loc firstBlockSize = <optimized out>, set = <optimized out>, block = <optimized out>, __FUNCTION__ =
"AllocSetContextCreateInternal",__func__ = "AllocSetContextCreateInternal" 

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
> > > print CacheMemoryContext == NULL
$4 = 1
> > > print parent
$5 = (MemoryContext) 0x0

Thanks,
Reid

At Thu, 28 Jul 2022 22:03:13 +0800, Zhang Mingli <zmlpostgres@gmail.com> wrote in 
> Hi,
> 
> On Jul 28, 2022, 21:30 +0800, Reid Thompson <reid.thompson@crunchydata.com>, wrote:
> > Attached is a patch to address this.

Good Catch!

> Codes seem good, my question is:
> 
> Do auto vacuum processes need CacheMemoryContext?

pgstat_report_vacuum requires it. Startup process doesn't seem to use
pgstats while recovery proceeding but requires the context only at
termination...

> Is it designed not to  create CacheMemoryContext in such processes?
> 
> If so, we’d better use TopMemoryContext in such processes.

That makes the memorycontext-tree structure unstable because
CacheMemoryContext can be created on-the-fly.

Honestly I don't like to call CreateCacheMemoryContext in the two
functions on-the-fly.  Since every process that calls
pgstat_initialize() necessarily calls pgstat_setup_memcxt() at latest
at process termination, I think we can create at least
CacheMemoryContext in pgstat_initialize(). Or couldn't we create the
all three contexts in the function, instead of calling
pgstat_setup_memcxt() on-the fly?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, 2022-07-29 at 11:53 +0900, Kyotaro Horiguchi wrote:
>
> That makes the memorycontext-tree structure unstable because
> CacheMemoryContext can be created on-the-fly.
>
> Honestly I don't like to call CreateCacheMemoryContext in the two
> functions on-the-fly.  Since every process that calls
> pgstat_initialize() necessarily calls pgstat_setup_memcxt() at latest
> at process termination, I think we can create at least
> CacheMemoryContext in pgstat_initialize().

Attached is a patch creating CacheMemoryContext() in pgstat_initialize()
rather than the two previous patch locations.

> Or couldn't we create the
> all three contexts in the function, instead of calling
> pgstat_setup_memcxt() on-the fly?

You note that that pgstat_setup_memcxt() is called at latest at process
termination -- was the intent to hold off on requesting memory for these
two contexts until it was needed?

> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center


--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

reid.thompson@crunchydata.com
www.crunchydata.com

At Thu, 04 Aug 2022 13:12:32 -0400, Reid Thompson <reid.thompson@crunchydata.com> wrote in
> On Fri, 2022-07-29 at 11:53 +0900, Kyotaro Horiguchi wrote:
> >
> > That makes the memorycontext-tree structure unstable because
> > CacheMemoryContext can be created on-the-fly.
> >
> > Honestly I don't like to call CreateCacheMemoryContext in the two
> > functions on-the-fly.  Since every process that calls
> > pgstat_initialize() necessarily calls pgstat_setup_memcxt() at latest
> > at process termination, I think we can create at least
> > CacheMemoryContext in pgstat_initialize().
>
> Attached is a patch creating CacheMemoryContext() in pgstat_initialize()
> rather than the two previous patch locations.
>
> > Or couldn't we create the
> > all three contexts in the function, instead of calling
> > pgstat_setup_memcxt() on-the fly?
>
> You note that that pgstat_setup_memcxt() is called at latest at process
> termination -- was the intent to hold off on requesting memory for these
> two contexts until it was needed?

I think it a bit different.  Previously that memory (but for a bit
different use, precisely) was required only when stats data is read so
almost all server processes didn't need it.  Now, every server process
that uses pgstats requires the two memory if it is going to write
stats.  Even if that didn't happen until process termination, that
memory eventually required to flush possibly remaining data.  That
final write might be avoidable but I'm not sure it's worth the
trouble.  As the result, calling pgstat_initialize() is effectively
the declaration that the process requires the memory.

Thus I thought that we may let pgstat_initialize() promptly allocate
the memory.

Does it make sense?


About the patch, I had something like the attached in my mind.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 05 Aug 2022 17:22:38 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Thus I thought that we may let pgstat_initialize() promptly allocate
> the memory.
> 
> Does it make sense?
> 
> 
> About the patch, I had something like the attached in my mind.

I haven't fully checked, but this change might cause all other calls
to CreateCacheMemoryContext useless.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2022-08-05 17:22:38 +0900, Kyotaro Horiguchi wrote:
> I think it a bit different.  Previously that memory (but for a bit
> different use, precisely) was required only when stats data is read so
> almost all server processes didn't need it.  Now, every server process
> that uses pgstats requires the two memory if it is going to write
> stats.  Even if that didn't happen until process termination, that
> memory eventually required to flush possibly remaining data.  That
> final write might be avoidable but I'm not sure it's worth the
> trouble.  As the result, calling pgstat_initialize() is effectively
> the declaration that the process requires the memory.

I don't think every process will end up calling pgstat_setup_memcxt() -
e.g. walsender, bgwriter, checkpointer probably don't? What do we gain by
creating the contexts eagerly?


> Thus I thought that we may let pgstat_initialize() promptly allocate
> the memory.

That makes some sense - but pgstat_attach_shmem() seems like a very strange
place for the call to CreateCacheMemoryContext().


I wonder if we shouldn't just use TopMemoryContext as the parent for most of
these contexts instead. CacheMemoryContext isn't actually a particularly good
fit anymore.

Greetings,

Andres Freund

At Sat, 6 Aug 2022 19:19:39 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2022-08-05 17:22:38 +0900, Kyotaro Horiguchi wrote:
> > I think it a bit different.  Previously that memory (but for a bit
> > different use, precisely) was required only when stats data is read so
> > almost all server processes didn't need it.  Now, every server process
> > that uses pgstats requires the two memory if it is going to write
> > stats.  Even if that didn't happen until process termination, that
> > memory eventually required to flush possibly remaining data.  That
> > final write might be avoidable but I'm not sure it's worth the
> > trouble.  As the result, calling pgstat_initialize() is effectively
> > the declaration that the process requires the memory.
> 
> I don't think every process will end up calling pgstat_setup_memcxt() -
> e.g. walsender, bgwriter, checkpointer probably don't? What do we gain by
> creating the contexts eagerly?

Yes. they acutally does, in shmem_shutdown hook function, during
at-termination stats write.  I didn't consider to make that not
happen, to save 2kB of memory on such small number of processes.

> > Thus I thought that we may let pgstat_initialize() promptly allocate
> > the memory.
> 
> That makes some sense - but pgstat_attach_shmem() seems like a very strange
> place for the call to CreateCacheMemoryContext().

Sure. (I hesitantly added #include for catcache.h..)

> I wonder if we shouldn't just use TopMemoryContext as the parent for most of
> these contexts instead. CacheMemoryContext isn't actually a particularly good
> fit anymore.

It looks better than creating CacheMemoryContext. Now
pgstat_initialize() creates the memory contexts for pgstats use under
TopMemoryContext.

And we don't hastle to avoid maybe-empty at-process-termination
writes..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2022-08-08 15:12:08 +0900, Kyotaro Horiguchi wrote:
> At Sat, 6 Aug 2022 19:19:39 -0700, Andres Freund <andres@anarazel.de> wrote in
> > Hi,
> >
> > On 2022-08-05 17:22:38 +0900, Kyotaro Horiguchi wrote:
> > > I think it a bit different.  Previously that memory (but for a bit
> > > different use, precisely) was required only when stats data is read so
> > > almost all server processes didn't need it.  Now, every server process
> > > that uses pgstats requires the two memory if it is going to write
> > > stats.  Even if that didn't happen until process termination, that
> > > memory eventually required to flush possibly remaining data.  That
> > > final write might be avoidable but I'm not sure it's worth the
> > > trouble.  As the result, calling pgstat_initialize() is effectively
> > > the declaration that the process requires the memory.
> >
> > I don't think every process will end up calling pgstat_setup_memcxt() -
> > e.g. walsender, bgwriter, checkpointer probably don't? What do we gain by
> > creating the contexts eagerly?
>
> Yes. they acutally does, in shmem_shutdown hook function, during
> at-termination stats write.  I didn't consider to make that not
> happen, to save 2kB of memory on such small number of processes.

That's true for checkpointer, but not e.g. for walwriter, bgwriter. I don't
see why we should force allocate memory that we're never going to use in
background processes.


> And we don't hastle to avoid maybe-empty at-process-termination
> writes..

Hm?

Greetings,

Andres Freund

Hi,

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



Hi,

On 2022-08-05 17:22:38 +0900, Kyotaro Horiguchi wrote:

I think it a bit different.  Previously that memory (but for a bit
different use, precisely) was required only when stats data is read so
almost all server processes didn't need it.  Now, every server process
that uses pgstats requires the two memory if it is going to write
stats.  Even if that didn't happen until process termination, that
memory eventually required to flush possibly remaining data.  That
final write might be avoidable but I'm not sure it's worth the
trouble.  As the result, calling pgstat_initialize() is effectively
the declaration that the process requires the memory.

I don't think every process will end up calling pgstat_setup_memcxt() -
e.g. walsender, bgwriter, checkpointer probably don't? What do we gain by
creating the contexts eagerly?

Thus I thought that we may let pgstat_initialize() promptly allocate
the memory.

That makes some sense - but pgstat_attach_shmem() seems like a very strange
place for the call to CreateCacheMemoryContext().


I wonder if we shouldn't just use TopMemoryContext as the parent for most of
these contexts instead. CacheMemoryContext isn't actually a particularly good
fit anymore.

Could using TopMemoryContext like in the attach be an option? (aka changing CacheMemoryContext by TopMemoryContext in the 3 places of interest): that would ensure the 3 pgStat* contexts to have a non NULL parent context.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

At Mon, 5 Sep 2022 08:52:44 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in 
> Could using TopMemoryContext like in the attach be an option? (aka
> changing CacheMemoryContext by TopMemoryContext in the 3 places of
> interest): that would ensure the 3 pgStat* contexts to have a non NULL
> parent context.

Of course it works. The difference from what I last proposed is
whether we postpone creating the memory contexts until the first call
to pgstat_get_entry_ref().  The rationale of creating them at
pgstat_attach_shmem is that anyway once pgstat_attach_shmem is called,
the process fainally creates the contexts at the end of the process,
and (I think) it's simpler that we don't do if() check at every
pgstat_get_entry_ref() call.

I forgot about pgStatPendingContext, but it is sensible that we treat
it the same way to the other two.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

At Mon, 5 Sep 2022 08:52:44 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

Could using TopMemoryContext like in the attach be an option? (aka
changing CacheMemoryContext by TopMemoryContext in the 3 places of
interest): that would ensure the 3 pgStat* contexts to have a non NULL
parent context.

Of course it works. The difference from what I last proposed is
whether we postpone creating the memory contexts until the first call
to pgstat_get_entry_ref(). 

Right.

 The rationale of creating them at
pgstat_attach_shmem is that anyway once pgstat_attach_shmem is called,
the process fainally creates the contexts at the end of the process,

Right.

IIUC the downside is to allocate the new contexts even for processes that don't need them (as mentioned by Andres upthread).

and (I think) it's simpler that we don't do if() check at every
pgstat_get_entry_ref() call.

I wonder how much of a concern the if() checks are, given they are all 3 legitimately using unlikely().

Looks like that both approaches have their pros and cons. I'm tempted to vote +1 on "just changing" the parent context to TopMemoryContext and not changing the allocations locations.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 2022-09-05 17:32:20 +0900, Kyotaro Horiguchi wrote:
> The rationale of creating them at pgstat_attach_shmem is that anyway once
> pgstat_attach_shmem is called, the process fainally creates the contexts at
> the end of the process, and (I think) it's simpler that we don't do if()
> check at every pgstat_get_entry_ref() call.

But that's not true, as pointed out here:
https://postgr.es/m/20220808192020.nc556tlgcp66fdgw%40awork3.anarazel.de

Nor does it make sense to reserve memory for the entire lifetime of a process
just because we might need it for a split second at the end.

Greetings,

Andres Freund

(It seems to me I overlooked some mails.. sorry.)

At Mon, 5 Sep 2022 15:47:37 -0700, Andres Freund <andres@anarazel.de> wrote in 
> On 2022-09-05 17:32:20 +0900, Kyotaro Horiguchi wrote:
> > The rationale of creating them at pgstat_attach_shmem is that anyway once
> > pgstat_attach_shmem is called, the process fainally creates the contexts at
> > the end of the process, and (I think) it's simpler that we don't do if()
> > check at every pgstat_get_entry_ref() call.
> 
> But that's not true, as pointed out here:
> https://postgr.es/m/20220808192020.nc556tlgcp66fdgw%40awork3.anarazel.de
> 
> Nor does it make sense to reserve memory for the entire lifetime of a process
> just because we might need it for a split second at the end.

Yeah, that's the most convincing argument aginst it.

At Mon, 5 Sep 2022 14:46:55 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in 
> Looks like that both approaches have their pros and cons. I'm tempted
> to vote +1 on "just changing" the parent context to TopMemoryContext
> and not changing the allocations locations.

Yeah. It is safe more than anything and we don't have a problem there.

So, I'm fine with just replacing the parent context at the three places.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

At Mon, 5 Sep 2022 14:46:55 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

Looks like that both approaches have their pros and cons. I'm tempted
to vote +1 on "just changing" the parent context to TopMemoryContext
and not changing the allocations locations.

Yeah. It is safe more than anything and we don't have a problem there.

So, I'm fine with just replacing the parent context at the three places.

Attached a patch proposal to do so.

Regards,
--

Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

At Wed, 7 Sep 2022 11:11:11 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in 

> On 9/6/22 7:53 AM, Kyotaro Horiguchi wrote:
> > So, I'm fine with just replacing the parent context at the three
> > places.

Looks good to me. To make sure, I ran make check-world with adding an
assertion check that all non-toplevel memcontexts are created under
non-null parent and I saw no failure.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

At Wed, 7 Sep 2022 11:11:11 +0200, "Drouvot, Bertrand" <bdrouvot@amazon.com> wrote in

On 9/6/22 7:53 AM, Kyotaro Horiguchi wrote:

So, I'm fine with just replacing the parent context at the three
places.

Looks good to me. To make sure, I ran make check-world with adding an
assertion check that all non-toplevel memcontexts are created under
non-null parent and I saw no failure.

Thanks!

I'm updating the CF entry to Ready for Committer.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 2022-09-07 11:11:11 +0200, Drouvot, Bertrand wrote:
> On 9/6/22 7:53 AM, Kyotaro Horiguchi wrote:
> > At Mon, 5 Sep 2022 14:46:55 +0200, "Drouvot, Bertrand"<bdrouvot@amazon.com>  wrote in
> > > Looks like that both approaches have their pros and cons. I'm tempted
> > > to vote +1 on "just changing" the parent context to TopMemoryContext
> > > and not changing the allocations locations.
> > Yeah. It is safe more than anything and we don't have a problem there.
> > 
> > So, I'm fine with just replacing the parent context at the three places.
> 
> Attached a patch proposal to do so.

Pushed. Thanks for the report and the fix!

Greetings,

Andres Freund

Hi,

On 9/17/22 6:10 PM, Andres Freund wrote:
> Hi,
> 
> On 2022-09-07 11:11:11 +0200, Drouvot, Bertrand wrote:
>> On 9/6/22 7:53 AM, Kyotaro Horiguchi wrote:
>>> At Mon, 5 Sep 2022 14:46:55 +0200, "Drouvot, Bertrand"<bdrouvot@amazon.com>  wrote in
>>>> Looks like that both approaches have their pros and cons. I'm tempted
>>>> to vote +1 on "just changing" the parent context to TopMemoryContext
>>>> and not changing the allocations locations.
>>> Yeah. It is safe more than anything and we don't have a problem there.
>>>
>>> So, I'm fine with just replacing the parent context at the three places.
>>
>> Attached a patch proposal to do so.
> 
> Pushed. Thanks for the report and the fix!

Thanks! I just marked the corresponding CF entry as Committed.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com