Re: -Wformat-zero-length

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?

Peter Eisentraut <peter_e(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)momjian(dot)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(at)alvh(dot)no-ip(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

On Wed, Aug 8, 2012 at 05:29:49PM -0400, Alvaro Herrera wrote:
> 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(at)momjian(dot)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.

True.

> 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.

pg_upgrade already starts/stops the server --- it just checks to make
sure they are both stopped.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Wed, Aug 8, 2012 at 06:42:27PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)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.

We now use a temporary port number, 50432.

> 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.

Agreed. Even with these changes, I still see a lot of complexity.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Wed, Aug 8, 2012 at 7:04 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> 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.
>
> Agreed. Even with these changes, I still see a lot of complexity.

I agree. That's why I said it needs some serious engineering time to
file down the rough edges, plural, not that it needs this fix in
particular. This would help to make things less error-prone, but it's
far from the only thing that is needed. As to what exactly is needed,
well that's up for discussion.

One of the big failure modes for pg_upgrade is... pg_dump's dump fails
to restore. That bothers me quite a bit because there are actually a
lot more people who rely on pg_dump than there are people who rely on
pg_upgrade, and it turns out there are all of these edge cases that
pg_dump doesn't actually handle all that well. Sure, you can edit the
dump by hand (if you're not using pg_upgrade) but that sucks.

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

On Thu, Aug 9, 2012 at 09:20:23AM -0400, Robert Haas wrote:
> On Wed, Aug 8, 2012 at 7:04 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> 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.
> >
> > Agreed. Even with these changes, I still see a lot of complexity.
>
> I agree. That's why I said it needs some serious engineering time to
> file down the rough edges, plural, not that it needs this fix in
> particular. This would help to make things less error-prone, but it's
> far from the only thing that is needed. As to what exactly is needed,
> well that's up for discussion.
>
> One of the big failure modes for pg_upgrade is... pg_dump's dump fails
> to restore. That bothers me quite a bit because there are actually a
> lot more people who rely on pg_dump than there are people who rely on
> pg_upgrade, and it turns out there are all of these edge cases that
> pg_dump doesn't actually handle all that well. Sure, you can edit the
> dump by hand (if you're not using pg_upgrade) but that sucks.

100% agree. 9.2 will improve pg_upgrade debugging capabilities and
speed, but there are still many rough edges where that I don't see clear
solutions. Therefore, I don't see pg_upgrade being simple anytime soon,
and hence, it is also unlikely pg_upgrade will be moved to /bin anytime
soon, if we use the same criteria.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On 8/8/12 5:29 PM, Alvaro Herrera wrote:
> 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.

Compare the pg_upgrade instructions

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

to the old pg_dump-based upgrade instructions that we used to give people:

http://www.postgresql.org/docs/8.4/static/install-upgrading.html

They are about the same in complexity (the pg_dump approach didn't talk
about updating statistics or removing the old cluster, so it has less
steps).

So I don't think the number of steps is a problem at all. They could be
simplified a little bit more, of course.

What's more of a problem in my mind is the "unknown unknowns" in
pg_upgrade's approach. In the pg_dump/restore approach, you knew that
if the dump succeeded and the restore succeeded, your new database was
very likely good. The only problem could be that pg_dump forgot to dump
something altogether, or that there is a general problem in executing
SQL commands, both of which would be obvious problems. With pg_upgrade,
however, you never know whether your new instance is good. You could
notice problems months later. That's a really tough proposition.

> 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.

Then you'd need to expose the entire pg_ctl shutdown mode logic through
pg_upgrade, which might not make things simpler.

> 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.

If you want to do that, why not write a shell script? That's what they
are for.

>> 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.
>
> Then you'd need to expose the entire pg_ctl shutdown mode logic through pg_upgrade, which might not make things simpler.

What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?

--
dim

On Sat, Aug 11, 2012 at 01:48:13AM +0200, Dimitri Fontaine wrote:
>
> >> 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.
> >
> > Then you'd need to expose the entire pg_ctl shutdown mode logic through pg_upgrade, which might not make things simpler.
>
> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?

You still need to run libpq applications, so would be doing some process
juggling to get the libpq clients to talk to a pipe.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On 8/10/12 7:48 PM, Dimitri Fontaine wrote:
>
>>> 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.
>>
>> Then you'd need to expose the entire pg_ctl shutdown mode logic through pg_upgrade, which might not make things simpler.
>
> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?

I think that's essentially equivalent to starting the server on a
Unix-domain socket in a private directory. But that has been rejected
because it doesn't work on Windows.

The question in my mind is, is there some other usable way on Windows
for two unrelated processes to communicate over file descriptors in a
private and secure way?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 8/10/12 7:48 PM, Dimitri Fontaine wrote:
>> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?

> I think that's essentially equivalent to starting the server on a
> Unix-domain socket in a private directory. But that has been rejected
> because it doesn't work on Windows.

> The question in my mind is, is there some other usable way on Windows
> for two unrelated processes to communicate over file descriptors in a
> private and secure way?

You're making this unnecessarily hard, because there is no need for the
two processes to be unrelated.

The implementation I'm visualizing is that a would-be client (think psql
or pg_dump, though the code would actually be in libpq) forks off a
process that becomes a standalone backend, and then they communicate
over a pair of pipes that were created before forking. This is
implementable on any platform that supports Postgres, because initdb
already relies on equivalent capabilities.

regards, tom lane

On Tue, Aug 14, 2012 at 05:56:39PM -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On 8/10/12 7:48 PM, Dimitri Fontaine wrote:
> >> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?
>
> > I think that's essentially equivalent to starting the server on a
> > Unix-domain socket in a private directory. But that has been rejected
> > because it doesn't work on Windows.
>
> > The question in my mind is, is there some other usable way on Windows
> > for two unrelated processes to communicate over file descriptors in a
> > private and secure way?
>
> You're making this unnecessarily hard, because there is no need for the
> two processes to be unrelated.
>
> The implementation I'm visualizing is that a would-be client (think psql
> or pg_dump, though the code would actually be in libpq) forks off a
> process that becomes a standalone backend, and then they communicate
> over a pair of pipes that were created before forking. This is
> implementable on any platform that supports Postgres, because initdb
> already relies on equivalent capabilities.

