Thread: 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
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.
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
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
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
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
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
Attachment
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
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
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
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
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
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
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
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
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
Attachment
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
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
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, 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 eitherneed to recheck if the hash exists before entering or recreate it after dropping. It seemed simpler to follow attoptcache'sexample. 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
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 eitherneed to recheck if the hash exists before entering or recreate it after dropping. It seemed simpler to follow attoptcache'sexample. 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