pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
Retain comments on indexes and constraints at ALTER TABLE ... TYPE ...
When a column's datatype is changed, ATExecAlterColumnType() rebuilds all
the affected indexes and constraints, and the comments from the old
indexes/constraints were not carried over.
To fix, create a synthetic COMMENT ON command in the work queue, to re-add
any comments on constraints. For indexes, there's a comment field in
IndexStmt that is used.
This fixes bug #13126, reported by Kirill Simonov. Original patch by
Michael Paquier, reviewed by Petr Jelinek and me. This bug is present in
all versions, but only backpatch to 9.5. Given how minor the issue is, it
doesn't seem worth the work and risk to backpatch further than that.
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/e42375fc8124e99c33fa330c53c2b4b502fa0baf
Modified Files
--------------
src/backend/commands/tablecmds.c | 65 ++++++++++++++++++++++++++++-
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/alter_table.out | 63 ++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 36 ++++++++++++++++
4 files changed, 163 insertions(+), 2 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
Retain comments on indexes and constraints at ALTER TABLE ... TYPE ...
One or the other of these patches has broken the majority of the
buildfarm.
regards, tom lane
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Tue, Jul 14, 2015 at 10:02 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
Retain comments on indexes and constraints at ALTER TABLE ... TYPE ...
One or the other of these patches has broken the majority of the
buildfarm.
Some ORDER BY clauses are missing to make the output consistent it
seems. At the same time the queries could be reformated a bit to not
be longer than 80 characters. A patch is attached.
--
Michael
Attachments:
20150714_fix_altertable.patchapplication/x-patch; name=20150714_fix_altertable.patchDownload
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 6b9291b..bd5069d 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2409,23 +2409,31 @@ CREATE TABLE comment_test (
CREATE INDEX comment_test_index ON comment_test(indexed_col);
COMMENT ON COLUMN comment_test.id IS 'Column ''id'' on comment_test';
COMMENT ON INDEX comment_test_index IS 'Simple index on comment_test';
-COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test IS 'CHECK constraint on comment_test.positive_col';
-COMMENT ON CONSTRAINT comment_test_pk ON comment_test IS 'PRIMARY KEY constraint of comment_test';
-COMMENT ON INDEX comment_test_pk IS 'Index backing the PRIMARY KEY of comment_test';
+COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test
+ IS 'CHECK constraint on comment_test.positive_col';
+COMMENT ON CONSTRAINT comment_test_pk ON comment_test
+ IS 'PRIMARY KEY constraint of comment_test';
+COMMENT ON INDEX comment_test_pk
+ IS 'Index backing the PRIMARY KEY of comment_test';
SELECT col_description('comment_test'::regclass, 1) as comment;
comment
-----------------------------
Column 'id' on comment_test
(1 row)
-SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+SELECT indexrelid::regclass AS index,
+ obj_description(indexrelid, 'pg_class') AS comment
+FROM pg_index where indrelid = 'comment_test'::regclass ORDER BY index;
index | comment
--------------------+-----------------------------------------------
- comment_test_index | Simple index on comment_test
comment_test_pk | Index backing the PRIMARY KEY of comment_test
+ comment_test_index | Simple index on comment_test
(2 rows)
-SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+SELECT conname as constraint,
+ obj_description(oid, 'pg_constraint') AS comment
+FROM pg_constraint where conrelid = 'comment_test'::regclass
+ORDER BY conname;
constraint | comment
---------------------------------+-----------------------------------------------
comment_test_pk | PRIMARY KEY constraint of comment_test
@@ -2449,18 +2457,23 @@ SELECT col_description('comment_test'::regclass, 1) as comment;
Column 'id' on comment_test
(1 row)
-SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+SELECT indexrelid::regclass AS index,
+ obj_description(indexrelid, 'pg_class') AS comment
+FROM pg_index where indrelid = 'comment_test'::regclass ORDER BY index;
index | comment
--------------------+-----------------------------------------------
- comment_test_pk | Index backing the PRIMARY KEY of comment_test
comment_test_index | Simple index on comment_test
+ comment_test_pk | Index backing the PRIMARY KEY of comment_test
(2 rows)
-SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+SELECT conname AS constraint,
+ obj_description(oid, 'pg_constraint') AS comment
+FROM pg_constraint where conrelid = 'comment_test'::regclass
+ORDER BY conname;
constraint | comment
---------------------------------+-----------------------------------------------
- comment_test_positive_col_check | CHECK constraint on comment_test.positive_col
comment_test_pk | PRIMARY KEY constraint of comment_test
+ comment_test_positive_col_check | CHECK constraint on comment_test.positive_col
(2 rows)
-- Check that we map relation oids to filenodes and back correctly. Only
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 9f755fa..666ac9c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1605,13 +1605,21 @@ CREATE INDEX comment_test_index ON comment_test(indexed_col);
COMMENT ON COLUMN comment_test.id IS 'Column ''id'' on comment_test';
COMMENT ON INDEX comment_test_index IS 'Simple index on comment_test';
-COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test IS 'CHECK constraint on comment_test.positive_col';
-COMMENT ON CONSTRAINT comment_test_pk ON comment_test IS 'PRIMARY KEY constraint of comment_test';
-COMMENT ON INDEX comment_test_pk IS 'Index backing the PRIMARY KEY of comment_test';
+COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test
+ IS 'CHECK constraint on comment_test.positive_col';
+COMMENT ON CONSTRAINT comment_test_pk ON comment_test
+ IS 'PRIMARY KEY constraint of comment_test';
+COMMENT ON INDEX comment_test_pk
+ IS 'Index backing the PRIMARY KEY of comment_test';
SELECT col_description('comment_test'::regclass, 1) as comment;
-SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
-SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+SELECT indexrelid::regclass AS index,
+ obj_description(indexrelid, 'pg_class') AS comment
+FROM pg_index where indrelid = 'comment_test'::regclass ORDER BY index;
+SELECT conname as constraint,
+ obj_description(oid, 'pg_constraint') AS comment
+FROM pg_constraint where conrelid = 'comment_test'::regclass
+ORDER BY conname;
-- Change the datatype of all the columns. ALTER TABLE is optimized to not
-- rebuild an index if the new data type is binary compatible with the old
@@ -1626,8 +1634,13 @@ ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
-- Check that the comments are intact.
SELECT col_description('comment_test'::regclass, 1) as comment;
-SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
-SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+SELECT indexrelid::regclass AS index,
+ obj_description(indexrelid, 'pg_class') AS comment
+FROM pg_index where indrelid = 'comment_test'::regclass ORDER BY index;
+SELECT conname AS constraint,
+ obj_description(oid, 'pg_constraint') AS comment
+FROM pg_constraint where conrelid = 'comment_test'::regclass
+ORDER BY conname;
-- Check that we map relation oids to filenodes and back correctly. Only
On 07/14/2015 04:02 PM, Tom Lane wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
Retain comments on indexes and constraints at ALTER TABLE ... TYPE ...
One or the other of these patches has broken the majority of the
buildfarm.
Ah, the order of the rows in the test queries was not stable. Added
ORDER BYs. Sorry about that.
- Heikki
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote:
This fixes bug #13126, reported by Kirill Simonov.
It looks like you missed something with the addition of
AT_ReAddComment:
test_ddl_deparse.c:80:11: warning: enumeration value 'AT_ReAddComment' not handled in switch [-Wswitch]
switch (subcmd->subtype)
^
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote:
This fixes bug #13126, reported by Kirill Simonov.
It looks like you missed something with the addition of
AT_ReAddComment:test_ddl_deparse.c:80:11: warning: enumeration value 'AT_ReAddComment' not handled in switch [-Wswitch]
switch (subcmd->subtype)
^
Oops. If someone could pick up the attached (backpatch to 9.5 needed)...
--
Michael
Attachments:
20150717_testddl_warning.patchbinary/octet-stream; name=20150717_testddl_warning.patchDownload
diff --git a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
index 44a5cb0..e70e796 100644
--- a/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
+++ b/src/test/modules/test_ddl_deparse/test_ddl_deparse.c
@@ -130,6 +130,9 @@ get_altertable_subcmdtypes(PG_FUNCTION_ARGS)
case AT_ReAddConstraint:
strtype = "(re) ADD CONSTRAINT";
break;
+ case AT_ReAddComment:
+ strtype = "(re) COMMENT";
+ break;
case AT_AlterConstraint:
strtype = "ALTER CONSTRAINT";
break;
On 07/17/2015 05:40 PM, Michael Paquier wrote:
On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote:
This fixes bug #13126, reported by Kirill Simonov.
It looks like you missed something with the addition of
AT_ReAddComment:test_ddl_deparse.c:80:11: warning: enumeration value 'AT_ReAddComment' not handled in switch [-Wswitch]
switch (subcmd->subtype)
^Oops. If someone could pick up the attached (backpatch to 9.5 needed)...
Hmm, that function is pretty fragile, it will segfault on any AT_* type
that it doesn't recognize. Thankfully you get that compiler warning, but
we have added AT_* type codes before in minor releases. I couldn't quite
figure out what the purpose of that module is, as there is no
documentation or README or file-header comments on it. If it's there
just to so you can run the regression tests that come with it, it might
make sense to just add a "default" case to that switch to handle any
unrecognized commands, and perhaps even remove the cases for the
currently untested subcommands as it's just dead code. If it's supposed
to act like a sample that you can copy-paste and modify, then perhaps
that would still be the best option, and add a comment there explaining
that it only cares about the most common subtypes but you can add
handlers for others if necessary.
Alvaro, you want to comment on that?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas wrote:
On 07/17/2015 05:40 PM, Michael Paquier wrote:
On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote:
This fixes bug #13126, reported by Kirill Simonov.
It looks like you missed something with the addition of
AT_ReAddComment:test_ddl_deparse.c:80:11: warning: enumeration value 'AT_ReAddComment' not handled in switch [-Wswitch]
switch (subcmd->subtype)
^Oops. If someone could pick up the attached (backpatch to 9.5 needed)...
Hmm, that function is pretty fragile, it will segfault on any AT_* type that
it doesn't recognize. Thankfully you get that compiler warning, but we have
added AT_* type codes before in minor releases.
Yeah, that module was put together in a bit of a rush when I decided to
remove the JSON deparsing part of the DDL patch.
I couldn't quite figure out what the purpose of that module is, as
there is no documentation or README or file-header comments on it.
Well, since it's in src/test/modules I thought it was clear that the
intention is just to be able to test the pg_ddl_command type --
obviously not. I will add a README or something.
If it's there just to so you can run the regression tests that come
with it, it might make sense to just add a "default" case to that
switch to handle any unrecognized commands, and perhaps even remove
the cases for the currently untested subcommands as it's just dead
code.
Well, I would prefer to have an output that says "unrecognized" and then
add more test cases to the SQL files so that there's not so much dead
code. I prefer that to removing the C support code, because then as
we add extra tests we don't need to modify the C source.
If it's supposed to act like a sample that you can copy-paste and
modify, then perhaps that would still be the best option, and add a
comment there explaining that it only cares about the most common
subtypes but you can add handlers for others if necessary.
I wasn't thinking in having it be useful for copy-paste. My longer-term
plan is to have the JSON deparsing extension live in src/extensions.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 07/18/2015 04:15 PM, Alvaro Herrera wrote:
Heikki Linnakangas wrote:
If it's there just to so you can run the regression tests that come
with it, it might make sense to just add a "default" case to that
switch to handle any unrecognized commands, and perhaps even remove
the cases for the currently untested subcommands as it's just dead
code.Well, I would prefer to have an output that says "unrecognized" and then
add more test cases to the SQL files so that there's not so much dead
code. I prefer that to removing the C support code, because then as
we add extra tests we don't need to modify the C source.
Ok. I added a case for AT_ReAddComment, and also a default-case that
says "unrecognized".
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas wrote:
On 07/18/2015 04:15 PM, Alvaro Herrera wrote:
Heikki Linnakangas wrote:
If it's there just to so you can run the regression tests that come
with it, it might make sense to just add a "default" case to that
switch to handle any unrecognized commands, and perhaps even remove
the cases for the currently untested subcommands as it's just dead
code.Well, I would prefer to have an output that says "unrecognized" and then
add more test cases to the SQL files so that there's not so much dead
code. I prefer that to removing the C support code, because then as
we add extra tests we don't need to modify the C source.Ok. I added a case for AT_ReAddComment, and also a default-case that says
"unrecognized".
Oh, sorry, I forgot to tell you that I was working on it.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera wrote:
Heikki Linnakangas wrote:
On 07/18/2015 04:15 PM, Alvaro Herrera wrote:
Heikki Linnakangas wrote:
If it's there just to so you can run the regression tests that come
with it, it might make sense to just add a "default" case to that
switch to handle any unrecognized commands, and perhaps even remove
the cases for the currently untested subcommands as it's just dead
code.Well, I would prefer to have an output that says "unrecognized" and then
add more test cases to the SQL files so that there's not so much dead
code. I prefer that to removing the C support code, because then as
we add extra tests we don't need to modify the C source.Ok. I added a case for AT_ReAddComment, and also a default-case that says
"unrecognized".Oh, sorry, I forgot to tell you that I was working on it.
I pushed the remaining part of my patch. Thanks for fixing the other
bits.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-07-17 21:40:45 +0300, Heikki Linnakangas wrote:
Hmm, that function is pretty fragile, it will segfault on any AT_* type that
it doesn't recognize. Thankfully you get that compiler warning, but we have
added AT_* type codes before in minor releases.
For in-core code that is supposed to handle all cases I find it much
better to get a compiler warning than to error out in a default:
case. The latter is very likely not going to be exercised by any test
and thus not be noticed for prolonged time.
Might make sense to test for -Werror=switch and add that to the compiler
flags by default.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers