-Wformat-zero-length

Peter Eisentraut <peter_e@gmx.net> writes:

I was adding gcc printf attributes to more functions in obscure places,
and now I'm seeing this in pg_upgrade:

relfilenode.c:72:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]

So the options I can see are either adding the compiler option
-Wno-format-zero-length (with configure check and all), or hack up the
code to do something like this instead: Before:

prep_status("");

After:

prep_status("%s", "");

Comments?

Shouldn't it be prep_status("\n")? If not, why not? (Right offhand, it
looks to me like prep_status could stand to be redefined in a less
bizarre fashion anyhow.)

regards, tom lane

I wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I was adding gcc printf attributes to more functions in obscure places,
and now I'm seeing this in pg_upgrade:

relfilenode.c:72:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]

Shouldn't it be prep_status("\n")? If not, why not?

On closer inspection, it appears to me that prep_status should never be
called with a string containing a newline, period, and the test it
contains for that case is just brain damage. The only reason to call it
at all is to produce a line like

message ......................

where something more is expected to be added to the line later. Calls
that are meant to produce a complete line could go directly to pg_log.

This in turn implies that transfer_all_new_dbs's use of the function is
broken and needs to be rethought.

regards, tom lane

On tor, 2011-07-07 at 18:09 -0400, Tom Lane wrote:

I wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I was adding gcc printf attributes to more functions in obscure places,
and now I'm seeing this in pg_upgrade:

relfilenode.c:72:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]

Shouldn't it be prep_status("\n")? If not, why not?

On closer inspection, it appears to me that prep_status should never be
called with a string containing a newline, period, and the test it
contains for that case is just brain damage. The only reason to call it
at all is to produce a line like

message ......................

where something more is expected to be added to the line later. Calls
that are meant to produce a complete line could go directly to pg_log.

This in turn implies that transfer_all_new_dbs's use of the function is
broken and needs to be rethought.

I think this got a bit besides the point. There is probably some
bogosity in the logging implementation, but I think the prep_status("")
call is correct and purposeful at that point, namely to clear the line.
The question is, do we consider empty format strings a bug worth warning
about, or should we shut off the warning?

On Thu, Jul 7, 2011 at 06:09:54PM -0400, Tom Lane wrote:

I wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

I was adding gcc printf attributes to more functions in obscure places,
and now I'm seeing this in pg_upgrade:

relfilenode.c:72:2: warning: zero-length gnu_printf format string [-Wformat-zero-length]

Shouldn't it be prep_status("\n")? If not, why not?

On closer inspection, it appears to me that prep_status should never be
called with a string containing a newline, period, and the test it
contains for that case is just brain damage. The only reason to call it
at all is to produce a line like

message ......................

where something more is expected to be added to the line later. Calls
that are meant to produce a complete line could go directly to pg_log.

This in turn implies that transfer_all_new_dbs's use of the function is
broken and needs to be rethought.

I changed a prep_status() call to pg_log() as you suggested, and
backpatched to 9.2. Patch attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Excerpts from Bruce Momjian's message of vie ago 03 12:44:35 -0400 2012:

I changed a prep_status() call to pg_log() as you suggested, and
backpatched to 9.2. Patch attached.

I dunno about this particular patch, but if we ever want to move
pg_upgrade out of contrib we will have to stare hard at it to improve
translatability of the messages it emits.

How ready are we to move it to src/bin/? Is it sensible to do so in
9.3?

--
Álvaro Herrera <alvherre@alvh.no-ip.org>

On Fri, Aug 3, 2012 at 12:53:22PM -0400, �lvaro Herrera wrote:

Excerpts from Bruce Momjian's message of vie ago 03 12:44:35 -0400 2012:

I changed a prep_status() call to pg_log() as you suggested, and
backpatched to 9.2. Patch attached.

I dunno about this particular patch, but if we ever want to move
pg_upgrade out of contrib we will have to stare hard at it to improve
translatability of the messages it emits.

Agreed.

How ready are we to move it to src/bin/? Is it sensible to do so in
9.3?

We have talked about that. Several people felt the instructions for
using pg_upgrade was already too complex and that it isn't a
general-user utility like pg_dump. Not sure if that is still the
general feeling.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Excerpts from Bruce Momjian's message of vie ago 03 13:32:21 -0400 2012:

On Fri, Aug 3, 2012 at 12:53:22PM -0400, Álvaro Herrera wrote:

How ready are we to move it to src/bin/? Is it sensible to do so in
9.3?

