From 8cf1c9606fba2dacb69173727f8567e94298fd2f Mon Sep 17 00:00:00 2001 From: Rahila Syed Date: Tue, 14 Jan 2025 11:30:36 +0530 Subject: [PATCH] Prevent the error on creating a dsm segment from an interrupt handler. If DSA or DSM segment is attached to or created while a transaction's resource owner is being released, it will throw an error. Check whether the resource owner is being released before adding the segment to that resource owner. --- src/backend/storage/ipc/dsm.c | 29 +++++++--- src/backend/utils/mmgr/dsa.c | 10 +++- src/backend/utils/resowner/resowner.c | 11 ++++ src/include/utils/resowner.h | 1 + .../modules/test_dsa/expected/test_dsa.out | 10 ++++ src/test/modules/test_dsa/sql/test_dsa.sql | 5 ++ src/test/modules/test_dsa/test_dsa--1.0.sql | 4 ++ src/test/modules/test_dsa/test_dsa.c | 53 +++++++++++++++++++ 8 files changed, 113 insertions(+), 10 deletions(-) diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c index f92a52a00e..53d5d1dd78 100644 --- a/src/backend/storage/ipc/dsm.c +++ b/src/backend/storage/ipc/dsm.c @@ -934,9 +934,20 @@ void dsm_unpin_mapping(dsm_segment *seg) { Assert(seg->resowner == NULL); - ResourceOwnerEnlarge(CurrentResourceOwner); - seg->resowner = CurrentResourceOwner; - ResourceOwnerRememberDSM(seg->resowner, seg); + + if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner)) + { + ResourceOwnerEnlarge(CurrentResourceOwner); + seg->resowner = CurrentResourceOwner; + ResourceOwnerRememberDSM(seg->resowner, seg); + } + else + { + /* Log failure of unpinning */ + elog(DEBUG2, "unable to unpin the segment %u as CurrentResourceOwner is NULL or releasing", + seg); + seg->resowner = NULL; + } } /* @@ -1202,9 +1213,6 @@ dsm_create_descriptor(void) { dsm_segment *seg; - if (CurrentResourceOwner) - ResourceOwnerEnlarge(CurrentResourceOwner); - seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment)); dlist_push_head(&dsm_segment_list, &seg->node); @@ -1214,9 +1222,14 @@ dsm_create_descriptor(void) seg->mapped_address = NULL; seg->mapped_size = 0; - seg->resowner = CurrentResourceOwner; - if (CurrentResourceOwner) + if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner)) + { + ResourceOwnerEnlarge(CurrentResourceOwner); + seg->resowner = CurrentResourceOwner; ResourceOwnerRememberDSM(CurrentResourceOwner, seg); + } + else + seg->resowner = NULL; slist_init(&seg->on_detach); diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c index 17d4f7a7a0..7b2c5a4015 100644 --- a/src/backend/utils/mmgr/dsa.c +++ b/src/backend/utils/mmgr/dsa.c @@ -1282,7 +1282,10 @@ create_internal(void *place, size_t size, */ area = palloc(sizeof(dsa_area)); area->control = control; - area->resowner = CurrentResourceOwner; + if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner)) + area->resowner = CurrentResourceOwner; + else + area->resowner = NULL; memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS); area->high_segment_index = 0; area->freed_segment_counter = 0; @@ -1338,7 +1341,10 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle) /* Build the backend-local area object. */ area = palloc(sizeof(dsa_area)); area->control = control; - area->resowner = CurrentResourceOwner; + if (CurrentResourceOwner && !IsResourceOwnerReleasing(CurrentResourceOwner)) + area->resowner = CurrentResourceOwner; + else + area->resowner = NULL; memset(&area->segment_maps[0], 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS); area->high_segment_index = 0; diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c index d39f3e1b65..fe506a7451 100644 --- a/src/backend/utils/resowner/resowner.c +++ b/src/backend/utils/resowner/resowner.c @@ -52,6 +52,7 @@ #include "storage/ipc.h" #include "storage/predicate.h" #include "storage/proc.h" +#include "utils/injection_point.h" #include "utils/memutils.h" #include "utils/resowner.h" @@ -702,6 +703,7 @@ ResourceOwnerReleaseInternal(ResourceOwner owner, Assert(phase == RESOURCE_RELEASE_BEFORE_LOCKS); Assert(!owner->sorted); owner->releasing = true; + INJECTION_POINT("dsa_create_on_res_release"); } else { @@ -1111,3 +1113,12 @@ ResourceOwnerForgetAioHandle(ResourceOwner owner, struct dlist_node *ioh_node) { dlist_delete_from(&owner->aio_handles, ioh_node); } + +/* + * Returns true if resource owner is being released + */ +bool +IsResourceOwnerReleasing(ResourceOwner owner) +{ + return (owner->releasing); +} diff --git a/src/include/utils/resowner.h b/src/include/utils/resowner.h index aede4bfc82..7b4c3765a1 100644 --- a/src/include/utils/resowner.h +++ b/src/include/utils/resowner.h @@ -163,6 +163,7 @@ extern void ReleaseAuxProcessResources(bool isCommit); struct LOCALLOCK; extern void ResourceOwnerRememberLock(ResourceOwner owner, struct LOCALLOCK *locallock); extern void ResourceOwnerForgetLock(ResourceOwner owner, struct LOCALLOCK *locallock); +extern bool IsResourceOwnerReleasing(ResourceOwner owner); /* special support for AIO */ struct dlist_node; diff --git a/src/test/modules/test_dsa/expected/test_dsa.out b/src/test/modules/test_dsa/expected/test_dsa.out index 266010e77f..9c67fb0c0e 100644 --- a/src/test/modules/test_dsa/expected/test_dsa.out +++ b/src/test/modules/test_dsa/expected/test_dsa.out @@ -11,3 +11,13 @@ SELECT test_dsa_resowners(); (1 row) +SELECT test_dsa_injection('dsa_create_on_res_release'); + test_dsa_injection +-------------------- + +(1 row) + +begin; +select 1/0; +ERROR: division by zero +commit; diff --git a/src/test/modules/test_dsa/sql/test_dsa.sql b/src/test/modules/test_dsa/sql/test_dsa.sql index c3d8db9437..a06ae84594 100644 --- a/src/test/modules/test_dsa/sql/test_dsa.sql +++ b/src/test/modules/test_dsa/sql/test_dsa.sql @@ -2,3 +2,8 @@ CREATE EXTENSION test_dsa; SELECT test_dsa_basic(); SELECT test_dsa_resowners(); +SELECT test_dsa_injection('dsa_create_on_res_release'); + +begin; +select 1/0; +commit; diff --git a/src/test/modules/test_dsa/test_dsa--1.0.sql b/src/test/modules/test_dsa/test_dsa--1.0.sql index 2904cb2352..4f0e655667 100644 --- a/src/test/modules/test_dsa/test_dsa--1.0.sql +++ b/src/test/modules/test_dsa/test_dsa--1.0.sql @@ -10,3 +10,7 @@ CREATE FUNCTION test_dsa_basic() CREATE FUNCTION test_dsa_resowners() RETURNS pg_catalog.void AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_dsa_injection(IN point_name TEXT) + RETURNS pg_catalog.void + AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_dsa/test_dsa.c b/src/test/modules/test_dsa/test_dsa.c index cd24d0f487..2505139241 100644 --- a/src/test/modules/test_dsa/test_dsa.c +++ b/src/test/modules/test_dsa/test_dsa.c @@ -14,10 +14,14 @@ #include "fmgr.h" #include "storage/lwlock.h" +#include "utils/builtins.h" #include "utils/dsa.h" +#include "utils/injection_point.h" #include "utils/resowner.h" PG_MODULE_MAGIC; +extern PGDLLEXPORT void test_dsa_inj(const char *name, + const void *private_data); /* Test basic DSA functionality */ PG_FUNCTION_INFO_V1(test_dsa_basic); @@ -111,3 +115,52 @@ test_dsa_resowners(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } + +PG_FUNCTION_INFO_V1(test_dsa_injection); +Datum +test_dsa_injection(PG_FUNCTION_ARGS) +{ + char *name = text_to_cstring(PG_GETARG_TEXT_PP(0)); + char *function = "test_dsa_inj"; + + InjectionPointAttach(name, "test_dsa", function, NULL, + 0); + + PG_RETURN_VOID(); +} + +void +test_dsa_inj(const char *name, const void *private_data) +{ + dsa_area *a; + dsa_pointer p[100]; + int tranche_id; + + /* XXX: this tranche is leaked */ + tranche_id = LWLockNewTrancheId(); + LWLockRegisterTranche(tranche_id, "test_dsa"); + + a = dsa_create(tranche_id); + for (int i = 0; i < 100; i++) + { + p[i] = dsa_allocate(a, 1000); + snprintf(dsa_get_address(a, p[i]), 1000, "foobar%d", i); + } + + for (int i = 0; i < 100; i++) + { + char buf[100]; + + snprintf(buf, 100, "foobar%d", i); + if (strcmp(dsa_get_address(a, p[i]), buf) != 0) + elog(ERROR, "no match"); + } + + for (int i = 0; i < 100; i++) + { + dsa_free(a, p[i]); + } + + dsa_detach(a); + return; +} -- 2.34.1