Thread: BUG #13126: table constraint loses its comment
The following bug has been logged on the website:
Bug reference: 13126
Logged by: Kirill Simonov
Email address: xi@resolvent.net
PostgreSQL version: 9.4.1
Operating system: Ubuntu 15.04
Description:
In some circumstances, the comment on a table constraint disappears. Here
is an example:
-- Create a table with a primary key constraint.
CREATE TYPE enum1 AS ENUM ('foo', 'bar');
CREATE TYPE enum2 AS ENUM ('foo', 'bar', 'baz');
CREATE TABLE t (x enum1 NOT NULL);
ALTER TABLE t ADD CONSTRAINT t_pk PRIMARY KEY (x);
COMMENT ON CONSTRAINT t_pk ON t IS 'the primary key of table "t"';
-- Find the constraint:
--
-- oid
-- ---------
-- 3400853
-- (1 row)
SELECT c.oid
FROM pg_constraint c
WHERE c.conname = 't_pk';
-- Find the comment on the constraint:
--
-- description
-- ------------------------------
-- the primary key of table "t"
-- (1 row)
SELECT d.description
FROM
pg_description d,
pg_constraint c
WHERE
d.classoid = 'pg_constraint'::regclass AND
c.conname = 't_pk' AND
d.objoid = c.oid;
-- Change the type of the primary key column.
ALTER TABLE t ALTER COLUMN x SET DATA TYPE enum2 USING x::text::enum2;
-- The constraint now has a different OID:
--
-- oid
-- ---------
-- 3400855
-- (1 row)
SELECT c.oid
FROM pg_constraint c
WHERE c.conname = 't_pk';
-- The constraint comment is lost:
--
-- description
-- -------------
-- (0 rows)
SELECT d.description
FROM
pg_description d,
pg_constraint c
WHERE
d.classoid = 'pg_constraint'::regclass AND
c.conname = 't_pk' AND
d.objoid = c.oid;
-- Cleanup.
DROP TABLE t;
DROP TYPE enum1;
DROP TYPE enum2;
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.
regards, tom lane
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? -- Michael
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. -- Michael
Attachment
On 05/27/2015 04:10 PM, 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.
The problem isn't limited to the cases where the old index is not
reused. This still fails with your patch:
postgres=# create table t(id text primary key);CREATE TABLE
postgres=# comment on constraint t_pkey on t is 'the primary key of
table "t"';COMMENT
postgres=# select d.description from pg_description d, pg_constraint c
where d.classoid = 'pg_constraint'::regclass and c.conname = 't_pkey'
and d.objoid = c.oid;
description
------------------------------
the primary key of table "t"
(1 row)
-- perform an ALTER TABLE that doesn't really do anything
postgres=# ALTER TABLE t ALTER COLUMN id SET DATA TYPE text;
ALTER TABLE
-- The comment is gone:
postgres=# select d.description from pg_description d, pg_constraint c
where d.classoid = 'pg_constraint'::regclass and c.conname = 't_pkey'
and d.objoid = c.oid;
description
-------------
(0 rows)
- Heikki
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
On Thu, Jul 2, 2015 at 9:28 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > The problem isn't limited to the cases where the old index is not reused. > This still fails with your patch: > [test case] Interesting, so my patch is over-complicated. I will fix in next version with more test cases to check as well reused indexes. -- Michael
On Thu, Jul 2, 2015 at 11:16 PM, Petr Jelinek <petr@2ndquadrant.com> 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
Attachment
On 2015-07-03 15:50, Michael Paquier wrote: > On Thu, Jul 2, 2015 at 11:16 PM, Petr Jelinek <petr@2ndquadrant.com> 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. > Ah ok, I missed Heikki's email. >> - 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? > 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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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
Attachment
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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Jul 7, 2015 at 6:24 PM, Petr Jelinek <petr@2ndquadrant.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. -- Michael
On 07/08/2015 08:12 AM, Michael Paquier wrote: > On Tue, Jul 7, 2015 at 6:24 PM, Petr Jelinek <petr@2ndquadrant.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
On Tue, Jul 14, 2015 at 1:42 AM, Heikki Linnakangas wrote: > 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. Ouch. I had a look at the patches and they look neater than what I drafted. Thanks! > 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. Well, while it's clearly a bug I don't think that it is worth the risk to destabilize back branches older than 9.5 in this code path. So +1 for doing it the way you are suggesting. We could still revisit that later on if there are more complaints, but I doubt there will be much. -- Michael
On 07/14/2015 10:29 AM, Michael Paquier wrote: > On Tue, Jul 14, 2015 at 1:42 AM, Heikki Linnakangas wrote: >> 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. > > Well, while it's clearly a bug I don't think that it is worth the risk > to destabilize back branches older than 9.5 in this code path. So +1 > for doing it the way you are suggesting. We could still revisit that > later on if there are more complaints, but I doubt there will be much. Ok, committed to master and 9.5. Thanks! - Heikki