We have talked about that. Several people felt the instructions for
using pg_upgrade was already too complex and that it isn't a
general-user utility like pg_dump. Not sure if that is still the
general feeling.

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Aug 3, 2012 at 03:10:24PM -0400, Alvaro Herrera wrote:

Excerpts from Bruce Momjian's message of vie ago 03 13:32:21 -0400 2012:

On Fri, Aug 3, 2012 at 12:53:22PM -0400, �lvaro Herrera wrote:

How ready are we to move it to src/bin/? Is it sensible to do so in
9.3?

We have talked about that. Several people felt the instructions for
using pg_upgrade was already too complex and that it isn't a
general-user utility like pg_dump. Not sure if that is still the
general feeling.

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 3, 2012 at 04:01:18PM -0400, Robert Haas wrote:

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

I don't know how to file down any of the existing rough edges.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Fri, Aug 3, 2012 at 10:02 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Fri, Aug 3, 2012 at 04:01:18PM -0400, Robert Haas wrote:

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

I don't know how to file down any of the existing rough edges.

That would be the "serious engineering time" Robert is referring to,
no? If you knew how to do it already it wouldn't require serious
engineering time, just SMOP.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Tue, Aug 7, 2012 at 03:06:23PM +0200, Magnus Hagander wrote:

On Fri, Aug 3, 2012 at 10:02 PM, Bruce Momjian <bruce@momjian.us> wrote:

On Fri, Aug 3, 2012 at 04:01:18PM -0400, Robert Haas wrote:

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

I don't know how to file down any of the existing rough edges.

That would be the "serious engineering time" Robert is referring to,
no? If you knew how to do it already it wouldn't require serious
engineering time, just SMOP.

Oh, I read "serious _engineering_ time" to say that it is just a matter
of coding, while I don't even have a design idea of how to improve this,
meaning it is a lot farther away than just coding it. I equiated
engineering with coding, which I guess was wrong.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Excerpts from Bruce Momjian's message of vie ago 03 16:02:28 -0400 2012:

On Fri, Aug 3, 2012 at 04:01:18PM -0400, Robert Haas wrote:

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

I don't know how to file down any of the existing rough edges.

So do you have a list of rough edges?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Aug 7, 2012 at 10:38:52AM -0400, Alvaro Herrera wrote:

Excerpts from Bruce Momjian's message of vie ago 03 16:02:28 -0400 2012:

On Fri, Aug 3, 2012 at 04:01:18PM -0400, Robert Haas wrote:

On Fri, Aug 3, 2012 at 3:22 PM, Bruce Momjian <bruce@momjian.us> wrote:

I don't disagree with pg_upgrade being operationally complex, but I
don't see how this relates to contrib vs. non-contrib at all. Are we
supposed to only have "simple" programs in src/bin? That seems a
strange policy.

Well, perhaps we need to re-open the discussion then.

I feel like putting it in src/bin would carry an implication of
robustness that I'm not sanguine about. Granted, putting it in
contrib has already pushed the envelope in that direction further than
is perhaps warranted. But ISTM that if we ever want to put this in
src/bin someone needs to devote some serious engineering time to
filing down the rough edges.

I don't know how to file down any of the existing rough edges.

So do you have a list of rough edges?

Yes, the list of rough edges is the 14-steps you have to perform to run
pg_upgrade, as documented in the pg_upgrade manual page:

http://www.postgresql.org/docs/9.2/static/pgupgrade.html

The unknown is how to reduce the number of steps in a way the community
would find acceptable.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Tue, Aug 7, 2012 at 10:59 AM, Bruce Momjian <bruce@momjian.us> wrote:

Yes, the list of rough edges is the 14-steps you have to perform to run
pg_upgrade, as documented in the pg_upgrade manual page:

http://www.postgresql.org/docs/9.2/static/pgupgrade.html

The unknown is how to reduce the number of steps in a way the community
would find acceptable.

I think this is one good idea:

http://archives.postgresql.org/message-id/29806.1340655654@sss.pgh.pa.us

The number of steps is an issue, but the likelihood of the actual
pg_upgrade run failing or doing the wrong thing is also something we
need to work on.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 8, 2012 at 04:23:04PM -0400, Robert Haas wrote:

On Tue, Aug 7, 2012 at 10:59 AM, Bruce Momjian <bruce@momjian.us> wrote:

Yes, the list of rough edges is the 14-steps you have to perform to run
pg_upgrade, as documented in the pg_upgrade manual page:

http://www.postgresql.org/docs/9.2/static/pgupgrade.html

The unknown is how to reduce the number of steps in a way the community
would find acceptable.

