Thread

Commits

  1. Fix corruption of pgstats shared hashtable due to OOM failures

  1. Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Mikhail Kot <mikhail.kot@databricks.com> — 2025-09-02T20:09:54Z

    Hi,
    
    I've encountered the following segmentation fault lately. It happens when
    Postgres is experiencing high memory pressure. There are multiple OOM errors in
    the log as well.
    
    Core was generated by `postgres: neondb_owner neondb ::1(46658) BIND
              '.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  pg_atomic_read_u32_impl (ptr=0x8) at
    ../../../../src/include/port/atomics/generic.h:48
    #1  pg_atomic_read_u32 (ptr=0x8) at ../../../../src/include/port/atomics.h:239
    #2  LWLockAttemptLock (lock=lock@entry=0x4,
    mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:821
    #3  0x000056446bce129f in LWLockConditionalAcquire (lock=0x4,
    mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1386
    #4  0x000056446bd0bacf in pgstat_lock_entry
    (entry_ref=entry_ref@entry=0x56446d9f4340, nowait=nowait@entry=true)
    at pgstat_shmem.c:625
    #5  0x000056446bd0a3c9 in pgstat_relation_flush_cb
    (entry_ref=0x56446d9f4340, nowait=<optimized out>) at
    pgstat_relation.c:794
    #6  0x000056446bd069f5 in pgstat_flush_pending_entries
    (nowait=<optimized out>) at pgstat.c:1217
    #7  pgstat_report_stat (force=<optimized out>, force@entry=false) at
    pgstat.c:658
    #8  0x000056446bcf16c1 in PostgresMain (dbname=<optimized out>,
    username=<optimized out>) at postgres.c:4623
    #9  0x000056446bc716b3 in BackendRun (port=<optimized out>,
    port=<optimized out>) at postmaster.c:4465
    #10 BackendStartup (port=<optimized out>) at postmaster.c:4193
    #11 ServerLoop () at postmaster.c:1782
    #12 0x000056446bc726ea in PostmasterMain (argc=argc@entry=3,
    argv=argv@entry=0x56446cd803b0) at postmaster.c:1466
    #13 0x000056446b9d5a00 in main (argc=3, argv=0x56446cd803b0) at main.c:238
    
    The error originates from pgstat_shmem.c file where shhashent is left in
    half-initialized state if pgstat_init_entry(), calling dsa_allocate0(), errors
    out with OOM. Then shhashent causes a segmentation fault on access. I propose a
    patch which solves this issue. The patch is for main branch, but the code is
    nearly identical in Postgres 13-17 so I suggest backporting it to other
    supported versions.
    
    The patch changes pgstat_init_entry()'s behaviour, returning NULL if memory
    allocation failed. It also adds sanity checks to routines accepting arguments
    returned by pgstat_init_entry().
    
    Reproducing this behaviour is tricky, because under OOM Postgres doesn't
    necessarily reach the condition where specific dsa_allocate0() call errors.
    
  2. Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-03T07:10:01Z

    On Tue, Sep 02, 2025 at 09:09:54PM +0100, Mikhail Kot wrote:
    > The error originates from pgstat_shmem.c file where shhashent is left in
    > half-initialized state if pgstat_init_entry(), calling dsa_allocate0(), errors
    > out with OOM. Then shhashent causes a segmentation fault on access. I propose a
    > patch which solves this issue. The patch is for main branch, but the code is
    > nearly identical in Postgres 13-17 so I suggest backporting it to other
    > supported versions.
    
    Ugh.  Support for pgstats in shared memory has been added in v15, so
    v13 and v14 are out of scope, aren't they?
    
    > The patch changes pgstat_init_entry()'s behaviour, returning NULL if memory
    > allocation failed. It also adds sanity checks to routines accepting arguments
    > returned by pgstat_init_entry().
    
    @@ -430,6 +430,9 @@ pgstat_database_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
     	PgStatShared_Database *sharedent;
     	PgStat_StatDBEntry *pendingent;
     
    +	if (!entry_ref)
    +		return false;
    
    The flush callbacks are by design expected to return false *if and
    only if* the flush of the stats is *not forced* and they could not be 
    flushed due to lock contention.  This addition means that when
    pgstat_report_stat() uses its "force" mode, then it may randomly
    behave incorrectly and would fail to fulfill its design contract.
    
    > Reproducing this behaviour is tricky, because under OOM Postgres doesn't
    > necessarily reach the condition where specific dsa_allocate0() call errors.
    
    Deterministic testing would not be complicated, one can use an
    injection point that does a stack manipulation with
    IS_INJECTION_POINT_ATTACHED() or just return an error, but I don't see
    much value in doing that here as long as a fix is local to
    pgstat_init_entry().
    
    Requiring that all the callers of pgstat_init_entry() need to cope
    with a potential OOM error path seems like a recipe that could be
    easily forgotten, even if we redesign the flush callbacks to be able
    to know about a more complex error state, which means to rewrite the
    code to return an enum state made of three possible values for OOM,
    success and lock contention.
    
    Anyway, couldn't we flip the order of the operations in
    pgstat_init_entry() so as we do first an allocation and avoid any
    inconsistency in the shared state?  Or we could use DSA_ALLOC_NO_OOM,
    and put back the shared state in a consistent state before issuing an
    error if we find that the result of the allocation is NULL.  Requiring
    an error on allocation seems more important to me here, we do that for
    palloc() and I don't see why we should not just do the same here for
    this DSA usage in the pgstats code.
    --
    Michael
    
  3. 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Steven Niu <niushiji@gmail.com> — 2025-09-03T07:22:00Z

    I found there are many cases of following pattern:
    
    ptr_1 = dsa_allocate();
    ptr_2 = dsa_get_address(xxx, ptr_1);
    ptr_2->yyy = zzz;
    
    Inside dsa_get_address(dsa_area *area, dsa_pointer dp):
    
          /* Convert InvalidDsaPointer to NULL. */
          if (!DsaPointerIsValid(dp))
                return NULL;
    
    So unless dsa_allocate() can ensure never returns InvalidDsaPointer, there is risk of SegV.
    In fact the function dsa_allocate() does return InvalidDsaPointer in some cases.
    
    So, maybe should we add pointer check in all places where dsa_get_address is called. Comments?
    
    
    ________________________________
    发件人: Mikhail Kot <mikhail.kot@databricks.com>
    已发送: 2025 年 9 月 03 日 星期三 04:09
    收件人: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
    抄送: to@myrrc.dev <to@myrrc.dev>
    主题: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c
    
    Hi,
    
    I've encountered the following segmentation fault lately. It happens when
    Postgres is experiencing high memory pressure. There are multiple OOM errors in
    the log as well.
    
    Core was generated by `postgres: neondb_owner neondb ::1(46658) BIND
              '.
    Program terminated with signal SIGSEGV, Segmentation fault.
    #0  pg_atomic_read_u32_impl (ptr=0x8) at
    ../../../../src/include/port/atomics/generic.h:48
    #1  pg_atomic_read_u32 (ptr=0x8) at ../../../../src/include/port/atomics.h:239
    #2  LWLockAttemptLock (lock=lock@entry=0x4,
    mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:821
    #3  0x000056446bce129f in LWLockConditionalAcquire (lock=0x4,
    mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1386
    #4  0x000056446bd0bacf in pgstat_lock_entry
    (entry_ref=entry_ref@entry=0x56446d9f4340, nowait=nowait@entry=true)
    at pgstat_shmem.c:625
    #5  0x000056446bd0a3c9 in pgstat_relation_flush_cb
    (entry_ref=0x56446d9f4340, nowait=<optimized out>) at
    pgstat_relation.c:794
    #6  0x000056446bd069f5 in pgstat_flush_pending_entries
    (nowait=<optimized out>) at pgstat.c:1217
    #7  pgstat_report_stat (force=<optimized out>, force@entry=false) at
    pgstat.c:658
    #8  0x000056446bcf16c1 in PostgresMain (dbname=<optimized out>,
    username=<optimized out>) at postgres.c:4623
    #9  0x000056446bc716b3 in BackendRun (port=<optimized out>,
    port=<optimized out>) at postmaster.c:4465
    #10 BackendStartup (port=<optimized out>) at postmaster.c:4193
    #11 ServerLoop () at postmaster.c:1782
    #12 0x000056446bc726ea in PostmasterMain (argc=argc@entry=3,
    argv=argv@entry=0x56446cd803b0) at postmaster.c:1466
    #13 0x000056446b9d5a00 in main (argc=3, argv=0x56446cd803b0) at main.c:238
    
    The error originates from pgstat_shmem.c file where shhashent is left in
    half-initialized state if pgstat_init_entry(), calling dsa_allocate0(), errors
    out with OOM. Then shhashent causes a segmentation fault on access. I propose a
    patch which solves this issue. The patch is for main branch, but the code is
    nearly identical in Postgres 13-17 so I suggest backporting it to other
    supported versions.
    
    The patch changes pgstat_init_entry()'s behaviour, returning NULL if memory
    allocation failed. It also adds sanity checks to routines accepting arguments
    returned by pgstat_init_entry().
    
    Reproducing this behaviour is tricky, because under OOM Postgres doesn't
    necessarily reach the condition where specific dsa_allocate0() call errors.
    
  4. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-03T09:43:20Z

    On Wed, Sep 03, 2025 at 07:22:00AM +0000, Steven Niu wrote:
    > So unless dsa_allocate() can ensure never returns InvalidDsaPointer,
    > there is risk of SegV.  In fact the function dsa_allocate() does
    > return InvalidDsaPointer in some cases.
    > 
    > So, maybe should we add pointer check in all places where dsa_get_address is called. Comments?
    
    dsa_allocate() calls dsa_allocate_extended() without DSA_ALLOC_NO_OOM,
    hence on allocation failure the code does a ereport(ERROR).  It would
    be a problem to not check the result if DSA_ALLOC_NO_OOM is used.
    
    The problem dealt with here is different, as far as I understand: we
    set some data in shared memory without considering that the DSA
    allocation could fail, leaving shared memory in an inconsistent state
    when an allocation failure occurs.  The problem is not in the
    allocation failure in itself, but in the dependency that we have
    between the state in shared memory and the allocation attempt for this
    pgstats code path. 
    --
    Michael
    
  5. 回复: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Steven Niu <niushiji@gmail.com> — 2025-09-03T09:55:35Z

    ________________________________________
    发件人: Michael Paquier
    已发送: 2025 年 9 月 03 日 星期三 17:43
    收件人: Steven Niu
    抄送: Mikhail Kot; pgsql-hackers@lists.postgresql.org; to@myrrc.dev
    主题: Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c
    
    On Wed, Sep 03, 2025 at 07:22:00AM +0000, Steven Niu wrote:
    > So unless dsa_allocate() can ensure never returns InvalidDsaPointer,
    > there is risk of SegV.  In fact the function dsa_allocate() does
    > return InvalidDsaPointer in some cases.
    >
    > So, maybe should we add pointer check in all places where dsa_get_address is called. Comments?
    
    dsa_allocate() calls dsa_allocate_extended() without DSA_ALLOC_NO_OOM,
    hence on allocation failure the code does a ereport(ERROR).  It would
    be a problem to not check the result if DSA_ALLOC_NO_OOM is used.
    
    Thanks, Michael, you are correct. 
    
    The problem dealt with here is different, as far as I understand: we
    set some data in shared memory without considering that the DSA
    allocation could fail, leaving shared memory in an inconsistent state
    when an allocation failure occurs.  The problem is not in the
    allocation failure in itself, but in the dependency that we have
    between the state in shared memory and the allocation attempt for this
    pgstats code path.
    --
    Michael
    
  6. Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Ranier Vilela <ranier.vf@gmail.com> — 2025-09-03T11:55:31Z

    Em qua., 3 de set. de 2025 às 03:34, Mikhail Kot <mikhail.kot@databricks.com>
    escreveu:
    
    > Hi,
    >
    > I've encountered the following segmentation fault lately. It happens when
    > Postgres is experiencing high memory pressure. There are multiple OOM
    > errors in
    > the log as well.
    >
    > Core was generated by `postgres: neondb_owner neondb ::1(46658) BIND
    >           '.
    > Program terminated with signal SIGSEGV, Segmentation fault.
    > #0  pg_atomic_read_u32_impl (ptr=0x8) at
    > ../../../../src/include/port/atomics/generic.h:48
    > #1  pg_atomic_read_u32 (ptr=0x8) at
    > ../../../../src/include/port/atomics.h:239
    > #2  LWLockAttemptLock (lock=lock@entry=0x4,
    > mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:821
    > #3  0x000056446bce129f in LWLockConditionalAcquire (lock=0x4,
    > mode=mode@entry=LW_EXCLUSIVE) at lwlock.c:1386
    > #4  0x000056446bd0bacf in pgstat_lock_entry
    > (entry_ref=entry_ref@entry=0x56446d9f4340, nowait=nowait@entry=true)
    > at pgstat_shmem.c:625
    > #5  0x000056446bd0a3c9 in pgstat_relation_flush_cb
    > (entry_ref=0x56446d9f4340, nowait=<optimized out>) at
    > pgstat_relation.c:794
    > #6  0x000056446bd069f5 in pgstat_flush_pending_entries
    > (nowait=<optimized out>) at pgstat.c:1217
    > #7  pgstat_report_stat (force=<optimized out>, force@entry=false) at
    > pgstat.c:658
    > #8  0x000056446bcf16c1 in PostgresMain (dbname=<optimized out>,
    > username=<optimized out>) at postgres.c:4623
    > #9  0x000056446bc716b3 in BackendRun (port=<optimized out>,
    > port=<optimized out>) at postmaster.c:4465
    > #10 BackendStartup (port=<optimized out>) at postmaster.c:4193
    > #11 ServerLoop () at postmaster.c:1782
    > #12 0x000056446bc726ea in PostmasterMain (argc=argc@entry=3,
    > argv=argv@entry=0x56446cd803b0) at postmaster.c:1466
    > #13 0x000056446b9d5a00 in main (argc=3, argv=0x56446cd803b0) at main.c:238
    >
    > The error originates from pgstat_shmem.c file where shhashent is left in
    > half-initialized state if pgstat_init_entry(), calling dsa_allocate0(),
    > errors
    > out with OOM. Then shhashent causes a segmentation fault on access. I
    > propose a
    > patch which solves this issue. The patch is for main branch, but the code
    > is
    > nearly identical in Postgres 13-17 so I suggest backporting it to other
    > supported versions.
    >
    > The patch changes pgstat_init_entry()'s behaviour, returning NULL if memory
    > allocation failed.
    
    I'm wondering if it wouldn't be better to raise elog(ERROR), and avoid
    many checks for this NULL.
    
    best regards,
    Ranier Vilela
    
  7. Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Mikhail Kot <mikhail.kot@databricks.com> — 2025-09-03T21:39:04Z

    Hi Michael, Steven, and Ranier,
    
    > Anyway, couldn't we flip the order of the operations in
    pgstat_init_entry() so as we do first an allocation and avoid any inconsistency
    in the shared state?
    
    The issue is not only in pgstat_init_entry(). Currently it errors on OOM but
    this doesn't prevent us from calling pgstat_lock_entry() through
    pgstat_get_entry_ref() which accesses a non-initialized lock.
    
    Here's the second version of the patch. Now we remove inserted hash entry
    on OOM which would prevent accessing the entry
    
  8. 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Steven Niu <niushiji@gmail.com> — 2025-09-04T02:31:34Z

    Hi, Mikhail,
    
    If pgstat_init_entry() errors on OOM, what would it returns to shheader, NULL?
    That would bring trouble to dshash_delete_entry().
    
    Thanks,
    Steven
    
    
    ________________________________
    发件人: Mikhail Kot <mikhail.kot@databricks.com>
    已发送: 2025 年 9 月 04 日 星期四 05:39
    收件人: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
    抄送: to@myrrc.dev <to@myrrc.dev>
    主题: Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c
    
    Hi Michael, Steven, and Ranier,
    
    > Anyway, couldn't we flip the order of the operations in
    pgstat_init_entry() so as we do first an allocation and avoid any inconsistency
    in the shared state?
    
    The issue is not only in pgstat_init_entry(). Currently it errors on OOM but
    this doesn't prevent us from calling pgstat_lock_entry() through
    pgstat_get_entry_ref() which accesses a non-initialized lock.
    
    Here's the second version of the patch. Now we remove inserted hash entry
    on OOM which would prevent accessing the entry
    
  9. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-04T06:30:49Z

    On Thu, Sep 04, 2025 at 02:31:34AM +0000, Steven Niu wrote:
    > If pgstat_init_entry() errors on OOM, what would it returns to shheader, NULL?
    > That would bring trouble to dshash_delete_entry().
    
    Based on the proposal of patch 0002, the code would throw an error
    after cleaning up the shared memory state.  The generation and
    refcount number assigned inside pgstat_init_entry() would not matter
    as well because the entry created by dshash_find_or_insert() would be
    entirely gone.  So I am not sure what's the point you are trying to
    make here.
    --
    Michael
    
  10. Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-04T07:33:30Z

    On Wed, Sep 03, 2025 at 10:39:04PM +0100, Mikhail Kot wrote:
    > The issue is not only in pgstat_init_entry(). Currently it errors on OOM but
    > this doesn't prevent us from calling pgstat_lock_entry() through
    > pgstat_get_entry_ref() which accesses a non-initialized lock.
    
    Spent more time on that, finally.  So your argument is that this leads
    to an inconsistent state in the hash table because the dshash API is
    designed to force a new entry creation if it cannot be found.
    
    -            shheader = pgstat_init_entry(kind, shhashent);
    +            PG_TRY();
    +            {
    +                shheader = pgstat_init_entry(kind, shhashent);
    +            }
    +            PG_CATCH();
    +            {
    +                dshash_delete_entry(pgStatLocal.shared_hash, shhashent);
    +                PG_RE_THROW();
    +            }
    +            PG_END_TRY();
    
    Hmm.  There are a couple of extra errors that can be reached, through
    get_segment_by_index() or dsa_get_address() for example, but these
    point to scenarios that should never happen or programming errors when
    using DSAs.
    
    > Here's the second version of the patch. Now we remove inserted hash entry
    > on OOM which would prevent accessing the entry
    
    There are only two callers of pgstat_init_entry(), so I am wondering
    if a better long-term thing would be to track this behavior in
    pgstat_init_entry(), and let the callers deal with the cleanup
    consequences, rather than have the callers of pgstat_init_entry()
    guess that they may need to do something with a TRY/CATCH block.  I
    doubt that the number of callers of pgstat_init_entry() will raise,
    but people like doing fancy things.
    
    In pgstat_read_statsfile(), an interesting side effect is the
    possibility to issue a soft error.  I have never seen anybody complain
    about an OOM from the startup process when reading the stats file,
    TBH, still prioritizing availability is an interesting take when
    reading the stats file when facing a DSA allocation failure.
    
    In order to reproduce one failure pattern, you can use the attached
    0002, then use this sequence to emulate the OOM path and the dshash
    table inconsistency (install src/test/modules/injection_points first):
    create extension injection_points;
    select injection_points_attach('pgstat-init-entry-oom', 'notice');
    -- SQL query able to create fresh pgstats entry
    -- ERROR with patch 0001, crash on HEAD.
    
    Note that none of that seems worth a backpatch, we have an history of
    treating unlikely-going-to-happen errors like OOMs as HEAD-only
    improvements.  This one is of the same class.
    --
    Michael
    
  11. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Steven Niu <niushiji@gmail.com> — 2025-09-04T07:38:41Z

    _______________________________________
    From: Michael Paquier
    Sent: Thursday, September 04, 2025 14:30
    To: Steven Niu
    Cc: Mikhail Kot; pgsql-hackers@lists.postgresql.org; to@myrrc.dev
    Subject: Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c
    
    On Thu, Sep 04, 2025 at 02:31:34AM +0000, Steven Niu wrote:
    > If pgstat_init_entry() errors on OOM, what would it returns to shheader, NULL?
    > That would bring trouble to dshash_delete_entry().
    
    Based on the proposal of patch 0002, the code would throw an error
    after cleaning up the shared memory state.  The generation and
    refcount number assigned inside pgstat_init_entry() would not matter
    as well because the entry created by dshash_find_or_insert() would be
    entirely gone.  So I am not sure what's the point you are trying to
    make here.
    --
    Michael
    
    
    Sorry, I made a mistake. I should say: 
    "If pgstat_init_entry() errors on OOM, the local variable shheader may be NULL. This would bring trouble to pgstat_acquire_entry_ref() in the line 30 of patch 002". 
    
  12. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Rider <oceanustz@gmail.com> — 2025-09-04T07:49:19Z

    Hi Steven,
    
    I think when an error is thrown within the PG_TRY block, the assignment to
    shheader is interrupted and never completes. As a result, shheader retains
    its initial, NULL value.
    
    And, the PG_RE_THROW() within the PG_CATCH block causes a non-local jump,
    immediately aborting the current execution path to handle the error at a
    higher level. This guarantees that the code following PG_END_TRY is
    unreachable in the error scenario.
    
    Steven Niu <niushiji@gmail.com> 于2025年9月4日周四 15:38写道:
    
    > _______________________________________
    > From: Michael Paquier
    > Sent: Thursday, September 04, 2025 14:30
    > To: Steven Niu
    > Cc: Mikhail Kot; pgsql-hackers@lists.postgresql.org; to@myrrc.dev
    > Subject: Re: 回复: Fix segfault while accessing half-initialized hash table
    > in pgstat_shmem.c
    >
    > On Thu, Sep 04, 2025 at 02:31:34AM +0000, Steven Niu wrote:
    > > If pgstat_init_entry() errors on OOM, what would it returns to shheader,
    > NULL?
    > > That would bring trouble to dshash_delete_entry().
    >
    > Based on the proposal of patch 0002, the code would throw an error
    > after cleaning up the shared memory state.  The generation and
    > refcount number assigned inside pgstat_init_entry() would not matter
    > as well because the entry created by dshash_find_or_insert() would be
    > entirely gone.  So I am not sure what's the point you are trying to
    > make here.
    > --
    > Michael
    >
    >
    > Sorry, I made a mistake. I should say:
    > "If pgstat_init_entry() errors on OOM, the local variable shheader may be
    > NULL. This would bring trouble to pgstat_acquire_entry_ref() in the line 30
    > of patch 002".
    >
    
  13. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Mikhail Kot <mikhail.kot@databricks.com> — 2025-09-05T20:46:55Z

    Hi,
    
    Do you want me to update the patch based on your suggestion on fault
    injection, or update the try/catch to the callers as discussed, or
    it's good to be included in Postgres?
    
    
    
    
  14. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-06T01:01:24Z

    On Fri, Sep 05, 2025 at 09:46:55PM +0100, Mikhail Kot wrote:
    > Do you want me to update the patch based on your suggestion on fault
    > injection, or update the try/catch to the callers as discussed, or
    > it's good to be included in Postgres?
    
    I would prefer the approach of letting the callers deal with the error
    handling, by making the callers of pgstat_init_entry() be aware of the
    problem based on the result returned, as posted at:
    https://www.postgresql.org/message-id/aLlAym4DHW4PM8Gg@paquier.xyz
    
    What I do not like in my patch is the change in
    pgstat_read_statsfile().  I have wondered about the availability
    argument but there's a risk of discarding the stats based on the
    memory pressure, which is different from the current pattern where we
    rely entirely on the state of the on-disk file for corruption.  So it
    should be changed to generate an ERROR, with an errdetail() similar to
    pgstat_get_entry_ref() but consistent in style to the other messages
    in pgstat_read_statsfile().
    
    The injection point business is useful for testing, but I don't see a
    point in including something in the patch, because the code changes
    influence the test outputs.
    
    A last thing that I was not able to spend much time on is how much it
    is possible to mess up with the shared memory state.  If it is worse
    than I suspected initially, where an OOM in a first session can cause
    crashes because of incorrect dshash entries in shmem depending on the
    stats kind fetched, a backpatch will be required, indeed.  The change
    is not really invasive, so that's OK on this side.
    
    If you can send an updated patch version among these lines, that would
    be of course helpful for me.  I should be able to get back to the
    problem on Monday my time.
    --
    Michael
    
  15. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-06T01:05:07Z

    On Thu, Sep 04, 2025 at 03:49:19PM +0800, Rider wrote:
    > And, the PG_RE_THROW() within the PG_CATCH block causes a non-local jump,
    > immediately aborting the current execution path to handle the error at a
    > higher level. This guarantees that the code following PG_END_TRY is
    > unreachable in the error scenario.
    
    Please see details in utils/elog.h, if you want to study this area of
    the code of course.  There is a large portion about volatile variables
    and compiler expectations which is also very interested to know about.
    And that's useful if you write your own extension code, outside of the
    core Postgres code.
    --
    Michael
    
  16. Re: 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c

    Michael Paquier <michael@paquier.xyz> — 2025-09-08T07:54:16Z

    On Sat, Sep 06, 2025 at 10:01:24AM +0900, Michael Paquier wrote:
    > A last thing that I was not able to spend much time on is how much it
    > is possible to mess up with the shared memory state.  If it is worse
    > than I suspected initially, where an OOM in a first session can cause
    > crashes because of incorrect dshash entries in shmem depending on the
    > stats kind fetched, a backpatch will be required, indeed.  The change
    > is not really invasive, so that's OK on this side.
    
    OK, I have played a bit more with all that, corrupting the shared
    hashtable of pgstats.  At the end, I have used a version close to what
    I have sent previously that changes pgstat_init_entry() to return NULL
    on OOM with dsa_allocate_extended(), as any other possible errors that
    could happen in this call involve elog(ERROR) and not-reachable cases.
    
    pgstat_read_statsfile() has been changed to raise an ERROR instead,
    which is what we did previously, giving priority to the on-disk stats
    when the environment is under memory pressure at startup.  The patch
    has required a few tweaks in the back-branches, nothing huge.
    
    Thanks for the report!
    --
    Michael