pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
13+ messages / 7 participants
[nested] [flat]
* pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-14 08:43 Heikki Linnakangas <heikki.linnakangas@iki.fi>
0 siblings, 0 replies; 13+ messages in thread
From: Heikki Linnakangas @ 2015-07-14 08:43 UTC (permalink / raw)
To: pgsql-committers
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
------
REL9_5_STABLE
Details
-------
http://git.postgresql.org/pg/commitdiff/9dee48c94b6eb544dd334ec021ff224454f2020f
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-14 08:43 Heikki Linnakangas <heikki.linnakangas@iki.fi>
0 siblings, 2 replies; 13+ messages in thread
From: Heikki Linnakangas @ 2015-07-14 08:43 UTC (permalink / raw)
To: pgsql-committers
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-14 13:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Heikki Linnakangas <heikki.linnakangas@iki.fi>
1 sibling, 2 replies; 13+ messages in thread
From: Tom Lane @ 2015-07-14 13:02 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@iki.fi>; +Cc: 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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-14 13:24 Michael Paquier <michael.paquier@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 13+ messages in thread
From: Michael Paquier @ 2015-07-14 13:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Heikki Linnakangas <heikki.linnakangas@iki.fi>; 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
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Attachments:
20150714_fix_altertable.patch (application/x-patch, 6.7K) download
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
* Re: pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-14 13:25 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 13+ messages in thread
From: Heikki Linnakangas @ 2015-07-14 13:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-17 14:16 Kevin Grittner <kgrittn@ymail.com>
parent: Heikki Linnakangas <heikki.linnakangas@iki.fi>
1 sibling, 1 reply; 13+ messages in thread
From: Kevin Grittner @ 2015-07-17 14:16 UTC (permalink / raw)
To: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; 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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-17 14:40 Michael Paquier <michael.paquier@gmail.com>
parent: Kevin Grittner <kgrittn@ymail.com>
0 siblings, 1 reply; 13+ messages in thread
From: Michael Paquier @ 2015-07-17 14:40 UTC (permalink / raw)
To: Kevin Grittner <kgrittn@ymail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
20150717_testddl_warning.patch (binary/octet-stream, 558B) download
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;
* Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-17 18:40 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Michael Paquier <michael.paquier@gmail.com>
0 siblings, 2 replies; 13+ messages in thread
From: Heikki Linnakangas @ 2015-07-17 18:40 UTC (permalink / raw)
To: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Alvaro Herrera <alvherre@2ndquadrant.com>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-18 13:15 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 1 reply; 13+ messages in thread
From: Alvaro Herrera @ 2015-07-18 13:15 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-20 07:47 Heikki Linnakangas <hlinnaka@iki.fi>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 13+ messages in thread
From: Heikki Linnakangas @ 2015-07-20 07:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-20 08:41 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 13+ messages in thread
From: Alvaro Herrera @ 2015-07-20 08:41 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-20 09:25 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2015-07-20 09:25 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
@ 2015-07-20 09:46 Andres Freund <andres@anarazel.de>
parent: Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 0 replies; 13+ messages in thread
From: Andres Freund @ 2015-07-20 09:46 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael.paquier@gmail.com>; Kevin Grittner <kgrittn@ymail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Alvaro Herrera <alvherre@2ndquadrant.com>
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
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2015-07-20 09:46 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2015-07-14 08:43 pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY Heikki Linnakangas <heikki.linnakangas@iki.fi>
2015-07-14 13:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2015-07-14 13:24 ` Michael Paquier <michael.paquier@gmail.com>
2015-07-14 13:25 ` Heikki Linnakangas <hlinnaka@iki.fi>
2015-07-17 14:16 ` Kevin Grittner <kgrittn@ymail.com>
2015-07-17 14:40 ` Michael Paquier <michael.paquier@gmail.com>
2015-07-17 18:40 ` Heikki Linnakangas <hlinnaka@iki.fi>
2015-07-18 13:15 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-07-20 07:47 ` Heikki Linnakangas <hlinnaka@iki.fi>
2015-07-20 08:41 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-07-20 09:25 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-07-20 09:46 ` Andres Freund <andres@anarazel.de>
2015-07-14 08:43 pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY Heikki Linnakangas <heikki.linnakangas@iki.fi>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox