On 2015-05-27 15:10, Michael Paquier wrote:
> On Wed, Apr 29, 2015 at 1:30 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Sun, Apr 26, 2015 at 6:05 AM, Tom Lane <tgl@sss.pgh.pa.us> 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.
>
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?
- I think the changes to ATPostAlterTypeParse should follow more closely
the coding of transformTableLikeClause - namely use the idxcomment
- 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.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services