Unexpected behavior after OOM errors

On Wed, 17 Jun 2026 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:

Hello hackers,

I'd like to share my findings related to OOM error handling. I'm not sure
how large the class of such anomalies is (and if all of these can be
detected and fixed), but please look at a few issues I have discovered so
far:

1) An issue in lookup_type_cache()

The following modification:

<snip>

makes this script:

<snip>

trigger an assertion failure:

<snip>

Without asserts enables, the server might crash.

I believe this is caused by partial subsystem initialization. Attached
patch 0001 should address this failure without causing the server to
restart on OOM.

2) An issue in GetSnapshotData()

The following modification:

<snip>

makes this script (max_prepared_transactions = 2 in postgresql.conf):

<snip>

trigger a segmentation fault:

<snip>

Again, caused by partial initialization, though in this case it's of a
SnapshotData* which is later checked again. Attached patch 0002 should
address this failure.

3) An issue in StandbyAcquireAccessExclusiveLock()

<snip>

I'm not sure how to solve this correctly; I think ideally the
StandbyAcquireAccessExclusiveLock() hash code would be wrapped by a
critical section, but I'm not 100% sure if that will be a sufficient
approach; and it'd definitely need some code to allow the various
hashmaps' memctxs to alloc during critical sections.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Wed, Jun 17, 2026 at 02:27:25PM +0200, Matthias van de Meent wrote:

On Wed, 17 Jun 2026 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:

1) An issue in lookup_type_cache()

I believe this is caused by partial subsystem initialization. Attached
patch 0001 should address this failure without causing the server to
restart on OOM.

Hmm. I think that this is an ordering problem. We could make the
callbacks be registered last, once we are sure that the two hash
tables and the in-progress list have been initialized. I am not sure
that this requires a new facility; it is also an advantage to keep the
initialization sequence in a one code path, without an abstraction.

RelIdToTypeIdCacheHash and RelIdToTypeIdCacheHash are in the
TopMemoryContext, static to the process, so we could just check them
for NULL-ness to make the initialization repeatable. That gives me
the attached v2. Reusing Alexander's randomness trick, that looks
stable here.

2) An issue in GetSnapshotData()

Again, caused by partial initialization, though in this case it's of a
SnapshotData* which is later checked again. Attached patch 0002 should
address this failure.

Yeah, that seems right to make repeated calls of GetSnapshotData()
able to work. LGTM.

3) An issue in StandbyAcquireAccessExclusiveLock()

<snip>

I'm not sure how to solve this correctly; I think ideally the
StandbyAcquireAccessExclusiveLock() hash code would be wrapped by a
critical section, but I'm not 100% sure if that will be a sufficient
approach; and it'd definitely need some code to allow the various
hashmaps' memctxs to alloc during critical sections.

Not checked this one yet.

Thoughts about the first part?
--
Michael

On 18/06/2026 07:37, Michael Paquier wrote:

On Wed, Jun 17, 2026 at 02:27:25PM +0200, Matthias van de Meent wrote:

On Wed, 17 Jun 2026 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:

1) An issue in lookup_type_cache()

I believe this is caused by partial subsystem initialization. Attached
patch 0001 should address this failure without causing the server to
restart on OOM.

Hmm. I think that this is an ordering problem. We could make the
callbacks be registered last, once we are sure that the two hash
tables and the in-progress list have been initialized. I am not sure
that this requires a new facility; it is also an advantage to keep the
initialization sequence in a one code path, without an abstraction.

RelIdToTypeIdCacheHash and RelIdToTypeIdCacheHash are in the
TopMemoryContext, static to the process, so we could just check them
for NULL-ness to make the initialization repeatable. That gives me
the attached v2. Reusing Alexander's randomness trick, that looks
stable here.

Yeah, this can be solved by ordering. It's a bit fiddly though. I don't
know about Matthias's proposal either, but it'd be nice to have a less
fiddly system for these.

One idea is to have something similar to
START_CRIT_SECTION()/END_CRIT_SECTION(), but instead of promoting the
ERROR to a PANIC, promote it to FATAL. That way, if any of these
one-time allocations fail, the backend exits. If you're so
memory-starved that you cannot even initialize the type cache, you won't
be able to do anything useful with the connection anyway.

Another idea is that instead of having these be singletons in the type
cache, initialized on first use, move it to a new TypeCacheInitialize()
function that is always called at backend startup, like
RelationCacheInitialize(). If an allocation fails at that stage, the
backend will just exit. I think that's my favorite alternative so far.

