Re: [HACKERS] pg_upgrade's exec_prog() coding improvement
On 08/24/2012 11:44 AM, Alvaro Herrera wrote: Again, win32 testing would be welcome. Sadly, buildfarm does not run pg_upgrade's make check. Yesterday I added a new module to the buildfarm client code to run this (https://github.com/PGBuildFarm/client-code/commit/ab812cb9920c65e39ec7358dc816371f1fef31eb). It required a couple of tweaks in the base code. This will be in a new buildfarm client release fairly shortly. It's running on crake now, and I will add it to pitta to get some Windows coverage. It would be a lot nicer is the test were written in Perl, since we don't necessarily have a Bourne shell available for MSVC builds, but we definitely have Perl available. None of this does what I think we really need, which is cross-release pg_upgrade testing, which remains on my TODO list as a fairly high priority item. cheers andrew -- 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] pg_upgrade's exec_prog() coding improvement
On 08/31/2012 10:52 AM, Andrew Dunstan wrote: On 08/24/2012 11:44 AM, Alvaro Herrera wrote: Again, win32 testing would be welcome. Sadly, buildfarm does not run pg_upgrade's make check. Yesterday I added a new module to the buildfarm client code to run this (https://github.com/PGBuildFarm/client-code/commit/ab812cb9920c65e39ec7358dc816371f1fef31eb). It required a couple of tweaks in the base code. This will be in a new buildfarm client release fairly shortly. It's running on crake now, and I will add it to pitta to get some Windows coverage. but it's not working on pitta :-( It will take me some time to work out why, and I have several more urgent things on my plate. Meanwhile at least we have Linux coverage. cheers andrew -- 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] pg_upgrade's exec_prog() coding improvement
Excerpts from Alvaro Herrera's message of vie ago 24 11:44:33 -0400 2012: Actually it seems better to just get rid of the extra varargs function and just have a single exec_prog. The extra NULL argument is not troublesome enough, it seems. Updated version attached. Applied (with some very minor changes). -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] pg_upgrade's exec_prog() coding improvement
On 23.08.2012 23:07, Alvaro Herrera wrote: One problem with this is that I get this warning: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? You can do what the warning suggests, and tell the compiler that exec_prog takes printf-like arguments. See elog.h for an example of that: extern int errmsg(const char *fmt,...) /* This extension allows gcc to check the format string for consistency with the supplied arguments. */ __attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2))); -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- 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] pg_upgrade's exec_prog() coding improvement
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 23.08.2012 23:07, Alvaro Herrera wrote: One problem with this is that I get this warning: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function âs_exec_progâ: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for âgnu_printfâ format attribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for âgnu_printfâ format attribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? You can do what the warning suggests, and tell the compiler that exec_prog takes printf-like arguments. exec_prog already has such decoration, and Alvaro's patch doesn't remove it. So the question is, exactly what the heck does that warning mean? regards, tom lane -- 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] pg_upgrade's exec_prog() coding improvement
On Fri, Aug 24, 2012 at 10:08:58AM -0400, Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 23.08.2012 23:07, Alvaro Herrera wrote: One problem with this is that I get this warning: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? You can do what the warning suggests, and tell the compiler that exec_prog takes printf-like arguments. exec_prog already has such decoration, and Alvaro's patch doesn't remove it. So the question is, exactly what the heck does that warning mean? It sounds like it is suggestioning to use more specific attribute decoration. This might be relevant: http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html -Wmissing-format-attribute Warn about function pointers that might be candidates for format attributes. Note these are only possible candidates, not absolute ones. GCC guesses that function pointers with format attributes that are used in assignment, initialization, parameter passing or return statements should have a corresponding format attribute in the resulting type. I.e. the left-hand side of the assignment or initialization, the type of the parameter variable, or the return type of the containing function respectively should also have a format attribute to avoid the warning. GCC also warns about function definitions that might be candidates for format attributes. Again, these are only possible candidates. GCC guesses that format attributes might be appropriate for any function that calls a function like vprintf or vscanf, but this might not always be the case, and some functions for which format attributes are appropriate may not be detected. and I see this option enabled in configure.in. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] pg_upgrade's exec_prog() coding improvement
Bruce Momjian br...@momjian.us writes: It sounds like it is suggestioning to use more specific attribute decoration. This might be relevant: http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html -Wmissing-format-attribute Warn about function pointers that might be candidates for format attributes. Note these are only possible candidates, not absolute ones. GCC guesses that function pointers with format attributes that are used in assignment, initialization, parameter passing or return statements should have a corresponding format attribute in the resulting type. I.e. the left-hand side of the assignment or initialization, the type of the parameter variable, or the return type of the containing function respectively should also have a format attribute to avoid the warning. GCC also warns about function definitions that might be candidates for format attributes. Again, these are only possible candidates. GCC guesses that format attributes might be appropriate for any function that calls a function like vprintf or vscanf, but this might not always be the case, and some functions for which format attributes are appropriate may not be detected. and I see this option enabled in configure.in. Hm. I'm a bit dubious about enabling warnings that are so admittedly heuristic as this. They admit that the warnings might be wrong, and yet document no way to shut them up. I think we might be better advised to not enable this warning. regards, tom lane -- 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] pg_upgrade's exec_prog() coding improvement
Actually it seems better to just get rid of the extra varargs function and just have a single exec_prog. The extra NULL argument is not troublesome enough, it seems. Updated version attached. Again, win32 testing would be welcome. Sadly, buildfarm does not run pg_upgrade's make check. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services upgrade_execprog-2.patch Description: Binary data -- 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] pg_upgrade's exec_prog() coding improvement
On Fri, 2012-08-24 at 10:08 -0400, Tom Lane wrote: Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 23.08.2012 23:07, Alvaro Herrera wrote: One problem with this is that I get this warning: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? You can do what the warning suggests, and tell the compiler that exec_prog takes printf-like arguments. exec_prog already has such decoration, and Alvaro's patch doesn't remove it. So the question is, exactly what the heck does that warning mean? The warning is about s_exec_prog, not exec_prog. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_upgrade's exec_prog() coding improvement
Hi, I've been bitten twice by exec_prog()s API, so here's a patch to try to make it a bit harder to misuse. There are two main changes here; one is to put the logfile option as the first argument; then comes a bool, then the format string. This means you get a warning if you pass the wrong number of arguments before the format; previously I mis-merged in the KEY SHARE patch so that I was passing the log file as format, and the compiler didn't complain at all. The other interesting change I did was move the responsibility of adding SYSTEMQUOTE and the %s 21 redirections to exec_prog itself, reducing clutter in the caller. This makes the callers a lot easier to read. One other minor change I did was split it in two versions: a simpler one with less frammishes, that doesn't let you supply an alternative log file and doesn't return a status value. This is used for all but one of the callers; only that one caller was interested in the result value anyway. And that caller is also the only one that passes an opt_log_file other than NULL, so I removed that from the simple version. Lastly, I removed the is_priv boolean, which seems pretty pointless; just made all calls set the umask inconditionally. The only caller doing this was the cp/xcopy call, and I don't see any reason for this to be different from other callers. This makes pg_upgrade a bit more readable. If anyone can test this on Windows I would be appreciate it. One problem with this is that I get this warning: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c: In function ‘s_exec_prog’: /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] /pgsql/source/HEAD/contrib/pg_upgrade/exec.c:96:2: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute] I have no idea how to silence that. Ideas? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services upgrade_execprog.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
