Re: alter_table regression test problem

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Kevin Grittner <kgrittn(at)ymail(dot)com>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: alter_table regression test problem
Date: 2013-11-07 14:32:19
Message-ID: 20131107143219.GB24361@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kevin Grittner 2013-11-07 14:49:58 Re: alter_table regression test problem
Previous Message Kevin Grittner 2013-11-07 14:23:13 Re: alter_table regression test problem