BTW, I'm surprised we create the hash tables are created in
TopMemoryContext rather than CacheMemoryContext...

- Heikki

On Thu, 18 Jun 2026 at 06:37, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jun 17, 2026 at 02:27:25PM +0200, Matthias van de Meent wrote:

On Wed, 17 Jun 2026 at 08:00, Alexander Lakhin <exclusion@gmail.com> wrote:

1) An issue in lookup_type_cache()

I believe this is caused by partial subsystem initialization. Attached
patch 0001 should address this failure without causing the server to
restart on OOM.

Hmm. I think that this is an ordering problem. We could make the
callbacks be registered last, once we are sure that the two hash
tables and the in-progress list have been initialized.

I don't disagree that there's an ordering problem, but in my view the
main problem is the fallible initialization of N components, gated
behind a single condition. The question of when to register the
callbacks is just one part of many.

I am not sure
that this requires a new facility; it is also an advantage to keep the
initialization sequence in a one code path, without an abstraction.

I'm not a huge fan of templating if(!global) { init_global(); } all
around. But, it works.

RelIdToTypeIdCacheHash and RelIdToTypeIdCacheHash are in the
TopMemoryContext, static to the process, so we could just check them
for NULL-ness to make the initialization repeatable. That gives me
the attached v2. Reusing Alexander's randomness trick, that looks
stable here.

This un-fixes one of the unlikely issues that was fixed in my patch -
though unrelated to OOM:

Each of the calls to
CacheRegisterSyscacheCallback/CacheRegisterRelcacheCallback can throw
an ERROR when all slots have been used. This would leave the typcache
in an invalid state, so I think that must be wrapped in a critical
section: neither syscache nor relcache has options to release
callbacks, and we can't safely continue without the callbacks
installed, so once an error is thrown here this backend can't ever be
properly initialized. This is unlike OOMs, whose conditions for
failure may (and often do) change as workloads change in other
backends.

I think Heikki's suggestion for a FATAL critical section option would
be a good alternative. It wouldn't always be sufficient, but would fix
issues here.

2) An issue in GetSnapshotData()

Again, caused by partial initialization, though in this case it's of a
SnapshotData* which is later checked again. Attached patch 0002 should
address this failure.

Yeah, that seems right to make repeated calls of GetSnapshotData()
able to work. LGTM.

Thanks for committing!

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Thu, 18 Jun 2026 at 06:37, Michael Paquier <michael@paquier.xyz> wrote:

3) An issue in StandbyAcquireAccessExclusiveLock()

<snip>

I'm not sure how to solve this correctly; I think ideally the
StandbyAcquireAccessExclusiveLock() hash code would be wrapped by a
critical section, but I'm not 100% sure if that will be a sufficient
approach; and it'd definitely need some code to allow the various
hashmaps' memctxs to alloc during critical sections.

Not checked this one yet.

I found that the attached patch v3 solves that issue. The assert fires
because we link the lock into the transaction's exclusive locks ahead
of actually having acquired the lock, and when that lock acquisition
fails, as part of the error handling we hit
StartupProcExit->ShutdownRecoveryTransactionEnvironment->StandbyReleaseAllLocks,
which causes this assertion failure because the lock was not taken by
this backend.

By moving StandbyAcquireAccessExclusiveLock's LockAcquire ahead of
when it links the lock to the transaction, the local data structure
doesn't know to clean up the lock until after it's acquired, so
failure in that process won't make error cleanup try to clean up the
lock.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Thu, Jun 18, 2026 at 05:27:57PM +0200, Matthias van de Meent wrote:

By moving StandbyAcquireAccessExclusiveLock's LockAcquire ahead of
when it links the lock to the transaction, the local data structure
doesn't know to clean up the lock until after it's acquired, so
failure in that process won't make error cleanup try to clean up the
lock.

Yep, reordering these two actions would take care of the list
inconsistency where the startup process goes down following the ERROR
promoted to a FATAL.

I have been fingering the idea of backpatching this fix for a few
minutes, actually, but discarded the idea at the end. It does not
require a random pattern to cause the failure, being actionable
through a combination of GUCs as Alexander has proved. Still, the
only consequence is an extra LOG entry telling that the lock is not
being tracked for non-assert builds. Confusing, OK, but not really
critical.

Comments?
--
Michael

On Thu, Jun 18, 2026 at 11:27:28AM +0200, Matthias van de Meent wrote:

