Re: [HACKERS] alter_table regression test problem
On Mon, Nov 11, 2013 at 4:34 PM, Andres Freund 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
Re: [HACKERS] alter_table regression test problem
Robert Haas schrieb: >On Mon, Nov 11, 2013 at 4:00 PM, Robert Haas >wrote: >> On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund > wrote: >>> On 2013-11-07 10:10:34 -0500, Tom Lane wrote: Andres Freund 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. 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
Re: [HACKERS] alter_table regression test problem
On Mon, Nov 11, 2013 at 4:00 PM, Robert Haas wrote: > On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund wrote: >> On 2013-11-07 10:10:34 -0500, Tom Lane wrote: >>> Andres Freund 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
Re: [HACKERS] alter_table regression test problem
On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund wrote: > On 2013-11-07 10:10:34 -0500, Tom Lane wrote: >> Andres Freund 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
Re: [HACKERS] alter_table regression test problem
On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
> Andres Freund 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
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_clas
Re: [HACKERS] alter_table regression test problem
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
Re: [HACKERS] alter_table regression test problem
Andres Freund 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
Re: [HACKERS] alter_table regression test problem
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
Re: [HACKERS] alter_table regression test problem
On 2013-11-07 09:55:55 -0500, Tom Lane wrote: > Kevin Grittner 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
Re: [HACKERS] alter_table regression test problem
Kevin Grittner 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
Re: [HACKERS] alter_table regression test problem
Andres Freund 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
Re: [HACKERS] alter_table regression test problem
On 2013-11-07 06:23:13 -0800, Kevin Grittner wrote:
> Andres Freund 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
Re: [HACKERS] alter_table regression test problem
Andres Freund 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
Re: [HACKERS] alter_table regression test problem
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=HEAD
> >
> > Did 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
>From ac8973aec6642810f11aff2c4d1c3079b1569f7f Mon Sep 17 00:00:00 2001
From: Andres Freund
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_sk
Re: [HACKERS] alter_table regression test problem
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=HEAD > > Did 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
Re: [HACKERS] alter_table regression test problem
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
Re: [HACKERS] alter_table regression test problem
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 Herrerahttp://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
Re: [HACKERS] alter_table regression test problem
Kevin Grittner 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
Re: [HACKERS] alter_table regression test problem
Hi, Kevin Grittner 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
[HACKERS] 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
