Thread: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Kevin Grittner
Date:
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
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Michael Paquier
Date:
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
Attachment
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Heikki Linnakangas
Date:
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
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Alvaro Herrera
Date:
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
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Heikki Linnakangas
Date:
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
Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Alvaro Herrera
Date:
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
Re: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Alvaro Herrera
Date:
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
Re: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY
From
Andres Freund
Date:
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