Re: alter_table regression test problem

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