On 11/11/2023 14:00, Alexander Lakhin wrote:
10.11.2023 17:26, Heikki Linnakangas wrote:
I think that is surprising behavior from the DSA facility. When you make
allocations with dsa_allocate() or just call
dsa_get_address() on an existing dsa_pointer, you wouldn't expect the current
resource owner to matter. I think
dsa_create/attach() should store the current resource owner in the dsa_area,
for use in subsequent operations on the
DSA, per attached patch (0002-Fix-dsa.c-with-different-resource-owners.patch).
With the patch 0002 applied, I'm observing another anomaly:
Ok, so the ownership of a dsa was still muddled :-(. Attached is a new
version that goes a little further. It replaces the whole
'mapping_pinned' flag in dsa_area with the 'resowner'. When a mapping is
pinned, it means that 'resowner == NULL'. This is now similar to how the
resowner field and pinning works in dsm.c.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 90a0f1c8204f01c423b60be032ea521e4afd7473 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 10 Nov 2023 13:54:11 +0200
Subject: [PATCH 1/3] Add test_dsa module.
This covers basic calls within a single backend process, and a test
that uses a different resource owner. The resource owner test
demonstrates that calling dsa_allocate() or dsa_get_address() while in
a different resource owner than in the dsa_create/attach call causes a
segfault. I think that's a bug.
This resource owner issue was originally found by Alexander Lakhin,
exposed by my large ResourceOwner rewrite commit.
Reported-by: Alexander Lakhin
Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com
---
src/test/modules/Makefile | 1 +
src/test/modules/meson.build | 1 +
src/test/modules/test_dsa/Makefile | 23 ++++
.../modules/test_dsa/expected/test_dsa.out | 13 ++
src/test/modules/test_dsa/meson.build | 33 +++++
src/test/modules/test_dsa/sql/test_dsa.sql | 4 +
src/test/modules/test_dsa/test_dsa--1.0.sql | 12 ++
src/test/modules/test_dsa/test_dsa.c | 115 ++++++++++++++++++
src/test/modules/test_dsa/test_dsa.control | 4 +
9 files changed, 206 insertions(+)
create mode 100644 src/test/modules/test_dsa/Makefile
create mode 100644 src/test/modules/test_dsa/expected/test_dsa.out
create mode 100644 src/test/modules/test_dsa/meson.build
create mode 100644 src/test/modules/test_dsa/sql/test_dsa.sql
create mode 100644 src/test/modules/test_dsa/test_dsa--1.0.sql
create mode 100644 src/test/modules/test_dsa/test_dsa.c
create mode 100644 src/test/modules/test_dsa/test_dsa.control
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 738b715e792..a18e4d28a04 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
test_copy_callbacks \
test_custom_rmgrs \
test_ddl_deparse \
+ test_dsa \
test_extensions \
test_ginpostinglist \
test_integerset \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d4828dc44d5..4e83c0f8d74 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -14,6 +14,7 @@ subdir('test_bloomfilter')
subdir('test_copy_callbacks')
subdir('test_custom_rmgrs')
subdir('test_ddl_deparse')
+subdir('test_dsa')
subdir('test_extensions')
subdir('test_ginpostinglist')
subdir('test_integerset')
diff --git a/src/test/modules/test_dsa/Makefile b/src/test/modules/test_dsa/Makefile
new file mode 100644
index 00000000000..77583464dca
--- /dev/null
+++ b/src/test/modules/test_dsa/Makefile
@@ -0,0 +1,23 @@
+# src/test/modules/test_dsa/Makefile
+
+MODULE_big = test_dsa
+OBJS = \
+ $(WIN32RES) \
+ test_dsa.o
+PGFILEDESC = "test_dsa - test code for dynamic shared memory areas"
+
+EXTENSION = test_dsa
+DATA = test_dsa--1.0.sql
+
+REGRESS = test_dsa
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_dsa
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_dsa/expected/test_dsa.out b/src/test/modules/test_dsa/expected/test_dsa.out
new file mode 100644
index 00000000000..266010e77fe
--- /dev/null
+++ b/src/test/modules/test_dsa/expected/test_dsa.out
@@ -0,0 +1,13 @@
+CREATE EXTENSION test_dsa;
+SELECT test_dsa_basic();
+ test_dsa_basic
+----------------
+
+(1 row)
+
+SELECT test_dsa_resowners();
+ test_dsa_resowners
+--------------------
+
+(1 row)
+
diff --git a/src/test/modules/test_dsa/meson.build b/src/test/modules/test_dsa/meson.build
new file mode 100644
index 00000000000..21738290ad5
--- /dev/null
+++ b/src/test/modules/test_dsa/meson.build
@@ -0,0 +1,33 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+test_dsa_sources = files(
+ 'test_dsa.c',
+)
+
+if host_system == 'windows'
+ test_dsa_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+ '--NAME', 'test_dsa',
+ '--FILEDESC', 'test_dsa - test code for dynamic shared memory areas',])
+endif
+
+test_dsa = shared_module('test_dsa',
+ test_dsa_sources,
+ kwargs: pg_test_mod_args,
+)
+test_install_libs += test_dsa
+
+test_install_data += files(
+ 'test_dsa.control',
+ 'test_dsa--1.0.sql',
+)
+
+tests += {
+ 'name': 'test_dsa',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'test_dsa',
+ ],
+ },
+}
diff --git a/src/test/modules/test_dsa/sql/test_dsa.sql b/src/test/modules/test_dsa/sql/test_dsa.sql
new file mode 100644
index 00000000000..c3d8db94372
--- /dev/null
+++ b/src/test/modules/test_dsa/sql/test_dsa.sql
@@ -0,0 +1,4 @@
+CREATE EXTENSION test_dsa;
+
+SELECT test_dsa_basic();
+SELECT test_dsa_resowners();
diff --git a/src/test/modules/test_dsa/test_dsa--1.0.sql b/src/test/modules/test_dsa/test_dsa--1.0.sql
new file mode 100644
index 00000000000..2904cb23525
--- /dev/null
+++ b/src/test/modules/test_dsa/test_dsa--1.0.sql
@@ -0,0 +1,12 @@
+/* src/test/modules/test_dsa/test_dsa--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_dsa" to load this file. \quit
+
+CREATE FUNCTION test_dsa_basic()
+ RETURNS pg_catalog.void
+ AS 'MODULE_PATHNAME' LANGUAGE C;
+
+CREATE FUNCTION test_dsa_resowners()
+ 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
new file mode 100644
index 00000000000..b3753b58577
--- /dev/null
+++ b/src/test/modules/test_dsa/test_dsa.c
@@ -0,0 +1,115 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_dsa.c
+ * Test dynamic shared memory areas (DSAs)
+ *
+ * Copyright (c) 2022-2023, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * src/test/modules/test_dsa/test_dsa.c
+ *
+ * -------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "utils/dsa.h"
+#include "storage/lwlock.h"
+#include "utils/resowner.h"
+
+PG_MODULE_MAGIC;
+
+/* Test basic DSA functionality */
+PG_FUNCTION_INFO_V1(test_dsa_basic);
+Datum
+test_dsa_basic(PG_FUNCTION_ARGS)
+{
+ int tranche_id;
+ dsa_area *a;
+ dsa_pointer p[100];
+
+ /* 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);
+
+ PG_RETURN_VOID();
+}
+
+/* Test using DSA across different resource owners */
+PG_FUNCTION_INFO_V1(test_dsa_resowners);
+Datum
+test_dsa_resowners(PG_FUNCTION_ARGS)
+{
+ int tranche_id;
+ dsa_area *a;
+ dsa_pointer p[10000];
+ ResourceOwner oldowner;
+ ResourceOwner childowner;
+
+ /* XXX: this tranche is leaked */
+ tranche_id = LWLockNewTrancheId();
+ LWLockRegisterTranche(tranche_id, "test_dsa");
+
+ /* Create DSA in parent resource owner */
+ a = dsa_create(tranche_id);
+
+ /* Switch to child resource owner, and do a bunch of allocations in the
+ * DSA */
+ oldowner = CurrentResourceOwner;
+ childowner = ResourceOwnerCreate(oldowner, "test_dsa temp owner");
+ CurrentResourceOwner = childowner;
+
+ for (int i = 0; i < 10000; i++)
+ {
+ p[i] = dsa_allocate(a, 1000);
+ snprintf(dsa_get_address(a, p[i]), 1000, "foobar%d", i);
+ }
+
+ /*
+ * Free them all. Not necessary, but demonstrates that you get a crash
+ * even if you free all the allocations that you made while in the child
+ * resource owner. The allocations are not "owned" by the resource owner.
+ */
+ for (int i = 0; i < 100; i++)
+ dsa_free(a, p[i]);
+
+ /* Release the child resource owner */
+ CurrentResourceOwner = oldowner;
+ ResourceOwnerRelease(childowner,
+ RESOURCE_RELEASE_BEFORE_LOCKS,
+ true, false);
+ ResourceOwnerRelease(childowner,
+ RESOURCE_RELEASE_LOCKS,
+ true, false);
+ ResourceOwnerRelease(childowner,
+ RESOURCE_RELEASE_AFTER_LOCKS,
+ true, false);
+ ResourceOwnerDelete(childowner);
+
+ dsa_detach(a);
+
+ PG_RETURN_VOID();
+}
diff --git a/src/test/modules/test_dsa/test_dsa.control b/src/test/modules/test_dsa/test_dsa.control
new file mode 100644
index 00000000000..ac9674b2193
--- /dev/null
+++ b/src/test/modules/test_dsa/test_dsa.control
@@ -0,0 +1,4 @@
+comment = 'Test code for dynamic shared memory areas'
+default_version = '1.0'
+module_pathname = '$libdir/test_dsa'
+relocatable = true
--
2.39.2
From 8ef5c0f197f0a095a19b4642ad94c56422e40d40 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sun, 12 Nov 2023 23:10:28 +0100
Subject: [PATCH 2/3] Fix dsa.c with different resource owners.
The comments in dsa.c suggested that areas were owned by resource
owners, but it was not in fact tracked explicitly. The DSM attachments
held by the dsa were owned by resource owners, but not the area
itself. That led to confusion if you used one resource owner to attach
or create the area, but then switched to a different resource owner
when allocating or even just accessing the allocations in the area
with dsa_get_address(). The additional DSM segments associated with
the area would get owned by a different resource owner than the
initial segment. To fix, add an explicit 'resowner' field to dsa_area.
It replaces the 'mapping_pinned' flag; resowner == NULL now indicates
that the mapping is pinned.
This fixes the bug demonstrated by the test case from previous commit.
Reviewed-by: Alexander Lakhin <exclus...@gmail.com>
Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com
---
src/backend/utils/mmgr/dsa.c | 38 ++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 8d1aace40ac..71cf11b1894 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -59,6 +59,7 @@
#include "utils/dsa.h"
#include "utils/freepage.h"
#include "utils/memutils.h"
+#include "utils/resowner.h"
/*
* The size of the initial DSM segment that backs a dsa_area created by
@@ -368,8 +369,13 @@ struct dsa_area
/* Pointer to the control object in shared memory. */
dsa_area_control *control;
- /* Has the mapping been pinned? */
- bool mapping_pinned;
+ /*
+ * All the mappings are owned by this. The dsa_area itself is not
+ * directly tracked by the ResourceOwner, but the effect is the same.
+ * NULL if the attachment has session lifespan, i.e if dsa_pin_mapping()
+ * has been called.
+ */
+ ResourceOwner resowner;
/*
* This backend's array of segment maps, ordered by segment index
@@ -645,12 +651,14 @@ dsa_pin_mapping(dsa_area *area)
{
int i;
- Assert(!area->mapping_pinned);
- area->mapping_pinned = true;
+ if (area->resowner != NULL)
+ {
+ area->resowner = NULL;
- for (i = 0; i <= area->high_segment_index; ++i)
- if (area->segment_maps[i].segment != NULL)
- dsm_pin_mapping(area->segment_maps[i].segment);
+ for (i = 0; i <= area->high_segment_index; ++i)
+ if (area->segment_maps[i].segment != NULL)
+ dsm_pin_mapping(area->segment_maps[i].segment);
+ }
}
/*
@@ -1264,7 +1272,7 @@ create_internal(void *place, size_t size,
*/
area = palloc(sizeof(dsa_area));
area->control = control;
- area->mapping_pinned = false;
+ area->resowner = CurrentResourceOwner;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1320,7 +1328,7 @@ 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->mapping_pinned = false;
+ area->resowner = CurrentResourceOwner;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
@@ -1743,6 +1751,7 @@ get_segment_by_index(dsa_area *area, dsa_segment_index index)
dsm_handle handle;
dsm_segment *segment;
dsa_segment_map *segment_map;
+ ResourceOwner oldowner;
/*
* If we are reached by dsa_free or dsa_get_address, there must be at
@@ -1761,11 +1770,12 @@ get_segment_by_index(dsa_area *area, dsa_segment_index index)
elog(ERROR,
"dsa_area could not attach to a segment that has been freed");
+ oldowner = CurrentResourceOwner;
+ CurrentResourceOwner = area->resowner;
segment = dsm_attach(handle);
+ CurrentResourceOwner = oldowner;
if (segment == NULL)
elog(ERROR, "dsa_area could not attach to segment");
- if (area->mapping_pinned)
- dsm_pin_mapping(segment);
segment_map = &area->segment_maps[index];
segment_map->segment = segment;
segment_map->mapped_address = dsm_segment_address(segment);
@@ -2067,6 +2077,7 @@ make_new_segment(dsa_area *area, size_t requested_pages)
size_t usable_pages;
dsa_segment_map *segment_map;
dsm_segment *segment;
+ ResourceOwner oldowner;
Assert(LWLockHeldByMe(DSA_AREA_LOCK(area)));
@@ -2151,12 +2162,13 @@ make_new_segment(dsa_area *area, size_t requested_pages)
}
/* Create the segment. */
+ oldowner = CurrentResourceOwner;
+ CurrentResourceOwner = area->resowner;
segment = dsm_create(total_size, 0);
+ CurrentResourceOwner = oldowner;
if (segment == NULL)
return NULL;
dsm_pin_segment(segment);
- if (area->mapping_pinned)
- dsm_pin_mapping(segment);
/* Store the handle in shared memory to be found by index. */
area->control->segment_handles[new_index] =
--
2.39.2
From 64536b950d003292658be77a16d09662ae440eab Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sun, 12 Nov 2023 23:11:14 +0100
Subject: [PATCH 3/3] Clear CurrentResourceOwner earlier in CommitTransaction.
Alexander reported a crash with repeated create + drop database, after
the ResourceOwner rewrite (commit b8bff07daa). That was fixed by the
previous commit, but it nevertheless seems like a good idea clear
CurrentResourceOwner earlier, because you're not supposed to use it
for anything after we start releasing it.
Reported-by: Alexander Lakhin
Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com
---
src/backend/access/transam/xact.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 74ce5f9491c..8fad8ffa1eb 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2309,6 +2309,7 @@ CommitTransaction(void)
CallXactCallbacks(is_parallel_worker ? XACT_EVENT_PARALLEL_COMMIT
: XACT_EVENT_COMMIT);
+ CurrentResourceOwner = NULL;
ResourceOwnerRelease(TopTransactionResourceOwner,
RESOURCE_RELEASE_BEFORE_LOCKS,
true, true);
@@ -2374,7 +2375,6 @@ CommitTransaction(void)
AtEOXact_LogicalRepWorkers(true);
pgstat_report_xact_timestamp(0);
- CurrentResourceOwner = NULL;
ResourceOwnerDelete(TopTransactionResourceOwner);
s->curTransactionOwner = NULL;
CurTransactionResourceOwner = NULL;
--
2.39.2
