Re: 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(at)ymail(dot)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(at)ymail(dot)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

Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)ymail(dot)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(at)ymail(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)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(at)gmail(dot)com> wrote:
> On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
>>> Andres Freund <andres(at)2ndquadrant(dot)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(at)gmail(dot)com> schrieb:
>On Mon, Nov 11, 2013 at 4:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>wrote:
>> On Thu, Nov 7, 2013 at 12:18 PM, Andres Freund
><andres(at)2ndquadrant(dot)com> wrote:
>>> On 2013-11-07 10:10:34 -0500, Tom Lane wrote:
>>>> Andres Freund <andres(at)2ndquadrant(dot)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 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

On Mon, Nov 11, 2013 at 4:34 PM, Andres Freund <andres(at)2ndquadrant(dot)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