Обсуждение: Re: [COMMITTERS] pgsql: Retain comments on indexes and constraints at ALTER TABLE ... TY

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

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

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

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

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

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

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

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