I think this is one good idea:

http://archives.postgresql.org/message-id/29806.1340655654@sss.pgh.pa.us

The number of steps is an issue, but the likelihood of the actual
pg_upgrade run failing or doing the wrong thing is also something we
need to work on.

If we currently require 14 steps to use pg_upgrade, how would that
reduce this number? What failures does it fix?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Excerpts from Bruce Momjian's message of mié ago 08 17:15:38 -0400 2012:

On Wed, Aug 8, 2012 at 04:23:04PM -0400, Robert Haas wrote:

On Tue, Aug 7, 2012 at 10:59 AM, Bruce Momjian <bruce@momjian.us> wrote:

Yes, the list of rough edges is the 14-steps you have to perform to run
pg_upgrade, as documented in the pg_upgrade manual page:

http://www.postgresql.org/docs/9.2/static/pgupgrade.html

The unknown is how to reduce the number of steps in a way the community
would find acceptable.

I think this is one good idea:

http://archives.postgresql.org/message-id/29806.1340655654@sss.pgh.pa.us

The number of steps is an issue, but the likelihood of the actual
pg_upgrade run failing or doing the wrong thing is also something we
need to work on.

If we currently require 14 steps to use pg_upgrade, how would that
reduce this number? What failures does it fix?

I think those 14 is a bit of a made-up number. Several of those steps
are about building pg_upgrade, not actually using it. And there are
some that are optional anyway.

The suggestion by Tom reduces the list by two steps because it doesn't
need to adjust pg_hba.conf or put it back in the original way
afterwards.

Another thing worth considering is to have pg_upgrade init, stop and
start clusters as necessary instead of requesting the user to do it.
I think this is two less steps.

I wonder if things would be facilitated by having a config file for
pg_upgrade to specify binary and PGDATA paths instead of having awkward
command line switches. That way you could request the user to create
such a file, then

# this runs the checks, gathers necessary .so files, stops old cluster,
# runs initdb for new cluster
pg_upgrade --config=/somewhere/pg_upgrade.conf --init-new

# now plead the user to install the .so files into the new cluster

# do the actual upgrade
pg_upgrade --config=/somewhere/pg_upgrade.conf --actually-upgrade

You could even have a mode on which pg_upgrade asks for the necessary
information to create the config file. For example

pg_upgrade --create-config=/somewhere/pg_upgrade.conf
Path to new binaries: /usr/local/pg92
Path to old binaries: /usr/local/pg84
Path to old data directory: /srv/pgsql-84/data
Path to new data directory: /srv/pgsql-92/data
Use link mode (y/N): n
... using copy mode.
[now run the checks and complain about missing binaries, etc]

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 8, 2012 at 4:29 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I wonder if things would be facilitated by having a config file for
pg_upgrade to specify binary and PGDATA paths instead of having awkward
command line switches. That way you could request the user to create
such a file, then

i like this idea, when i have used pg_upgrade i have been running it
several times with --check until everything is ok and then the actual
upgrade... so i always create a shell script to run it, a config file
should be a good idea

You could even have a mode on which pg_upgrade asks for the necessary
information to create the config file. For example

pg_upgrade --create-config=/somewhere/pg_upgrade.conf
Path to new binaries: /usr/local/pg92
Path to old binaries: /usr/local/pg84
Path to old data directory: /srv/pgsql-84/data
Path to new data directory: /srv/pgsql-92/data
Use link mode (y/N): n
... using copy mode.
[now run the checks and complain about missing binaries, etc]

even better

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Excerpts from Bruce Momjian's message of mié ago 08 17:15:38 -0400 2012:

On Wed, Aug 8, 2012 at 04:23:04PM -0400, Robert Haas wrote:

I think this is one good idea:
http://archives.postgresql.org/message-id/29806.1340655654@sss.pgh.pa.us

If we currently require 14 steps to use pg_upgrade, how would that
reduce this number? What failures does it fix?

The suggestion by Tom reduces the list by two steps because it doesn't
need to adjust pg_hba.conf or put it back in the original way
afterwards.

Even more to the point, it flat-out eliminates failure modes associated
with somebody connecting to either the old or the new cluster while
pg_upgrade is working. Schemes that involve temporarily hacking
pg_hba.conf can't provide any iron-clad guarantee, because if pg_upgrade
can connect to a postmaster, so can somebody else.

The point I think Robert was trying to make is that we need to cut down
not only the complexity of running pg_upgrade, but the number of failure
modes. At least that's how I'd define improvement here.

regards, tom lane