[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-20 Thread Heikki Linnakangas

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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-20 Thread Andres Freund
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


[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-20 Thread Alvaro Herrera
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 Herrerahttp://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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-20 Thread Alvaro Herrera
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 Herrerahttp://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


[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-18 Thread Alvaro Herrera
Heikki Linnakangas wrote:
 On 07/17/2015 05:40 PM, Michael Paquier wrote:
 On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner kgri...@ymail.com wrote:
 Heikki Linnakangas heikki.linnakan...@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 Herrerahttp://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


[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-17 Thread Michael Paquier
On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner kgri...@ymail.com wrote:
 Heikki Linnakangas heikki.linnakan...@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


20150717_testddl_warning.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-17 Thread Kevin Grittner
Heikki Linnakangas heikki.linnakan...@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-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

2015-07-17 Thread Heikki Linnakangas

On 07/17/2015 05:40 PM, Michael Paquier wrote:

On Fri, Jul 17, 2015 at 11:16 PM, Kevin Grittner kgri...@ymail.com wrote:

Heikki Linnakangas heikki.linnakan...@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