Thread: Re: [COMMITTERS] pgsql: Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt.
Heikki Linnakangas <heikki.linnakangas@iki.fi> writes:
> Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt.
> Per warning from -Wmissing-format-attribute.
Hm, this is exactly what I removed yesterday, because it makes the build
fail outright on old gcc:
gcc -O1 -Wall -Wmissing-prototypes -Wpointer-arith -Wformat-security -fno-strict-aliasing -g
-I../../../src/interfaces/libpq-I../../../src/include -D_XOPEN_SOURCE_EXTENDED -D_USE_CTYPE_MACROS -c -o pg_dump.o
pg_dump.c
In file included from pg_backup.h:29,
from pg_backup_archiver.h:32,
from pg_dump.c:60:
dumputils.h:48: argument format specified for non-function `on_exit_msg_func'
make: *** [pg_dump.o] Error 1
Perhaps we have to refactor to avoid the use of a function variable
here. It didn't seem particularly critical to do it like that rather
than with, say, a bool.
Or maybe we should turn off that warning. It seems to be leaping to
conclusions about what the usage of the function variable is.
regards, tom lane
On 25.03.2013 15:36, Tom Lane wrote: > Heikki Linnakangas<heikki.linnakangas@iki.fi> writes: >> Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt. >> Per warning from -Wmissing-format-attribute. > > Hm, this is exactly what I removed yesterday, because it makes the build > fail outright on old gcc: > > gcc -O1 -Wall -Wmissing-prototypes -Wpointer-arith -Wformat-security -fno-strict-aliasing -g -I../../../src/interfaces/libpq-I../../../src/include -D_XOPEN_SOURCE_EXTENDED -D_USE_CTYPE_MACROS -c -o pg_dump.o pg_dump.c > In file included from pg_backup.h:29, > from pg_backup_archiver.h:32, > from pg_dump.c:60: > dumputils.h:48: argument format specified for non-function `on_exit_msg_func' > make: *** [pg_dump.o] Error 1 > > Perhaps we have to refactor to avoid the use of a function variable > here. It didn't seem particularly critical to do it like that rather > than with, say, a bool. Hmm. exit_horribly() is also used in pg_dumpall, so if you just put a call to a function in parallel.c in there, the linker will complain when linking pg_dumpall. BTW, we never reset on_exit_msg_func, even after getting out of parallel mode by calling ParallelBackupEnd(). The Assert in parallel_exit_msg_func() will fail if it gets called after ParallelBackupEnd(). The attached seems to work. With this patch, on_exit_msg_func() is gone. There's a different implementation of exit_horribly for pg_dumpall and pg_dump/restore. In pg_dumpall, it just calls vwrite_msg(). In pg_dump/restore's version, the logic from parallel_exit_msg_func() is moved directly to exit_horribly(). > Or maybe we should turn off that warning. It seems to be leaping to > conclusions about what the usage of the function variable is. Oh? Its conclusion seems correct to me: the function variable takes printf-like arguments. - Heikki
Attachment
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 25.03.2013 15:36, Tom Lane wrote:
>> Heikki Linnakangas<heikki.linnakangas@iki.fi> writes:
>>> Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt.
>>> Per warning from -Wmissing-format-attribute.
>> Hm, this is exactly what I removed yesterday, because it makes the build
>> fail outright on old gcc:
> The attached seems to work. With this patch, on_exit_msg_func() is gone.
> There's a different implementation of exit_horribly for pg_dumpall and
> pg_dump/restore. In pg_dumpall, it just calls vwrite_msg(). In
> pg_dump/restore's version, the logic from parallel_exit_msg_func() is
> moved directly to exit_horribly().
Seems probably reasonable, though if we're taking exit_horribly out of
dumputils.c, meseems it ought not be declared in dumputils.h anymore.
Can we put that declaration someplace else, rather than commenting it
with an apology?
regards, tom lane
On 26.03.2013 02:02, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >> On 25.03.2013 15:36, Tom Lane wrote: >>> Heikki Linnakangas<heikki.linnakangas@iki.fi> writes: >>>> Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt. >>>> Per warning from -Wmissing-format-attribute. > >>> Hm, this is exactly what I removed yesterday, because it makes the build >>> fail outright on old gcc: > >> The attached seems to work. With this patch, on_exit_msg_func() is gone. >> There's a different implementation of exit_horribly for pg_dumpall and >> pg_dump/restore. In pg_dumpall, it just calls vwrite_msg(). In >> pg_dump/restore's version, the logic from parallel_exit_msg_func() is >> moved directly to exit_horribly(). > > Seems probably reasonable, though if we're taking exit_horribly out of > dumputils.c, meseems it ought not be declared in dumputils.h anymore. > Can we put that declaration someplace else, rather than commenting it > with an apology? Ugh, the patch I posted doesn't actually work, because dumputils.c is also used in psql and some scripts, so you get a linker error in those. psql and scripts don't use exit_horribly or many of the other functions in dumputils.c, so I think we should split dumputils.c into two parts anyway. fmtId and the other functions that are used by psql in one file, and the functions that are only shared between pg_dumpall and pg_dump in another. Then there's also functions that are used by pg_dump and pg_restore, but not pg_dumpall or psql. I'll try moving things around a bit... - Heikki
On 26.03.2013 09:51, Heikki Linnakangas wrote: > On 26.03.2013 02:02, Tom Lane wrote: >> Heikki Linnakangas<hlinnakangas@vmware.com> writes: >>> On 25.03.2013 15:36, Tom Lane wrote: >>>> Heikki Linnakangas<heikki.linnakangas@iki.fi> writes: >>>>> Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt. >>>>> Per warning from -Wmissing-format-attribute. >> >>>> Hm, this is exactly what I removed yesterday, because it makes the >>>> build >>>> fail outright on old gcc: >> >>> The attached seems to work. With this patch, on_exit_msg_func() is gone. >>> There's a different implementation of exit_horribly for pg_dumpall and >>> pg_dump/restore. In pg_dumpall, it just calls vwrite_msg(). In >>> pg_dump/restore's version, the logic from parallel_exit_msg_func() is >>> moved directly to exit_horribly(). >> >> Seems probably reasonable, though if we're taking exit_horribly out of >> dumputils.c, meseems it ought not be declared in dumputils.h anymore. >> Can we put that declaration someplace else, rather than commenting it >> with an apology? > > Ugh, the patch I posted doesn't actually work, because dumputils.c is > also used in psql and some scripts, so you get a linker error in those. > psql and scripts don't use exit_horribly or many of the other functions > in dumputils.c, so I think we should split dumputils.c into two parts > anyway. fmtId and the other functions that are used by psql in one file, > and the functions that are only shared between pg_dumpall and pg_dump in > another. Then there's also functions that are used by pg_dump and > pg_restore, but not pg_dumpall or psql. > > I'll try moving things around a bit... This is what I came up with. I created a new file, misc.c (for lack of a better name), for things that are shared by pg_dump and pg_restore, but not pg_dumpall or other programs. I moved all the parallel stuff from dumputils.c to parallel.c, and everything else that's not used outside pg_dump and pg_restore to misc.c. I moved exit_horribly() to parallel.c, because it needs to do things differently in parallel mode. I still used a function pointer, not for the printf-style message printing routine, but for making dumputils.c independent of parallel mode. getThreadLocalPQBuffer() is now a function pointer; the default implementation just uses a static variable, but when pg_dump/restore enters parallel mode, it points the function pointer to a version that uses thread-local storage (on windows). - Heikki
Attachment
Heikki Linnakangas wrote: > This is what I came up with. I created a new file, misc.c (for lack > of a better name), for things that are shared by pg_dump and > pg_restore, but not pg_dumpall or other programs. I moved all the > parallel stuff from dumputils.c to parallel.c, and everything else > that's not used outside pg_dump and pg_restore to misc.c. I moved > exit_horribly() to parallel.c, because it needs to do things > differently in parallel mode. Not happy with misc.c as a filename. How about pg_dump_utils.c or pg_dump_misc.c? I think the comment at the top should explicitely say that the file is intended not to be linked in pg_dumpall. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Not happy with misc.c as a filename. We already have two misc.c files: src/backend/utils/adt/misc.c src/interfaces/ecpg/ecpglib/misc.c I much prefer not to repeat the same filename in different directories if we can avoid it. > How about pg_dump_utils.c or pg_dump_misc.c? Those seem reasonable to me. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-03-26 13:14:53 -0700, Kevin Grittner wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > Not happy with misc.c as a filename. > > We already have two misc.c files: > > src/backend/utils/adt/misc.c > src/interfaces/ecpg/ecpglib/misc.c > > I much prefer not to repeat the same filename in different > directories if we can avoid it. > > > How about pg_dump_utils.c or pg_dump_misc.c? > > Those seem reasonable to me. I vote against including pg_ in the filename, for an implementation private file that seems duplicative. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 26.03.2013 22:14, Kevin Grittner wrote: > Alvaro Herrera<alvherre@2ndquadrant.com> wrote: > >> Not happy with misc.c as a filename. > > We already have two misc.c files: > > src/backend/utils/adt/misc.c > src/interfaces/ecpg/ecpglib/misc.c > > I much prefer not to repeat the same filename in different > directories if we can avoid it. > >> How about pg_dump_utils.c or pg_dump_misc.c? > > Those seem reasonable to me. pg_dump_utils.c could be confused with dumputils.c. And I'd rather not have pg_dump in the filename, because the file is for functions that are used in both pg_dump and pg_restore. To add to the confusion, there's also common.c, which is only used by pg_dump (historical reasons). There's a bunch of files called pg_backup_*.c that are also shared between pg_dump and pg_restore, so perhaps pg_backup_utils.c? - Heikki
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> Alvaro Herrera<alvherre@2ndquadrant.com> wrote:
>>> Not happy with misc.c as a filename.
> There's a bunch of files called pg_backup_*.c that are also shared
> between pg_dump and pg_restore, so perhaps pg_backup_utils.c?
Works for me. There's inherently not going to be a lot of content
in this filename, so we aren't going to get anything particularly
memorable (though I agree with the duplicativeness argument against
"misc.c").
regards, tom lane
On 27.03.2013 15:28, Tom Lane wrote: > Heikki Linnakangas<hlinnakangas@vmware.com> writes: >>> Alvaro Herrera<alvherre@2ndquadrant.com> wrote: >>>> Not happy with misc.c as a filename. > >> There's a bunch of files called pg_backup_*.c that are also shared >> between pg_dump and pg_restore, so perhaps pg_backup_utils.c? > > Works for me. Ok, sold. - Heikki