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 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
