Re: BUG #13126: table constraint loses its comment

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Petr Jelinek <petr(at)2ndquadrant(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, xi(at)resolvent(dot)net, PostgreSQL mailing lists <pgsql-bugs(at)postgresql(dot)org>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: BUG #13126: table constraint loses its comment
Date: 2015-07-13 16:42:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

On 07/08/2015 08:12 AM, Michael Paquier wrote:
> On Tue, Jul 7, 2015 at 6:24 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:
>> On 2015-07-04 13:45, Michael Paquier wrote:
>>> On Fri, Jul 3, 2015 at 11:59 PM, Petr Jelinek wrote:
>>>> Well for indexes you don't really need to add the new AT command, as
>>>> IndexStmt has char *idxcomment which it will automatically uses as
>>>> comment
>>>> if not NULL. While I am not huge fan of the idxcomment it doesn't seem
>>>> to
>>>> be easy to remove it in the future and it's what transformTableLikeClause
>>>> uses so it might be good to be consistent with that.
>>> Oh, right, I completely missed your point and this field in IndexStmt.
>>> Yes it is better to be consistent in this case and to use it. It makes
>>> as well the code easier to follow.
>>> Btw, regarding the new AT routines, I am getting find of them, it
>>> makes easier to follow which command is added where in the command
>>> queues.
>>> Updated patch attached.
>> Cool, I am happy with the patch now. Marking as ready for committer.
> Thanks for the review.

I don't much like splitting the code across multiple helper functions,
it makes the overall logic more difficult to follow, although I agree
that the indentation has made the pretty hard to read as it is. I'm
planning to commit the attached two patches. The first one is just
reformatting changes to ATExecAlterColumnType(), turning the switch-case
statements into if-else blocks. If-else doesn't cause so much
indentation, and allows defining variables local to the "cases" more
naturally, so it alleviates the indentation problem somewhat. The second
patch is the actual bug fix.

There was one bug in this patch: the COMMENT statement that was
constructed didn't schema-qualify the relation, so if the ALTERed table
was not in search_path, the operation would fail with a "relation not
found" error (or add the comment to wrong object). Fixed that.

I plan to commit the attached patches later today or tomorrow. But how
do you feel about back-patching this? It should be possible to
backpatch, although at a quick test it seems that there have been small
changes to the affected code in many versions, so it would require some
work. Also, in back-branches we'd need to put the new AT_ReAddComment
type to the end of the list, like we've done when we added
AT_ReAddConstraint in 9.3. I'm inclined to backpatch this to 9.5 only,
even though this is clearly a bug fix, on the grounds that it's not a
very serious bug and there's always some risk in backpatching.

- Heikki

Attachment Content-Type Size
0001-Reformat-code-in-ATPostAlterTypeParse.patch text/x-diff 4.5 KB
0002-Retain-comments-on-indexes-and-constraints-at-ALTER-.patch text/x-diff 12.6 KB

In response to


Browse pgsql-bugs by date

  From Date Subject
Next Message Tom Lane 2015-07-13 20:13:38 Re: BUG #13498: make check failures
Previous Message pete 2015-07-13 14:49:17 BUG #13499: building with xlc optimization level 5 can not initdb

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2015-07-13 16:53:22 Re: intarray planning/exeuction problem with multiple operators
Previous Message Alvaro Herrera 2015-07-13 16:13:08 Re: [PATCH] Add missing \ddp psql command