Each of the calls to
CacheRegisterSyscacheCallback/CacheRegisterRelcacheCallback can throw
an ERROR when all slots have been used. This would leave the typcache
in an invalid state, so I think that must be wrapped in a critical
section: neither syscache nor relcache has options to release
callbacks, and we can't safely continue without the callbacks
installed, so once an error is thrown here this backend can't ever be
properly initialized. This is unlike OOMs, whose conditions for
failure may (and often do) change as workloads change in other
backends.

We don't ERROR when failing to register a syscache/relcache callback,
we FATAL if we reach one of the thresholds. Reaching these thresholds
points to me to a programming error anyway, so these should not matter
in the field. The OOM is a random pattern that can happen outside the
Postgres realm.

Just in case, I have planted a elog(FATAL) triggering randomly in the
middle of cache registration callback calls, and the typcache
inconsistency does not come in play with the shutdown sequence once
these trigger even if we have the tables set but not the callbacks.
As a whole, I tend to think that reordering the actions is a solution
good enough here.

I think Heikki's suggestion for a FATAL critical section option would
be a good alternative. It wouldn't always be sufficient, but would fix
issues here.

That sounds like an interesting idea, potentially reusable for other
areas, but I'm not really convinced that we need to add this kind of
facility for the case dealt with here. To me, that's also where we
could use a TRY/CATCH block and call it a day. If others feel
differently about this matter, I'm fine to be outvoted.
--
Michael

On Fri, 19 Jun 2026 at 01:55, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jun 18, 2026 at 11:27:28AM +0200, Matthias van de Meent wrote:

Each of the calls to
CacheRegisterSyscacheCallback/CacheRegisterRelcacheCallback can throw
an ERROR when all slots have been used. This would leave the typcache
in an invalid state, so I think that must be wrapped in a critical
section: neither syscache nor relcache has options to release
callbacks, and we can't safely continue without the callbacks
installed, so once an error is thrown here this backend can't ever be
properly initialized. This is unlike OOMs, whose conditions for
failure may (and often do) change as workloads change in other
backends.

We don't ERROR when failing to register a syscache/relcache callback,
we FATAL if we reach one of the thresholds.

Ah, thanks for correcting me. I'm not sure why I had ERROR in mind,
but you're obviously correct. Your patch v2 LGTM.

Reaching these thresholds
points to me to a programming error anyway, so these should not matter
in the field.

I don't think that's (necessarily) correct. These callbacks are
accessible to extensions, and if you load sufficiently many of those
you could still run out of slots even if each extension stayed well
within a reasonable threshold.

I think Heikki's suggestion for a FATAL critical section option would
be a good alternative. It wouldn't always be sufficient, but would fix
issues here.

That sounds like an interesting idea, potentially reusable for other
areas, but I'm not really convinced that we need to add this kind of
facility for the case dealt with here. To me, that's also where we
could use a TRY/CATCH block and call it a day. If others feel
differently about this matter, I'm fine to be outvoted.

While TRY/CATCH would work, I'm not so keen on adding those to
system-initalizing code; the allocations generally go into contexts
that aren't cleaned up nicely during error handling. E.g. a partial
hash initialization for any of the cache hashmaps due to OOM will leak
its allocations.
Failing the connection for that makes sure we complete the right
cleanup procedures and not leak those resources (and it adds another
item to the multi-threading concerns list).

-Matthias

On Fri, 19 Jun 2026 at 01:30, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jun 18, 2026 at 05:27:57PM +0200, Matthias van de Meent wrote:

By moving StandbyAcquireAccessExclusiveLock's LockAcquire ahead of
when it links the lock to the transaction, the local data structure
doesn't know to clean up the lock until after it's acquired, so
failure in that process won't make error cleanup try to clean up the
lock.

Yep, reordering these two actions would take care of the list
inconsistency where the startup process goes down following the ERROR
promoted to a FATAL.

I have been fingering the idea of backpatching this fix for a few
minutes, actually, but discarded the idea at the end. It does not
require a random pattern to cause the failure, being actionable
through a combination of GUCs as Alexander has proved. Still, the
only consequence is an extra LOG entry telling that the lock is not
being tracked for non-assert builds. Confusing, OK, but not really
critical.

Comments?

Because it fixes an assertion, I'd vote for backpatching; but because
the case is handled safely without assertions I also wouldn't be upset
if it wasn't backpatched.

-Matthias