On 2013-11-04 13:48:32 +0100, Andres Freund wrote:
> What about just unowning the smgr entry with
> if (rel->rd_smgr != NULL)
> smgrsetowner(NULL, rel->rd_smgr)
> when closing the fake relcache entry?
>
> That shouldn't require any further changes than changing the comment in
> smgrsetowner() that this isn't something expected to frequently happen.
Attached is a patch doing it like that, it required slightmy more
invasive changes than that. With the patch applied we survive replay of
a primary's make check run under valgrind without warnings.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 849559b9f1d0c20ea938d1dd13b37b53ebede856 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 5 Nov 2013 17:23:09 +0100
Subject: [PATCH] Un-own SMgrRelations in FreeFakeRelcacheEntry().
The previous coding left SMgrRelation->smgr_owner point into free'd
memory after FreeFakeRelcacheEntry() was executed which then was
accessed and written to when commit messages including smgr
invalidations or dropped relations where replayed during crash
recovery or physical replication.
Due to the memory allocation patterns during recovery this usually
didn't cause big problems but was noticed using valgrind.
---
src/backend/access/transam/xlogutils.c | 3 +++
src/backend/storage/smgr/smgr.c | 41 ++++++++++++++++++++++++++++++----
src/include/storage/smgr.h | 1 +
3 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index 5429d5e..94ddd78 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -433,6 +433,9 @@ CreateFakeRelcacheEntry(RelFileNode rnode)
void
FreeFakeRelcacheEntry(Relation fakerel)
{
+ /* make sure the fakerel is not referenced anymore */
+ if (fakerel->rd_smgr != NULL)
+ smgrunown(fakerel->rd_smgr);
pfree(fakerel);
}
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index f7f1437..c9a2a36 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -85,6 +85,7 @@ static SMgrRelation first_unowned_reln = NULL;
/* local function prototypes */
static void smgrshutdown(int code, Datum arg);
static void remove_from_unowned_list(SMgrRelation reln);
+static void add_to_unowned_list(SMgrRelation reln);
/*
@@ -174,9 +175,7 @@ smgropen(RelFileNode rnode, BackendId backend)
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
reln->md_fd[forknum] = NULL;
- /* place it at head of unowned list (to make smgrsetowner cheap) */
- reln->next_unowned_reln = first_unowned_reln;
- first_unowned_reln = reln;
+ add_to_unowned_list(reln);
}
return reln;
@@ -191,7 +190,7 @@ smgropen(RelFileNode rnode, BackendId backend)
void
smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
{
- /* We don't currently support "disowning" an SMgrRelation here */
+ /* We don't support "disowning" an SMgrRelation here */
Assert(owner != NULL);
/*
@@ -214,6 +213,25 @@ smgrsetowner(SMgrRelation *owner, SMgrRelation reln)
}
/*
+ * smgrunown() -- Remove long-lived reference to an SMgrRelation object if one
+ * exists
+ */
+void
+smgrunown(SMgrRelation reln)
+{
+ if (reln->smgr_owner == NULL)
+ return;
+
+ /* unset the owner's reference */
+ *(reln->smgr_owner) = NULL;
+
+ /* unset our reference to the owner*/
+ reln->smgr_owner = NULL;
+
+ add_to_unowned_list(reln);
+}
+
+/*
* remove_from_unowned_list -- unlink an SMgrRelation from the unowned list
*
* If the reln is not present in the list, nothing happens. Typically this
@@ -246,6 +264,21 @@ remove_from_unowned_list(SMgrRelation reln)
}
/*
+ * add_to_unowned_list -- link an SMgrRelation onto the unowned list
+ *
+ * Check remove_from_unowned_list()s comments for performance
+ * considerations.
+ */
+static void
+add_to_unowned_list(SMgrRelation reln)
+{
+ /* place it at head of unowned list (to make smgrsetowner cheap) */
+ reln->next_unowned_reln = first_unowned_reln;
+ first_unowned_reln = reln;
+}
+
+
+/*
* smgrexists() -- Does the underlying file for a fork exist?
*/
bool
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 98b6f13..20c834d 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -80,6 +80,7 @@ extern void smgrinit(void);
extern SMgrRelation smgropen(RelFileNode rnode, BackendId backend);
extern bool smgrexists(SMgrRelation reln, ForkNumber forknum);
extern void smgrsetowner(SMgrRelation *owner, SMgrRelation reln);
+extern void smgrunown(SMgrRelation reln);
extern void smgrclose(SMgrRelation reln);
extern void smgrcloseall(void);
extern void smgrclosenode(RelFileNodeBackend rnode);
--
1.8.4.21.g992c386.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
