alter_table regression test problem
In checking things out with CLOBBER_CACHE_ALWAYS, I was getting
this problem, which seems to be unrelated to my changes:
*** /home/kgrittn/pg/master/src/test/regress/expected/alter_table.out 2013-11-01 09:07:35.418829105 -0500
--- /home/kgrittn/pg/master/src/test/regress/results/alter_table.out 2013-11-06 11:06:29.785830560 -0600
***************
*** 2320,2325 ****
) mapped;
incorrectly_mapped | have_mappings
--------------------+---------------
! 0 | t
(1 row)
--- 2320,2325 ----
) mapped;
incorrectly_mapped | have_mappings
--------------------+---------------
! 1 | t
(1 row)
======================================================================
I looked for detail with this query:
SELECT
mapped_oid, oid
FROM (
SELECT
oid, reltablespace, relfilenode, relname,
pg_filenode_relation(reltablespace, pg_relation_filenode(oid)) mapped_oid
FROM pg_class
WHERE relkind IN ('r', 'i', 'S', 't', 'm')
) mapped
WHERE (mapped_oid != oid OR mapped_oid IS NULL);
... with this result:
mapped_oid | oid
------------+------
2139062143 | 2619
(1 row)
2619 is the oid for pg_statistic, and the mapped_oid value matches
what we use to clobber the cache. Any ideas before I start
digging?
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
Kevin Grittner <kgrittn@ymail.com> schrieb:
In checking things out with CLOBBER_CACHE_ALWAYS, I was getting
this problem, which seems to be unrelated to my changes:
... with this result:
mapped_oid | oid
------------+------
2139062143 | 2619
(1 row)2619 is the oid for pg_statistic, and the mapped_oid value matches
what we use to clobber the cache. Any ideas before I start
digging?
Interesting. That's new code of mine, but it hasn't shown up in the clobber animals so far. I'll have a look.
Thanks,
Andred
---
Please excuse brevity and formatting - I am writing this on my mobile phone.--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> wrote:
In checking things out with CLOBBER_CACHE_ALWAYS, I was getting
this problem, which seems to be unrelated to my changes:
On a CLOBBER_CACHE_ALWAYS build I did a fresh initdb, started the
cluster, and immediately tested this query on both the postgres and
template1 databases, with the same result:
SELECT
*
FROM (
SELECT
oid, reltablespace, relfilenode, relname,
pg_filenode_relation(reltablespace,
pg_relation_filenode(oid)) mapped_oid
FROM pg_class
WHERE relkind IN ('r', 'i', 'S', 't', 'm')
) mapped
WHERE (mapped_oid != oid OR mapped_oid IS NULL);
oid | reltablespace | relfilenode | relname | mapped_oid
------+---------------+-------------+--------------+------------
2619 | 0 | 11828 | pg_statistic | 2139062143
(1 row)
That makes for a pretty simple test for git bisect, even if
everything including initdb is painfully slow with
CLOBBER_CACHE_ALWAYS.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner wrote:
That makes for a pretty simple test for git bisect, even if
everything including initdb is painfully slow with
CLOBBER_CACHE_ALWAYS.
Most likely culprit is f01d1ae3a104019d6d68aeff85c4816a275130b3
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-06 17:00:40 -0300, Alvaro Herrera wrote:
Kevin Grittner wrote:
That makes for a pretty simple test for git bisect, even if
everything including initdb is painfully slow with
CLOBBER_CACHE_ALWAYS.Most likely culprit is f01d1ae3a104019d6d68aeff85c4816a275130b3
Well, that test tests the functionality added in that commit, so sure,
it can't be before that. What confuses me is that relfilenodemap has
survived quite some CLOBBER_CACHE_ALWAYS runs in the buildfarm since:
http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=jaguarundi&br=HEAD
Did you compile with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVELY?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-07 00:17:34 +0100, Andres Freund wrote:
On 2013-11-06 17:00:40 -0300, Alvaro Herrera wrote:
Kevin Grittner wrote:
That makes for a pretty simple test for git bisect, even if
everything including initdb is painfully slow with
CLOBBER_CACHE_ALWAYS.Most likely culprit is f01d1ae3a104019d6d68aeff85c4816a275130b3
Well, that test tests the functionality added in that commit, so sure,
it can't be before that. What confuses me is that relfilenodemap has
survived quite some CLOBBER_CACHE_ALWAYS runs in the buildfarm since:
http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=jaguarundi&br=HEADDid you compile with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVELY?
Either way, the code is completely and utterly broken in the face of
cache invalidations that are received when it does its internal
heap_open(RelationRelationId). I can't have been thinking straight when
I wrote the invalidation logic.
Will send a fix.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-07 00:30:55 +0100, Andres Freund wrote:
On 2013-11-07 00:17:34 +0100, Andres Freund wrote:
On 2013-11-06 17:00:40 -0300, Alvaro Herrera wrote:
Kevin Grittner wrote:
That makes for a pretty simple test for git bisect, even if
everything including initdb is painfully slow with
CLOBBER_CACHE_ALWAYS.Most likely culprit is f01d1ae3a104019d6d68aeff85c4816a275130b3
Well, that test tests the functionality added in that commit, so sure,
it can't be before that. What confuses me is that relfilenodemap has
survived quite some CLOBBER_CACHE_ALWAYS runs in the buildfarm since:
http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=jaguarundi&br=HEADDid you compile with CLOBBER_CACHE_ALWAYS or CLOBBER_CACHE_RECURSIVELY?
Either way, the code is completely and utterly broken in the face of
cache invalidations that are received when it does its internal
heap_open(RelationRelationId). I can't have been thinking straight when
I wrote the invalidation logic.
Ok, I've attached a fix for this, which unfortunately is not all that
small.
Could either of you commit it?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
0001-Fix-fundamental-issues-with-relfilenodemap-s-cache-i.patchtext/x-patch; charset=us-asciiDownload
>From ac8973aec6642810f11aff2c4d1c3079b1569f7f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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.
---
src/backend/utils/cache/relfilenodemap.c | 189 ++++++++++++++++---------------
1 file changed, 99 insertions(+), 90 deletions(-)
diff --git a/src/backend/utils/cache/relfilenodemap.c b/src/backend/utils/cache/relfilenodemap.c
index f3f9a09..d3d77dd 100644
--- a/src/backend/utils/cache/relfilenodemap.c
+++ b/src/backend/utils/cache/relfilenodemap.c
@@ -43,8 +43,8 @@ typedef struct
typedef struct
{
- RelfilenodeMapKey key; /* lookup key - must be first */
- Oid relid; /* pg_class.oid */
+ RelfilenodeMapKey key; /* lookup key - must be first */
+ Oid relid; /* pg_class.oid */
} RelfilenodeMapEntry;
/*
@@ -54,26 +54,24 @@ typedef struct
static void
RelfilenodeMapInvalidateCallback(Datum arg, Oid relid)
{
- HASH_SEQ_STATUS status;
+ 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;
- }
-
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 +90,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 +112,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 +149,7 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
Relation relation;
HeapTuple ntp;
ScanKeyData skey[2];
+ Oid relid;
if (RelfilenodeMapHash == NULL)
InitializeRelfilenodeMap();
@@ -169,82 +163,97 @@ RelidByRelfilenode(Oid reltablespace, Oid relfilenode)
key.relfilenode = relfilenode;
/*
- * Check cache and enter entry if nothing could be 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.
+ * 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 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 */
+ /* initialize empty/negative cache entry before doing the actual lookups */
+ relid = InvalidOid;
+
/* check shared tables */
if (reltablespace == GLOBALTABLESPACE_OID)
{
- entry->relid = RelationMapFilenodeToOid(relfilenode, true);
- return entry->relid;
+ relid = RelationMapFilenodeToOid(relfilenode, true);
}
+ /*
+ * Not a shared table, could either be a plain relation or a
+ * database specific but nailed one, like e.g. pg_class.
+ */
+ else
+ {
+ /* check forplain relations by looking in pg_class */
+ relation = heap_open(RelationRelationId, AccessShareLock);
- /* check plain relations by looking in pg_class */
- relation = heap_open(RelationRelationId, AccessShareLock);
-
- /* copy scankey to local copy, it will be modified during the scan */
- memcpy(skey, relfilenode_skey, sizeof(skey));
+ /* copy scankey to local copy, it will be modified during the scan */
+ memcpy(skey, relfilenode_skey, sizeof(skey));
- /* set scan arguments */
- skey[0].sk_argument = ObjectIdGetDatum(reltablespace);
- skey[1].sk_argument = ObjectIdGetDatum(relfilenode);
+ /* set scan arguments */
+ skey[0].sk_argument = ObjectIdGetDatum(reltablespace);
+ skey[1].sk_argument = ObjectIdGetDatum(relfilenode);
- scandesc = systable_beginscan(relation,
- ClassTblspcRelfilenodeIndexId,
- true,
- NULL,
- 2,
- skey);
+ scandesc = systable_beginscan(relation,
+ ClassTblspcRelfilenodeIndexId,
+ true,
+ NULL,
+ 2,
+ skey);
- found = false;
+ found = false;
- while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
- {
- bool isnull PG_USED_FOR_ASSERTS_ONLY;
+ while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
+ {
+ bool isnull PG_USED_FOR_ASSERTS_ONLY;
- if (found)
- elog(ERROR,
- "unexpected duplicate for tablespace %u, relfilenode %u",
- reltablespace, relfilenode);
- found = true;
+ if (found)
+ elog(ERROR,
+ "unexpected duplicate for tablespace %u, relfilenode %u",
+ reltablespace, relfilenode);
+ found = true;
#ifdef USE_ASSERT_CHECKING
- if (assert_enabled)
- {
- Oid check;
- check = fastgetattr(ntp, Anum_pg_class_reltablespace,
- RelationGetDescr(relation),
- &isnull);
- Assert(!isnull && check == reltablespace);
-
- check = fastgetattr(ntp, Anum_pg_class_relfilenode,
- RelationGetDescr(relation),
- &isnull);
- Assert(!isnull && check == relfilenode);
- }
+ if (assert_enabled)
+ {
+ Oid check;
+ check = fastgetattr(ntp, Anum_pg_class_reltablespace,
+ RelationGetDescr(relation),
+ &isnull);
+ Assert(!isnull && check == reltablespace);
+
+ check = fastgetattr(ntp, Anum_pg_class_relfilenode,
+ RelationGetDescr(relation),
+ &isnull);
+ Assert(!isnull && check == relfilenode);
+ }
#endif
- entry->relid = HeapTupleGetOid(ntp);
- }
+ relid = HeapTupleGetOid(ntp);
+ }
- systable_endscan(scandesc);
- heap_close(relation, AccessShareLock);
+ systable_endscan(scandesc);
+ heap_close(relation, AccessShareLock);
- /* check for tables that are mapped but not shared */
- if (!found)
- entry->relid = RelationMapFilenodeToOid(relfilenode, false);
+ /* check for tables that are mapped but not shared */
+ if (!found)
+ relid = RelationMapFilenodeToOid(relfilenode, false);
+ }
+
+ /*
+ * Only enter entry into cache now, our opening of pg_class could have
+ * caused a cache invalidation 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.5.rc1.dirty
Andres Freund <andres@2ndquadrant.com> wrote:
Ok, I've attached a fix for this, which unfortunately is not all
that small.
Could either of you commit it?
Unfortunately, I don't feel I have a good enough grasp of the
caching/invalidation mechanism to be a committer for this
particular patch, but I think you could make it a lot easier to
review by eliminating some of the whitespace changes. I applied
your patch and then ran pgindent, which promptly undid some of the
whitespace changes this patch makes, so there is really no excuse
for those. I think this is one of those cases where the large
chunk of code inside the new "else" block should not be indented
with the initial patch -- a pgindent-based whitespace-only patch
should follow so that the substantive changes here are easier to
see in the initial patch. Also, I *really* don't like the
whitespace and comment placement here:
/* check shared tables */
if (reltablespace == GLOBALTABLESPACE_OID)
{
relid = RelationMapFilenodeToOid(relfilenode, true);
}
/*
* Not a shared table, could either be a plain relation or a database
* specific but nailed one, like e.g. pg_class.
*/
else
{
... which is what results from pgindent acting on this patch.
Please move the "else" comment inside the opening brace for the
"else".
I think that would make the patch a lot easier for someone to
review, and then it can be reformatted separately.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-07 06:23:13 -0800, Kevin Grittner wrote:
Andres Freund <andres@2ndquadrant.com> wrote:
Ok, I've attached a fix for this, which unfortunately is not all
that small.
Could either of you commit it?Unfortunately, I don't feel I have a good enough grasp of the
caching/invalidation mechanism to be a committer for this
particular patch,
That's understandable.
but I think you could make it a lot easier to
review by eliminating some of the whitespace changes. I applied
your patch and then ran pgindent, which promptly undid some of the
whitespace changes this patch makes, so there is really no excuse
for those.
I'll try to produce a pgindent-clean version.
I think this is one of those cases where the large
chunk of code inside the new "else" block should not be indented
with the initial patch -- a pgindent-based whitespace-only patch
should follow so that the substantive changes here are easier to
see in the initial patch.
I think commiting code with fundamentally broken indentation like that
is pretty much pointless though. Somebody who wants to look at the
actual changes without the whitespace noise can just use git diff -w/git
blame -w/... to eliminate those while viewing.
Also, I *really* don't like the
whitespace and comment placement here:/* check shared tables */
if (reltablespace == GLOBALTABLESPACE_OID)
{
relid = RelationMapFilenodeToOid(relfilenode, true);
}/*
* Not a shared table, could either be a plain relation or a database
* specific but nailed one, like e.g. pg_class.
*/
else
{... which is what results from pgindent acting on this patch.
Please move the "else" comment inside the opening brace for the
"else".
Man, I *hate* pgindent. Imo the comment belongs to the outside, not the
inside since it's toplevel logic that matters. Anyway I'll try to clean
it up somehow.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-11-07 06:23:13 -0800, Kevin Grittner wrote:
I think this is one of those cases where the large
chunk of code inside the new "else" block should not be indented
with the initial patch -- a pgindent-based whitespace-only patch
should follow so that the substantive changes here are easier to
see in the initial patch.I think commiting code with fundamentally broken indentation like that
is pretty much pointless though. Somebody who wants to look at the
actual changes without the whitespace noise can just use git diff -w/git
blame -w/... to eliminate those while viewing.
I think it is pretty much SOP for committers to prefer a patch that
adds a new pair of braces around 50 lines of code and only changes
non-whitespace of a few lines within that block to leave the block
at its old indentation. 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.
Personally, I was surprised how small this apparently-large patch
became when I used git --color-words to compare it, which would be
another option, I guess; but there is precedent for the approach I
suggested.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@ymail.com> writes:
I think it is pretty much SOP for committers to prefer a patch that
adds a new pair of braces around 50 lines of code and only changes
non-whitespace of a few lines within that block to leave the block
at its old indentation.
Yes. It's much easier to review a patch done that way than to wonder if
the apparently-just-whitespace changes are masking something substantive.
In fact, if I'm reviewing a patch that reindents a big chunk of existing
code, I'll frequently not use the patch but reconstruct it that way,
just to be sure.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-07 09:55:55 -0500, Tom Lane wrote:
Kevin Grittner <kgrittn@ymail.com> writes:
I think it is pretty much SOP for committers to prefer a patch that
adds a new pair of braces around 50 lines of code and only changes
non-whitespace of a few lines within that block to leave the block
at its old indentation.Yes. It's much easier to review a patch done that way than to wonder if
the apparently-just-whitespace changes are masking something substantive.
In fact, if I'm reviewing a patch that reindents a big chunk of existing
code, I'll frequently not use the patch but reconstruct it that way,
just to be sure.
But why not just use git diff/show/whatever -w or, as suggested by
Kevin, --color-words?
ISTM the patch author is much more likely to mistake when indenting code
strangely.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@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.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/07/2013 09:59 AM, Andres Freund wrote:
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.
Perhaps we need more frequent pgindent runs.
both patch and git-apply have options to ignore whitespace changes.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
Andres Freund <andres@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
Attachments:
0001-Fix-fundamental-issues-with-relfilenodemap-s-cache-i.patchtext/x-patch; charset=us-asciiDownload
>From 96a637319cbe4bfa58dbd7677a74c31b23e8e248 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@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
On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
Andres Freund <andres@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.
Gah. Well, of course, I have the opposite preference from Tom on how
this should be indented. Sigh.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 11, 2013 at 4:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
Andres Freund <andres@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.Gah. Well, of course, I have the opposite preference from Tom on how
this should be indented. Sigh.
...and I hit send too soon.
I'm pretty sure that the current coding, which blows away the whole
relation, is used in other places, and I really don't see why it
should be fundamentally flawed, or why we should change it to clear
the cache entries out one by one instead of en masse.
RelidByRelfilenode definitely needs to use HASH_FIND rather than
HASH_ENTER, so that part I agree with.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> schrieb:
On Mon, Nov 11, 2013 at 4:00 PM, Robert Haas <robertmhaas@gmail.com>
wrote:On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund
<andres@2ndquadrant.com> wrote:
On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
Andres Freund <andres@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, oreven
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), maybeit's
that.
So, here's the patch (slightly editorialized) with reverted
indenting of
that block.
Gah. Well, of course, I have the opposite preference from Tom on how
this should be indented. Sigh....and I hit send too soon.
I'm pretty sure that the current coding, which blows away the whole
relation, is used in other places, and I really don't see why it
should be fundamentally flawed, or why we should change it to clear
the cache entries out one by one instead of en masse.
RelidByRelfilenode definitely needs to use HASH_FIND rather than
HASH_ENTER, so that part I agree with.
It surely is possible to go that route, but imagine what happens if the heap_open() blows away the entire hash. We'd either need to recheck if the hash exists before entering or recreate it after dropping. It seemed simpler to follow attoptcache's example.
Regards,
Andres
--
Please excuse brevity and formatting - I am writing this on my mobile phone.
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Nov 11, 2013 at 4:34 PM, Andres Freund <andres@2ndquadrant.com> wrote:
I'm pretty sure that the current coding, which blows away the whole
relation, is used in other places, and I really don't see why it
should be fundamentally flawed, or why we should change it to clear
the cache entries out one by one instead of en masse.
RelidByRelfilenode definitely needs to use HASH_FIND rather than
HASH_ENTER, so that part I agree with.It surely is possible to go that route, but imagine what happens if the heap_open() blows away the entire hash. We'd either need to recheck if the hash exists before entering or recreate it after dropping. It seemed simpler to follow attoptcache's example.
I'm not sure if this is the best way forward, but I don't feel like
arguing about it, either, so committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers