On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > On 2013-11-07 06:49:58 -0800, Kevin Grittner wrote:
> >> It's up to the committer whether to indent
> >> after review and make both substantive and whitespace changes
> >> together, but I have definitely seen those done separately, or even
> >> left for the next global pgindent run to fix.
>
> > Hm. I don't remember many such cases and I've just looked across the git
> > history and i didn't really find anything after
> > a04161f2eab55f72b3c3dba9baed0ec09e7f633f, but that was back when
> > individuals couldn't run pgindent because of the typedefs file.
>
> FWIW, I tend to pgindent before committing, now that it's easy to do so.
> But in cases like this, I'd much rather review the patch *before* that
> happens. Basically the point of the review is to follow and confirm
> the patch author's thought process, and I'll bet you put the braces in
> before reindenting too.
Well, I did both at the same time, I have an emacs command for that
;). But independent from that technicality, reindenting is part of
changing the flow of logic for me - I've spent a couple of years
primarily writing python (where indentation is significant), maybe it's
that.
So, here's the patch (slightly editorialized) with reverted indenting of
that block.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 96a637319cbe4bfa58dbd7677a74c31b23e8e248 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Thu, 7 Nov 2013 10:17:28 +0100
Subject: [PATCH] Fix fundamental issues with relfilenodemap's cache
invalidation logic.
relfilenodemap.c's invalidation callback used to simply delete its
entire hash when a cache reset message arrived and it used to enter a
provisional cache entry into the hash before doing the lookups in
pg_class.
Both is a bad idea because the heap_open() in RelidByRelfilenode() can
cause cache invalidations to be received which could cause us to
access and return free'd memory.
Also fix the possible, but unlikely, scenario where a non-FATAL ERROR
occurs after the hash_create() in InitializeRelfilenodeMap() leaving
RelfilenodeMap in a partially initialized state.
Found by Kevin Grittner.
Needs a pgindent run, but that'd make the diff harder to read.
---
src/backend/utils/cache/relfilenodemap.c | 111 +++++++++++++++++--------------
1 file changed, 61 insertions(+), 50 deletions(-)
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index f3f9a09..6b06afb 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -57,23 +57,20 @@ RelfilenodeMapInvalidateCallback(Datum arg, Oid relid)
HASH_SEQ_STATUS status;
RelfilenodeMapEntry *entry;
- /* nothing to do if not active or deleted */
- if (RelfilenodeMapHash == NULL)
- return;
-
- /* if relid is InvalidOid, we must invalidate the entire cache */
- if (relid == InvalidOid)
- {
- hash_destroy(RelfilenodeMapHash);
- RelfilenodeMapHash = NULL;
- return;
- }
+ /* callback only gets registered after creating the hash */
+ Assert(RelfilenodeMapHash != NULL);
hash_seq_init(&status, RelfilenodeMapHash);
while ((entry = (RelfilenodeMapEntry *) hash_seq_search(&status)) != NULL)
{
- /* Same OID may occur in more than one tablespace. */
- if (entry->relid == relid)
+ /*
+ * If relid is InvalidOid, signalling a complete reset, we must remove
+ * all entries, otherwise just remove the specific relation's entry.
+ * Always remove negative cache entries.
+ */
+ if (relid == InvalidOid || /* complete reset */
+ entry->relid == InvalidOid || /* negative cache entry */
+ entry->relid == relid) /* individual flushed relation */
{
if (hash_search(RelfilenodeMapHash,
(void *) &entry->key,
@@ -92,32 +89,12 @@ static void
InitializeRelfilenodeMap(void)
{
HASHCTL ctl;
- static bool initial_init_done = false;
- int i;
+ int i;
/* Make sure we've initialized CacheMemoryContext. */
if (CacheMemoryContext == NULL)
CreateCacheMemoryContext();
- /* Initialize the hash table. */
- MemSet(&ctl, 0, sizeof(ctl));
- ctl.keysize = sizeof(RelfilenodeMapKey);
- ctl.entrysize = sizeof(RelfilenodeMapEntry);
- ctl.hash = tag_hash;
- ctl.hcxt = CacheMemoryContext;
-
- RelfilenodeMapHash =
- hash_create("RelfilenodeMap cache", 1024, &ctl,
- HASH_ELEM | HASH_FUNCTION | HASH_CONTEXT);
-
- /*
- * For complete resets we simply delete the entire hash, but there's no
- * need to do the other stuff multiple times. Especially the initialization
- * of the relcche invalidation should only be done once.
- */
- if (initial_init_done)
- return;
-
/* build skey */
MemSet(&relfilenode_skey, 0, sizeof(relfilenode_skey));
@@ -134,10 +111,25 @@ InitializeRelfilenodeMap(void)
relfilenode_skey[0].sk_attno = Anum_pg_class_reltablespace;
relfilenode_skey[1].sk_attno = Anum_pg_class_relfilenode;
+ /* Initialize the hash table. */
+ MemSet(&ctl, 0, sizeof(ctl));
+ ctl.keysize = sizeof(RelfilenodeMapKey);
+ ctl.entrysize = sizeof(RelfilenodeMapEntry);
+ ctl.hash = tag_hash;
+ ctl.hcxt = CacheMemoryContext;
+
+ /*
+ * Only create the RelfilenodeMapHash now, so we don't end up partially
+ * initialized when fmgr_info_cxt() above ERRORs out with an out of memory
+ * error.
+ */
+ RelfilenodeMapHash =
+ hash_create("RelfilenodeMap cache", 1024, &ctl,
+ HASH_ELEM | HASH_FUNCTION | HASH_CONTEXT);
+
/* Watch for invalidation events. */
CacheRegisterRelcacheCallback(RelfilenodeMapInvalidateCallback,
(Datum) 0);
- initial_init_done = true;
}
/*
@@ -156,6 +148,7 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Relation relation;
HeapTuple ntp;
ScanKeyData skey[2];
+ Oid relid;
if (RelfilenodeMapHash == NULL)
InitializeRelfilenodeMap();
@@ -169,30 +162,37 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
key.relfilenode = relfilenode;
/*
- * Check cache and enter entry if nothing could be found. Even if no target
+ * Check cache and return entry if one is found. Even if no target
* relation can be found later on we store the negative match and return a
- * InvalidOid from cache. That's not really necessary for performance since
- * querying invalid values isn't supposed to be a frequent thing, but the
- * implementation is simpler this way.
+ * InvalidOid from cache. That's not really necessary for performance
+ * since querying invalid values isn't supposed to be a frequent thing,
+ * but it's basically free.
*/
- entry = hash_search(RelfilenodeMapHash, (void *) &key, HASH_ENTER, &found);
+ entry = hash_search(RelfilenodeMapHash, (void *) &key, HASH_FIND, &found);
if (found)
return entry->relid;
- /* initialize empty/negative cache entry before doing the actual lookup */
- entry->relid = InvalidOid;
-
/* ok, no previous cache entry, do it the hard way */
- /* check shared tables */
+ /* initialize empty/negative cache entry before doing the actual lookups */
+ relid = InvalidOid;
+
if (reltablespace == GLOBALTABLESPACE_OID)
{
- entry->relid = RelationMapFilenodeToOid(relfilenode, true);
- return entry->relid;
+ /*
+ * Ok, shared table, check relmapper.
+ */
+ relid = RelationMapFilenodeToOid(relfilenode, true);
}
+ else
+ {
+ /*
+ * Not a shared table, could either be a plain relation or a
+ * non-shared, nailed one, like e.g. pg_class.
+ */
- /* check plain relations by looking in pg_class */
+ /* check for plain relations by looking in pg_class */
relation = heap_open(RelationRelationId, AccessShareLock);
/* copy scankey to local copy, it will be modified during the scan */
@@ -236,7 +236,7 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Assert(!isnull && check == relfilenode);
}
#endif
- entry->relid = HeapTupleGetOid(ntp);
+ relid = HeapTupleGetOid(ntp);
}
systable_endscan(scandesc);
@@ -244,7 +244,18 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
/* check for tables that are mapped but not shared */
if (!found)
- entry->relid = RelationMapFilenodeToOid(relfilenode, false);
+ relid = RelationMapFilenodeToOid(relfilenode, false);
+ }
+
+ /*
+ * Only enter entry into cache now, our opening of pg_class could have
+ * caused cache invalidations to be executed which would have deleted a
+ * new entry if we had HASH_ENTERed it above.
+ */
+ entry = hash_search(RelfilenodeMapHash, (void *) &key, HASH_ENTER, &found);
+ if (found)
+ elog(ERROR, "corrupted hashtable");
+ entry->relid = relid;
- return entry->relid;
+ return relid;
}
--
1.8.3.251.g1462b67
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
