Thread: -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@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. +
Attachment
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. +
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. +
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 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 HaganderMe: 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
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@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. 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@momjian.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@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. 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@momjian.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@momjian.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@momjian.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@momjian.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 singleuser 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 singleuser 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@momjian.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 singleuser 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@gmx.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
singleuser 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@gmx.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 thesingle 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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.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@momjian.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@momjian.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@gmx.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 thesingle 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@gmx.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 thesingle 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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +