BUG #13126: table constraint loses its comment

BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

On 07/08/2015 08:12 AM, Michael Paquier wrote:
> On Tue, Jul 7, 2015 at 6:24 PM, Petr Jelinek  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

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

On Wed, Apr 29, 2015 at 1:30 PM, Michael Paquier
 wrote:
> On Sun, Apr 26, 2015 at 6:05 AM, Tom Lane  wrote:
>> xi@resolvent.net writes:
>>> In some circumstances, the comment on a table constraint disappears.  Here
>>> is an example:
>>
>> Hm, yeah.  The problem is that ATExecAlterColumnType() rebuilds all the
>> affected indexes from scratch, and it isn't doing anything about copying
>> their comments to the new objects (either comments on the constraints, or
>> comments directly on the indexes).
>>
>> The least painful way to fix it might be to charter ATPostAlterTypeCleanup
>> to create COMMENT commands and add those to the appropriate work queue,
>> rather than complicating the data structure initially emitted by
>> ATExecAlterColumnType.  But it'd still be a fair amount of new code I'm
>> afraid.
>>
>> Not planning to fix this personally, but maybe someone else would like to
>> take it up.
>
> In order to fix this, an idea would be to add a new routine in
> ruleutils.c that generates the COMMENT query string, and then call it
> directly from tablecmds.c. On master, I imagine that we could even add
> some SQL interface if there is some need.
> Thoughts?

After looking at this problem, I noticed that the test case given
above does not cover everything: primary key indexes, constraints
(CHECK for example) and indexes alone have also their comments removed
after ALTER TABLE when those objects are re-created. I finished with
the patch attached to fix everything, patch that includes a set of
regression tests covering all the code paths added.
-- 
Michael

Re: BUG #13126: table constraint loses its comment

On Thu, Jul 2, 2015 at 11:16 PM, Petr Jelinek  wrote:
> I was going through the code and have few comments:
> - Why do you change the return value of TryReuseIndex? Can't we use reuse
> the same OidIsValid(stmt->oldNode) check that ATExecAddIndex is doing
> instead?

As pointed out by Heikki previously, that is actually unnecessary,
comments are still lost even if the index is reused for constraints.
So perhaps for simplicity we could just unconditionally recreate the
comments all the time if they are available.

> - I think the changes to ATPostAlterTypeParse should follow more closely the
> coding of transformTableLikeClause - namely use the idxcomment

I am not sure I follow here. Could you elaborate?

> - Also the changes to ATPostAlterTypeParse move the code somewhat
> uncomfortably over the 80 char width, I don't really see a way to fix that
> except for moving some of the nesting out to another function.

Yes, I did some refactoring here by adding a set of new routines
dedicated to attach generated commands to the correct queue. This way
the code is empty of large switch/case blocks.

Update patch is attached, with the issue reported by Heikki upthread
fixed as well.
Regards,
-- 
Michael

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment

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.
-- 
Michael

Re: BUG #13126: table constraint loses its comment

Re: BUG #13126: table constraint loses its comment