[PostgreSQL-Hackers] -Wformat-zero-length

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

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

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?

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?

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.

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.

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.

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]

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
even better

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

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.

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

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.
Then you'd need to expose the entire pg_ctl shutdown mode logic through
pg_upgrade, which might not make things simpler.
If you want to do that, why not write a shell script? That's what they
are for.

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.

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

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

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.

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