Thanks for the testing again!
On 10/11/2023 11:00, Alexander Lakhin wrote:
I could see two failure modes:
2023-11-10 08:42:28.870 UTC [1163274] ERROR: ResourceOwnerEnlarge called after
release started
2023-11-10 08:42:28.870 UTC [1163274] STATEMENT: drop table t;
2023-11-10 08:42:28.870 UTC [1163274] WARNING: AbortTransaction while in
COMMIT state
2023-11-10 08:42:28.870 UTC [1163274] PANIC: cannot abort transaction 906, it
was already committed
2023-11-10 08:43:27.897 UTC [1164148] ERROR: ResourceOwnerEnlarge called after
release started
2023-11-10 08:43:27.897 UTC [1164148] STATEMENT: DROP DATABASE db69;
2023-11-10 08:43:27.897 UTC [1164148] WARNING: AbortTransaction while in
COMMIT state
2023-11-10 08:43:27.897 UTC [1164148] PANIC: cannot abort transaction 1043, it
was already committed
The stack trace for the second ERROR (ResourceOwnerEnlarge called ...) is:
...
#6 0x0000558af5b2f35c in ResourceOwnerEnlarge (owner=0x558af716f3c8) at
resowner.c:455
#7 0x0000558af5888f18 in dsm_create_descriptor () at dsm.c:1207
#8 0x0000558af5889205 in dsm_attach (h=3172038420) at dsm.c:697
#9 0x0000558af5b1ebed in get_segment_by_index (area=0x558af711da18, index=2)
at dsa.c:1764
#10 0x0000558af5b1ea4b in dsa_get_address (area=0x558af711da18,
dp=2199023329568) at dsa.c:970
#11 0x0000558af5669366 in dshash_seq_next (status=0x7ffdd5912fd0) at
dshash.c:687
#12 0x0000558af5901998 in pgstat_drop_database_and_contents (dboid=16444) at
pgstat_shmem.c:830
#13 0x0000558af59016f0 in pgstat_drop_entry (kind=PGSTAT_KIND_DATABASE,
dboid=16444, objoid=0) at pgstat_shmem.c:888
#14 0x0000558af59044eb in AtEOXact_PgStat_DroppedStats
(xact_state=0x558af7111ee0, isCommit=true) at pgstat_xact.c:88
#15 0x0000558af59043c7 in AtEOXact_PgStat (isCommit=true, parallel=false) at
pgstat_xact.c:55
#16 0x0000558af53c782e in CommitTransaction () at xact.c:2371
#17 0x0000558af53c709e in CommitTransactionCommand () at xact.c:306
...
The quick, straightforward fix is to move the "CurrentResourceOwner =
NULL" line earlier in CommitTransaction, per attached
0003-Clear-CurrentResourceOwner-earlier-in-CommitTransact.patch. You're
not allowed to use the resource owner after you start to release it; it
was a bit iffy even before the ResourceOwner rewrite but now it's
explicitly forbidden. By clearing CurrentResourceOwner as soon as we
start releasing it, we can prevent any accidental use.
When CurrentResourceOwner == NULL, dsm_attach() returns a handle that's
not associated with any ResourceOwner. That's appropriate for the pgstat
case. The DSA is "pinned", so the handle is forgotten from the
ResourceOwner right after calling dsm_attach() anyway.
Looking closer at dsa.c, I think this is a wider problem though. The
comments don't make it very clear how it's supposed to interact with
ResourceOwners. There's just this brief comment in dsa_pin_mapping():
* By default, areas are owned by the current resource owner, which means they
* are detached automatically when that scope ends.
The dsa_area struct isn't directly owned by any ResourceOwner though.
The DSM segments created by dsa_create() or dsa_attach() are.
But the functions dsa_allocate() and dsa_get_address() can create or
attach more DSM segments to the area, and they will be owned by the by
the current resource owner *at the time of the call*. So if you call
dsa_get_address() while in a different resource owner, things get very
confusing. The attached new test module demonstrates that
(0001-Add-test_dsa-module.patch), here's a shortened version:
a = dsa_create(tranche_id);
/* Switch to new resource owner */
oldowner = CurrentResourceOwner;
childowner = ResourceOwnerCreate(oldowner, "temp owner");
CurrentResourceOwner = childowner;
/* make a bunch of allocations */
for (int i = 0; i < 10000; i++)
p[i] = dsa_allocate(a, 1000);
/* 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);
This first prints warnings on The ResourceOwnerRelease() calls:
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 2395813396
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 3922992700
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 1155452762
2023-11-10 13:57:21.475 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 4045183168
2023-11-10 13:57:21.476 EET [745346] WARNING: resource was not closed:
dynamic shared memory segment 1529990480
And a segfault at the dsm_detach() call:
2023-11-10 13:57:21.480 EET [745246] LOG: server process (PID 745346)
was terminated by signal 11: Segmentation fault
#0 0x000055a5148f64ee in slist_pop_head_node (head=0x55a516d06bf8) at
../../../../src/include/lib/ilist.h:1034
#1 0x000055a5148f5eba in dsm_detach (seg=0x55a516d06bc0) at dsm.c:822
#2 0x000055a514b86db0 in dsa_detach (area=0x55a516d64dc8) at dsa.c:1939
#3 0x00007fd5dcaee5e0 in test_dsa_resowners (fcinfo=0x55a516d56a28) at
test_dsa.c:112
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).
--
Heikki Linnakangas
Neon (https://neon.tech)
From 09ae4ae0fef8156da0f0026aadcd67307e1dc6f6 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 39af16dbd035047572198ca6e8631a5306d4879b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 10 Nov 2023 13:51:56 +0200
Subject: [PATCH 2/3] Fix dsa.c with different resource owners.
This fixes the bug with mixing different resource owners, demonstrated
by the test case from previous commit.
Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com
---
src/backend/utils/mmgr/dsa.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 8d1aace40ac..c32bb58618b 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
@@ -371,6 +372,13 @@ struct dsa_area
/* 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. Can be NULL
+ * for an area that's held open for session lifetime.
+ */
+ ResourceOwner owner;
+
/*
* This backend's array of segment maps, ordered by segment index
* corresponding to control->segment_handles. Some of the area's segments
@@ -1265,6 +1273,7 @@ create_internal(void *place, size_t size,
area = palloc(sizeof(dsa_area));
area->control = control;
area->mapping_pinned = false;
+ area->owner = CurrentResourceOwner;
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
area->freed_segment_counter = 0;
@@ -1321,6 +1330,7 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
area = palloc(sizeof(dsa_area));
area->control = control;
area->mapping_pinned = false;
+ area->owner = CurrentResourceOwner;
memset(&area->segment_maps[0], 0,
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
area->high_segment_index = 0;
@@ -1743,6 +1753,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,7 +1772,10 @@ 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->owner;
segment = dsm_attach(handle);
+ CurrentResourceOwner = oldowner;
if (segment == NULL)
elog(ERROR, "dsa_area could not attach to segment");
if (area->mapping_pinned)
@@ -2067,6 +2081,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,7 +2166,10 @@ make_new_segment(dsa_area *area, size_t requested_pages)
}
/* Create the segment. */
+ oldowner = CurrentResourceOwner;
+ CurrentResourceOwner = area->owner;
segment = dsm_create(total_size, 0);
+ CurrentResourceOwner = oldowner;
if (segment == NULL)
return NULL;
dsm_pin_segment(segment);
--
2.39.2
From f5fbb84504e336e99c1d9bc4fb38e7564d220799 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 10 Nov 2023 14:30:54 +0200
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