I think the big question is whether we need to modify every binary that
pg_upgrade executes to underestand this pipe communication method.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Tue, Aug 14, 2012 at 05:56:39PM -0400, Tom Lane wrote:
>> The implementation I'm visualizing is that a would-be client (think psql
>> or pg_dump, though the code would actually be in libpq) forks off a
>> process that becomes a standalone backend, and then they communicate
>> over a pair of pipes that were created before forking. This is
>> implementable on any platform that supports Postgres, because initdb
>> already relies on equivalent capabilities.

> I think the big question is whether we need to modify every binary that
> pg_upgrade executes to underestand this pipe communication method.

I think we can fix it once in libpq and we're done. It'd be driven
by some new connection-string option, and the clients as such would
never need to know that they're not talking to a regular postmaster.

regards, tom lane

On Tue, Aug 14, 2012 at 06:53:49PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > On Tue, Aug 14, 2012 at 05:56:39PM -0400, Tom Lane wrote:
> >> The implementation I'm visualizing is that a would-be client (think psql
> >> or pg_dump, though the code would actually be in libpq) forks off a
> >> process that becomes a standalone backend, and then they communicate
> >> over a pair of pipes that were created before forking. This is
> >> implementable on any platform that supports Postgres, because initdb
> >> already relies on equivalent capabilities.
>
> > I think the big question is whether we need to modify every binary that
> > pg_upgrade executes to understand this pipe communication method.
>
> I think we can fix it once in libpq and we're done. It'd be driven
> by some new connection-string option, and the clients as such would
> never need to know that they're not talking to a regular postmaster.

OK, I like the idea if we can make it work. I am unclear how we would
pass this connection thing. Peter's idea of putting the socket in the
current directory is great, except for Windows support. Maybe we
should just implement this for non-Windows.

FYI, we only use new binaries, so we would only need the support in the
new libpq client libraries. However, we can't backpatch this into the
old servers, so I am concerned it will not be possible to implement
this if it requires server changes.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Tue, 2012-08-14 at 17:56 -0400, Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > On 8/10/12 7:48 PM, Dimitri Fontaine wrote:
> >> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?
>
> > I think that's essentially equivalent to starting the server on a
> > Unix-domain socket in a private directory. But that has been rejected
> > because it doesn't work on Windows.
>
> > The question in my mind is, is there some other usable way on Windows
> > for two unrelated processes to communicate over file descriptors in a
> > private and secure way?
>
> You're making this unnecessarily hard, because there is no need for the
> two processes to be unrelated.
>
> The implementation I'm visualizing is that a would-be client (think psql
> or pg_dump, though the code would actually be in libpq) forks off a
> process that becomes a standalone backend, and then they communicate
> over a pair of pipes that were created before forking. This is
> implementable on any platform that supports Postgres, because initdb
> already relies on equivalent capabilities.

Well, that would be an interesting feature, but it's debatable which
among this or just adding a new socket type (if available) is harder.

On Tue, Aug 14, 2012 at 11:54:53PM -0400, Peter Eisentraut wrote:
> On Tue, 2012-08-14 at 17:56 -0400, Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > > On 8/10/12 7:48 PM, Dimitri Fontaine wrote:
> > >> What about having single user mode talk fe/be protocol, and talk to it via a UNIX pipe, with pg_upgrade starting the single user backend as a subprocess?
> >
> > > I think that's essentially equivalent to starting the server on a
> > > Unix-domain socket in a private directory. But that has been rejected
> > > because it doesn't work on Windows.
> >
> > > The question in my mind is, is there some other usable way on Windows
> > > for two unrelated processes to communicate over file descriptors in a
> > > private and secure way?
> >
> > You're making this unnecessarily hard, because there is no need for the
> > two processes to be unrelated.
> >
> > The implementation I'm visualizing is that a would-be client (think psql
> > or pg_dump, though the code would actually be in libpq) forks off a
> > process that becomes a standalone backend, and then they communicate
> > over a pair of pipes that were created before forking. This is
> > implementable on any platform that supports Postgres, because initdb
> > already relies on equivalent capabilities.
>
> Well, that would be an interesting feature, but it's debatable which
> among this or just adding a new socket type (if available) is harder.

TODO added:

Find cleaner way to start/stop dedicated servers for upgrades

http://archives.postgresql.org/pgsql-hackers/2012-08/msg00275.php

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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