Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit a8b330f

Browse files
committed
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 before 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 is arguably a bug fix, but I'm not backpatching because it doesn't seem to be a live bug in the back branches. In 'master', it is a bug because commit b8bff07 made ResourceOwners more strict so that you are no longer allowed to remember new resources in a ResourceOwner after you have started to release it. Merely accessing a dsa pointer might need to attach a new DSM segment, and before this commit it was temporarily remembered in the current owner for a very brief period even if the DSA was pinned. And that could happen in AtEOXact_PgStat(), which is called after the owner is already released. Reported-by: Alexander Lakhin Reviewed-by: Alexander Lakhin, Thomas Munro, Andres Freund Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com
1 parent f26c236 commit a8b330f

File tree

1 file changed

+25
-13
lines changed
  • src/backend/utils/mmgr

1 file changed

+25
-13
lines changed

src/backend/utils/mmgr/dsa.c

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
#include "utils/dsa.h"
6060
#include "utils/freepage.h"
6161
#include "utils/memutils.h"
62+
#include "utils/resowner.h"
6263

6364
/*
6465
* The size of the initial DSM segment that backs a dsa_area created by
@@ -368,8 +369,13 @@ struct dsa_area
368369
/* Pointer to the control object in shared memory. */
369370
dsa_area_control *control;
370371

371-
/* Has the mapping been pinned? */
372-
bool mapping_pinned;
372+
/*
373+
* All the mappings are owned by this. The dsa_area itself is not
374+
* directly tracked by the ResourceOwner, but the effect is the same. NULL
375+
* if the attachment has session lifespan, i.e if dsa_pin_mapping() has
376+
* been called.
377+
*/
378+
ResourceOwner resowner;
373379

374380
/*
375381
* This backend's array of segment maps, ordered by segment index
@@ -645,12 +651,14 @@ dsa_pin_mapping(dsa_area *area)
645651
{
646652
int i;
647653

648-
Assert(!area->mapping_pinned);
649-
area->mapping_pinned = true;
654+
if (area->resowner != NULL)
655+
{
656+
area->resowner = NULL;
650657

651-
for (i = 0; i <= area->high_segment_index; ++i)
652-
if (area->segment_maps[i].segment != NULL)
653-
dsm_pin_mapping(area->segment_maps[i].segment);
658+
for (i = 0; i <= area->high_segment_index; ++i)
659+
if (area->segment_maps[i].segment != NULL)
660+
dsm_pin_mapping(area->segment_maps[i].segment);
661+
}
654662
}
655663

656664
/*
@@ -1264,7 +1272,7 @@ create_internal(void *place, size_t size,
12641272
*/
12651273
area = palloc(sizeof(dsa_area));
12661274
area->control = control;
1267-
area->mapping_pinned = false;
1275+
area->resowner = CurrentResourceOwner;
12681276
memset(area->segment_maps, 0, sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
12691277
area->high_segment_index = 0;
12701278
area->freed_segment_counter = 0;
@@ -1320,7 +1328,7 @@ attach_internal(void *place, dsm_segment *segment, dsa_handle handle)
13201328
/* Build the backend-local area object. */
13211329
area = palloc(sizeof(dsa_area));
13221330
area->control = control;
1323-
area->mapping_pinned = false;
1331+
area->resowner = CurrentResourceOwner;
13241332
memset(&area->segment_maps[0], 0,
13251333
sizeof(dsa_segment_map) * DSA_MAX_SEGMENTS);
13261334
area->high_segment_index = 0;
@@ -1743,6 +1751,7 @@ get_segment_by_index(dsa_area *area, dsa_segment_index index)
17431751
dsm_handle handle;
17441752
dsm_segment *segment;
17451753
dsa_segment_map *segment_map;
1754+
ResourceOwner oldowner;
17461755

17471756
/*
17481757
* 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)
17611770
elog(ERROR,
17621771
"dsa_area could not attach to a segment that has been freed");
17631772

1773+
oldowner = CurrentResourceOwner;
1774+
CurrentResourceOwner = area->resowner;
17641775
segment = dsm_attach(handle);
1776+
CurrentResourceOwner = oldowner;
17651777
if (segment == NULL)
17661778
elog(ERROR, "dsa_area could not attach to segment");
1767-
if (area->mapping_pinned)
1768-
dsm_pin_mapping(segment);
17691779
segment_map = &area->segment_maps[index];
17701780
segment_map->segment = segment;
17711781
segment_map->mapped_address = dsm_segment_address(segment);
@@ -2067,6 +2077,7 @@ make_new_segment(dsa_area *area, size_t requested_pages)
20672077
size_t usable_pages;
20682078
dsa_segment_map *segment_map;
20692079
dsm_segment *segment;
2080+
ResourceOwner oldowner;
20702081

20712082
Assert(LWLockHeldByMe(DSA_AREA_LOCK(area)));
20722083

@@ -2151,12 +2162,13 @@ make_new_segment(dsa_area *area, size_t requested_pages)
21512162
}
21522163

21532164
/* Create the segment. */
2165+
oldowner = CurrentResourceOwner;
2166+
CurrentResourceOwner = area->resowner;
21542167
segment = dsm_create(total_size, 0);
2168+
CurrentResourceOwner = oldowner;
21552169
if (segment == NULL)
21562170
return NULL;
21572171
dsm_pin_segment(segment);
2158-
if (area->mapping_pinned)
2159-
dsm_pin_mapping(segment);
21602172

21612173
/* Store the handle in shared memory to be found by index. */
21622174
area->control->segment_handles[new_index] =

0 commit comments

Comments
 (0)