Re: [COMMITTERS] pgsql: Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt.

Add PF_PRINTF_ATTRIBUTE to on_exit_msg_fmt.

Per warning from -Wmissing-format-attribute.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/ea988ee8c8b191615e730f930bcde6144a598688

Modified Files
--------------
src/bin/pg_dump/dumputils.h | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)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(dot)linnakangas(at)iki(dot)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

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 25.03.2013 15:36, Tom Lane wrote:
>> Heikki Linnakangas<heikki(dot)linnakangas(at)iki(dot)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(at)vmware(dot)com> writes:
>> On 25.03.2013 15:36, Tom Lane wrote:
>>> Heikki Linnakangas<heikki(dot)linnakangas(at)iki(dot)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(at)vmware(dot)com> writes:
>>> On 25.03.2013 15:36, Tom Lane wrote:
>>>> Heikki Linnakangas<heikki(dot)linnakangas(at)iki(dot)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

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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)vmware(dot)com> writes:
>> Alvaro Herrera<alvherre(at)2ndquadrant(dot)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(at)vmware(dot)com> writes:
>>> Alvaro Herrera<alvherre(at)2ndquadrant(dot)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