Thread: moving from contrib to bin
Let's take another crack at moving stuff out of contrib. Nobody likes contrib. The task is only finding something that most people like better. Last time this was attempted, the discussion got lost in exactly which extensions are worthy enough to be considered official or something like that. I want to dodge that here by starting at the opposite end: 1. move programs to src/bin/ 2. move test things to src/test/ (I had that in my notes, but someone already did that, so the stars must be aligned.) 3. deal with extensions later Here are the contrib programs: oid2name pg_archivecleanup pg_standby pg_test_fsync pg_test_timing pg_upgrade pg_xlogdump pgbench vacuumlo The proposal would basically be to mv contrib/$x src/bin/$x and also move the reference pages in the documentation. We could consider alternative arrangements, if there is interest, such as moving vacuumlo to scripts or moving pg_archivecleanup and pg_standby into one directory. It doesn't matter very much to me. There is precedent for this: Some time ago we moved reindexdb from contrib to scripts. Besides moving things out of contrib, there is also a practical motivation for this. Putting both client and server programs into contrib creates issues for packagers. One would have to create a separate -contrib-client package to package this properly. Not to mention packaging a bunch of unrelated extensions with these programs. If we make these normal programs in src/bin/, packagers can put them into the normal -client and -server packages, and everything will fall into place. Besides, a number of packagers have been treating pg_upgrade specially and moved it out of contrib, so this would just be catching up with reality a bit. Comments?
Peter Eisentraut <peter_e@gmx.net> writes:
> Last time this was attempted, the discussion got lost in exactly which
> extensions are worthy enough to be considered official or something like
> that. I want to dodge that here by starting at the opposite end:
> 1. move programs to src/bin/
> Here are the contrib programs:
> oid2name
> pg_archivecleanup
> pg_standby
> pg_test_fsync
> pg_test_timing
> pg_upgrade
> pg_xlogdump
> pgbench
> vacuumlo
> The proposal would basically be to mv contrib/$x src/bin/$x and also
> move the reference pages in the documentation.
Personally, I'm good with moving pg_archivecleanup, pg_standby,
pg_upgrade, pg_xlogdump, and pgbench this way. (Although wasn't there
just some discussion about pg_standby being obsolete? If so, shouldn't
we remove it instead of promoting it?) As for the others:
I'm not exactly convinced that we want to encourage packagers to include
either pg_test_fsync or pg_test_timing in standard packages. They are not
all that useful to ordinary users.
oid2name and vacuumlo, besides being of very dubious general utility,
are fails from a namespacing standpoint. If we were to promote them
into standard install components I think a minimum requirement should be
to rename them to pg_something. (oid2name is an entirely bogus name for
what it does, anyway.) That would also be a good opportunity to revisit
their rather-ad-hoc APIs.
regards, tom lane
On 2014-12-08 22:50:30 -0500, Tom Lane wrote: > I'm not exactly convinced that we want to encourage packagers to include > either pg_test_fsync or pg_test_timing in standard packages. They are not > all that useful to ordinary users. I actually think both are quite useful when setting up new systems to quickly screen for problems. There still is a fairly large number of virtualized systems with pretty much broken timing functions; and checking whether fsync actually takes some time is also good thing to do in virtualized environments - it's not an infrequent thing to see fsyncs taking unrealistically low time. Neither is likely to be harmful. So it doesn't seem harmful to move them. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Dec 8, 2014 at 9:00 PM, Andres Freund <andres@2ndquadrant.com> wrote: > I actually think both are quite useful when setting up new systems to > quickly screen for problems. There still is a fairly large number of > virtualized systems with pretty much broken timing functions; and > checking whether fsync actually takes some time is also good thing to do > in virtualized environments - it's not an infrequent thing to see fsyncs > taking unrealistically low time. > > Neither is likely to be harmful. So it doesn't seem harmful to move > them. +1 -- Peter Geoghegan
> Here are the contrib programs: > > oid2name > pg_archivecleanup > pg_standby > pg_test_fsync > pg_test_timing > pg_upgrade > pg_xlogdump > pgbench > vacuumlo > > The proposal would basically be to mv contrib/$x src/bin/$x and also > move the reference pages in the documentation. +1 Considering that all of the above have been around for a while, it's kind of silly that they're still in contrib. Mostly that just guarantees that nobody will use them, even when it's appropriate. The one exception I might make above is pg_standby. What do we need this for today, exactly? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Mon, Dec 8, 2014 at 10:26 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > Let's take another crack at moving stuff out of contrib. Nobody likes > contrib. The task is only finding something that most people like better. I like contrib fine. It's a great place for things to live that are not quite baked enough for core, but still worth distributing. > oid2name > pg_archivecleanup > pg_standby > pg_test_fsync > pg_test_timing > pg_upgrade > pg_xlogdump > pgbench > vacuumlo > > The proposal would basically be to mv contrib/$x src/bin/$x and also > move the reference pages in the documentation. I think pg_archivecleanup, pg_upgrade, and possibly pgbench have enough general utility to justify putting them in src/bin, but not the rest. oid2name, pg_standby, and vacuumlo are probably closer to being candidates for removal than promotion in my book. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Peter Eisentraut wrote: > Here are the contrib programs: > > oid2name > pg_archivecleanup > pg_standby > pg_test_fsync > pg_test_timing > pg_upgrade > pg_xlogdump > pgbench > vacuumlo > > The proposal would basically be to mv contrib/$x src/bin/$x and also > move the reference pages in the documentation. Maybe it makes sense to have a distinction between client programs and server programs. Can we have src/sbin/ and move stuff that involves the server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we could also move pg_resetxlog, pg_controldata and initdb there.) (For pg_upgrade you also need to do something about pg_upgrade_support, which is good because that is one very ugly crock.) I agree that oid2name and vacuumlo need to be in a better state to deserve their promotion to src/bin, if we keep them at all. In any case I support the move out of contrib of everything except vacuumlo and oid2name, for reasons already stated by others in the thread. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Dec 9, 2014 at 06:10:02PM -0300, Alvaro Herrera wrote: > (For pg_upgrade you also need to do something about pg_upgrade_support, > which is good because that is one very ugly crock.) FYI, pg_upgrade_support was segregated from pg_upgrade only because we wanted separate binary and shared object build/install targets. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 12/9/14 1:18 PM, Josh Berkus wrote: > The one exception I might make above is pg_standby. What do we need > this for today, exactly? This was discussed recently and people wanted to keep it.
On 12/9/14 4:10 PM, Alvaro Herrera wrote: > Maybe it makes sense to have a distinction between client programs and > server programs. Can we have src/sbin/ and move stuff that involves the > server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, > pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we > could also move pg_resetxlog, pg_controldata and initdb there.) I was thinking about that. What do others think?
On 12/9/14 4:32 PM, Bruce Momjian wrote: > On Tue, Dec 9, 2014 at 06:10:02PM -0300, Alvaro Herrera wrote: >> (For pg_upgrade you also need to do something about pg_upgrade_support, >> which is good because that is one very ugly crock.) > > FYI, pg_upgrade_support was segregated from pg_upgrade only because we > wanted separate binary and shared object build/install targets. I think the actual reason is that the makefile structure won't let you have them both in the same directory. I don't see why you would need separate install targets. How about we move these support functions into the backend? It's not like we don't already have other pg_upgrade hooks baked in all over the place.
On 12/12/2014 03:07 PM, Peter Eisentraut wrote: > On 12/9/14 4:10 PM, Alvaro Herrera wrote: >> Maybe it makes sense to have a distinction between client programs and >> server programs. Can we have src/sbin/ and move stuff that involves the >> server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, >> pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we >> could also move pg_resetxlog, pg_controldata and initdb there.) > > I was thinking about that. What do others think? Sounds good. We already separate server and client programs in the docs, and packagers put them in different packages too. This should make packagers' life a little bit easier in the long run. - Heikki
On 12/12/2014 03:11 PM, Heikki Linnakangas wrote: > On 12/12/2014 03:07 PM, Peter Eisentraut wrote: >> On 12/9/14 4:10 PM, Alvaro Herrera wrote: >>> Maybe it makes sense to have a distinction between client programs and >>> server programs. Can we have src/sbin/ and move stuff that involves the >>> server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, >>> pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we >>> could also move pg_resetxlog, pg_controldata and initdb there.) >> >> I was thinking about that. What do others think? > > Sounds good. We already separate server and client programs in the docs, > and packagers put them in different packages too. This should make > packagers' life a little bit easier in the long run. src/sbin might not be a good name for the directory, though. We're not going to install the programs in /usr/sbin, are we? Maybe src/server-bin and src/client-bin. - Heikki
On 2014-12-12 15:11:01 +0200, Heikki Linnakangas wrote: > On 12/12/2014 03:07 PM, Peter Eisentraut wrote: > >On 12/9/14 4:10 PM, Alvaro Herrera wrote: > >>Maybe it makes sense to have a distinction between client programs and > >>server programs. Can we have src/sbin/ and move stuff that involves the > >>server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, > >>pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we > >>could also move pg_resetxlog, pg_controldata and initdb there.) > > > >I was thinking about that. What do others think? > > Sounds good. We already separate server and client programs in the docs, and > packagers put them in different packages too. This should make packagers' > life a little bit easier in the long run. Wouldn't a make install-server/client targets or something similar actually achieve the same thing? Seems simpler to maintain to me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Dec 12, 2014 at 03:13:44PM +0200, Heikki Linnakangas wrote: > On 12/12/2014 03:11 PM, Heikki Linnakangas wrote: > >On 12/12/2014 03:07 PM, Peter Eisentraut wrote: > >>On 12/9/14 4:10 PM, Alvaro Herrera wrote: > >>>Maybe it makes sense to have a distinction between client programs and > >>>server programs. Can we have src/sbin/ and move stuff that involves the > >>>server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, > >>>pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we > >>>could also move pg_resetxlog, pg_controldata and initdb there.) > >> > >>I was thinking about that. What do others think? > > > >Sounds good. We already separate server and client programs in the docs, > >and packagers put them in different packages too. This should make > >packagers' life a little bit easier in the long run. > > src/sbin might not be a good name for the directory, though. We're > not going to install the programs in /usr/sbin, are we? Maybe > src/server-bin and src/client-bin. I am confused by the above because you are mixing /src and /bin. If we install the binaries in new directories, that is going to require multiple adjustments to $PATH --- that doesn't seem like a win, and we only have 25 binaries in pgsql/bin now (my Debian /usr/bin has 2306 binaries). I assume I am misunderstanding something. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Fri, Dec 12, 2014 at 08:11:31AM -0500, Peter Eisentraut wrote: > On 12/9/14 4:32 PM, Bruce Momjian wrote: > > On Tue, Dec 9, 2014 at 06:10:02PM -0300, Alvaro Herrera wrote: > >> (For pg_upgrade you also need to do something about pg_upgrade_support, > >> which is good because that is one very ugly crock.) > > > > FYI, pg_upgrade_support was segregated from pg_upgrade only because we > > wanted separate binary and shared object build/install targets. > > I think the actual reason is that the makefile structure won't let you > have them both in the same directory. I don't see why you would need > separate install targets. > > How about we move these support functions into the backend? It's not > like we don't already have other pg_upgrade hooks baked in all over the > place. Yes, we can easily do that, and it makes sense. The functions are already protected to not do anything unless the server is in binary upgrade mode. If we move them into the backend I think we need to add a super-user check as well. The reason we don't have one now is that they are installed/uninstalled by the super-user as part of the pg_upgrade process. Moving pg_upgrade out of contrib is going to give me additional gloating opportunities at conferences. :-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 12/8/14 10:50 PM, Tom Lane wrote: > oid2name and vacuumlo, besides being of very dubious general utility, > are fails from a namespacing standpoint. If we were to promote them > into standard install components I think a minimum requirement should be > to rename them to pg_something. (oid2name is an entirely bogus name for > what it does, anyway.) That would also be a good opportunity to revisit > their rather-ad-hoc APIs. I'm going to leave these two out for now. I'll start investigating whether they can be removed, or replaced by something else.
On 12/12/14 8:13 AM, Andres Freund wrote: > Wouldn't a make install-server/client targets or something similar > actually achieve the same thing? Seems simpler to maintain to me. Adding non-standard makefile targets comes with its own set of maintenance issues. Restructuring the source tree and having the existing makefile structure just work might end up being simpler. Just to be clear, I'm far from convinced that any of this is worthwhile; I'm just keeping the conversation going.
Bruce Momjian wrote: > On Fri, Dec 12, 2014 at 03:13:44PM +0200, Heikki Linnakangas wrote: > > On 12/12/2014 03:11 PM, Heikki Linnakangas wrote: > > >Sounds good. We already separate server and client programs in the docs, > > >and packagers put them in different packages too. This should make > > >packagers' life a little bit easier in the long run. > > > > src/sbin might not be a good name for the directory, though. We're > > not going to install the programs in /usr/sbin, are we? Maybe > > src/server-bin and src/client-bin. > > I am confused by the above because you are mixing /src and /bin. If we > install the binaries in new directories, that is going to require > multiple adjustments to $PATH --- that doesn't seem like a win, and we > only have 25 binaries in pgsql/bin now (my Debian /usr/bin has 2306 > binaries). I assume I am misunderstanding something. We already have src/bin/; the mixture of "src/" and "bin/" predates us. Of course, the stuff we keep in there is not binaries but source code that produces binaries. As for src/sbin/, we wouldn't install anything to the system's /usr/sbin/ of course, only /usr/bin/, just like the stuff in src/bin/. But it would be slightly more clear what we keep in each src/ subdir. I think our current src/bin/ is a misnomer, but it seems late to fix that. In a greenfield I think we could have "src/clients/" and "src/srvtools/" or something like that, and everything would install to /usr/bin. Then there would be no doubt where to move each program from contrib. Maybe there is no point to all of this and we should just move it all to src/bin/ as originally proposed, which is simpler anyway. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2014-12-12 11:27:01 -0300, Alvaro Herrera wrote: > We already have src/bin/; the mixture of "src/" and "bin/" predates us. > Of course, the stuff we keep in there is not binaries but source code > that produces binaries. > > As for src/sbin/, we wouldn't install anything to the system's > /usr/sbin/ of course, only /usr/bin/, just like the stuff in src/bin/. > But it would be slightly more clear what we keep in each src/ subdir. I think sbin is a spectactularly bad name, let's not go there. If anything, make it srvbin or something like that. > I think our current src/bin/ is a misnomer, but it seems late to fix > that. In a greenfield I think we could have "src/clients/" and > "src/srvtools/" or something like that, and everything would install to > /usr/bin. Then there would be no doubt where to move each program from > contrib. Maybe. We could just do that now - git's file change tracking is good enough for that kind of move. > Maybe there is no point to all of this and we should just move it all to > src/bin/ as originally proposed, which is simpler anyway. +1. Packagers already don't use the current boundaries for packaging... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 12/12/2014 03:07 PM, Peter Eisentraut wrote:
>> On 12/9/14 4:10 PM, Alvaro Herrera wrote:
>>> Maybe it makes sense to have a distinction between client programs and
>>> server programs. Can we have src/sbin/ and move stuff that involves the
>>> server side in there? I think that'd be pg_xlogdump, pg_archivecleanup,
>>> pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we
>>> could also move pg_resetxlog, pg_controldata and initdb there.)
>> I was thinking about that. What do others think?
> Sounds good. We already separate server and client programs in the docs,
> and packagers put them in different packages too. This should make
> packagers' life a little bit easier in the long run.
I'm pretty much -1 on relocating anything that's under src/bin already.
The history mess and back-patching pain would outweigh any notional
cleanliness --- and AFAICS it's entirely notional. As an ex-packager
I can tell you that where stuff sits in the source tree makes precisely
*zero* difference to a packager. She's going to do "make install-world"
and then her package recipe will list out which files in the install tree
go into which sub-package. Perhaps it would get clearer to packagers if
we also installed stuff into $INSTALLDIR/sbin, but I doubt that such a
change is going to fly with anyone else. The bin vs sbin distinction
is not universal.
regards, tom lane
Peter Eisentraut <peter_e@gmx.net> writes:
> On 12/9/14 4:32 PM, Bruce Momjian wrote:
>> On Tue, Dec 9, 2014 at 06:10:02PM -0300, Alvaro Herrera wrote:
>>> (For pg_upgrade you also need to do something about pg_upgrade_support,
>>> which is good because that is one very ugly crock.)
>> FYI, pg_upgrade_support was segregated from pg_upgrade only because we
>> wanted separate binary and shared object build/install targets.
> I think the actual reason is that the makefile structure won't let you
> have them both in the same directory. I don't see why you would need
> separate install targets.
> How about we move these support functions into the backend? It's not
> like we don't already have other pg_upgrade hooks baked in all over the
> place.
I don't particularly object to having the C code built into the backend;
there's not that much of it, and if we could static-ize some of the global
variables that are involved presently, it'd be a Good Thing IMO. However,
the current arrangement makes sure that the function are not accessible
except during pg_upgrade, and that seems like a Good Thing as well. So
I think pg_upgrade should continue to have SQL scripts that create and
delete the SQL function definitions for these.
regards, tom lane
Peter Eisentraut <peter_e@gmx.net> writes:
> On 12/12/14 8:13 AM, Andres Freund wrote:
>> Wouldn't a make install-server/client targets or something similar
>> actually achieve the same thing? Seems simpler to maintain to me.
> Adding non-standard makefile targets comes with its own set of
> maintenance issues.
It would be of zero value to packagers anyway; certainly so for those
following the Red Hat tradition, in which you tell the package Makefile
to install everything and then what goes into which subpackage is
sorted out in a separate, subsequent step. Possibly Debian or other
packaging infrastructures do it differently, but I doubt that.
Really, if we want to tell packagers that foo is a client program and
bar is a server-side program, the documentation is where to address it.
regards, tom lane
On 2014-12-12 10:20:58 -0500, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > On 12/12/14 8:13 AM, Andres Freund wrote: > >> Wouldn't a make install-server/client targets or something similar > >> actually achieve the same thing? Seems simpler to maintain to me. > > > Adding non-standard makefile targets comes with its own set of > > maintenance issues. > > It would be of zero value to packagers anyway; certainly so for those > following the Red Hat tradition, in which you tell the package Makefile > to install everything and then what goes into which subpackage is > sorted out in a separate, subsequent step. Possibly Debian or other > packaging infrastructures do it differently, but I doubt that. Debian has that step as well - you don't really have to use it, but the postgres debian packages do so. They already don't adhere to the current distinction. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Tom Lane wrote: > Heikki Linnakangas <hlinnakangas@vmware.com> writes: > > On 12/12/2014 03:07 PM, Peter Eisentraut wrote: > >> On 12/9/14 4:10 PM, Alvaro Herrera wrote: > >>> Maybe it makes sense to have a distinction between client programs and > >>> server programs. Can we have src/sbin/ and move stuff that involves the > >>> server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, > >>> pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we > >>> could also move pg_resetxlog, pg_controldata and initdb there.) > > >> I was thinking about that. What do others think? > > > Sounds good. We already separate server and client programs in the docs, > > and packagers put them in different packages too. This should make > > packagers' life a little bit easier in the long run. > > I'm pretty much -1 on relocating anything that's under src/bin already. So let's put the whole bunch under src/bin/ and be done with it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 12, 2014 at 11:00 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> I'm pretty much -1 on relocating anything that's under src/bin already. I agree. I can't see packagers putting it anywhere except for $SOMETHING/bin in the final install, so what do we get out of dividing it up in some weird way in our tree? > So let's put the whole bunch under src/bin/ and be done with it. I'm not really convinced this is a very good idea. What do we get out of moving everything, or even anything, from contrib? It will make back-patching harder, but more importantly, it will possibly create the false impression that everything we distribute is on equal footing. Right now, we've got stuff like vacuumlo in contrib which is useful but, let's face it, also a cheap hack. If we decide that executables can no longer live in contrib, then every time somebody submits something in the future, we've got to decide whether it deserves parity with psql and pg_dump or whether we shouldn't include it at all. contrib is a nice middle-ground. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12/08/2014 07:50 PM, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: >> Last time this was attempted, the discussion got lost in exactly which >> extensions are worthy enough to be considered official or something like >> that. I want to dodge that here by starting at the opposite end: >> 1. move programs to src/bin/ > >> Here are the contrib programs: > >> oid2name >> pg_archivecleanup >> pg_standby >> pg_test_fsync >> pg_test_timing >> pg_upgrade >> pg_xlogdump >> pgbench >> vacuumlo > >> The proposal would basically be to mv contrib/$x src/bin/$x and also >> move the reference pages in the documentation. > > Personally, I'm good with moving pg_archivecleanup, pg_standby, > pg_upgrade, pg_xlogdump, and pgbench this way. (Although wasn't there > just some discussion about pg_standby being obsolete? If so, shouldn't > we remove it instead of promoting it?) As for the others: > Let's not forget pg_upgrade which is arguably the most important of everything listed. JD -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, @cmdpromptinc "If we send our children to Caesar for their education, we should not be surprised when they come back as Romans."
Robert Haas <robertmhaas@gmail.com> writes:
> I'm not really convinced this is a very good idea. What do we get out
> of moving everything, or even anything, from contrib? It will make
> back-patching harder, but more importantly, it will possibly create
> the false impression that everything we distribute is on equal
> footing. Right now, we've got stuff like vacuumlo in contrib which is
> useful but, let's face it, also a cheap hack. If we decide that
> executables can no longer live in contrib, then every time somebody
> submits something in the future, we've got to decide whether it
> deserves parity with psql and pg_dump or whether we shouldn't include
> it at all. contrib is a nice middle-ground.
Yeah, that's a good point. I think part of the motivation here is the
thought that some of these programs, like pg_upgrade, *should* now be
considered on par with pg_dump et al. But it does not follow that
everything in contrib is, or should be, on that level.
regards, tom lane
On 2014-12-12 11:14:56 -0500, Robert Haas wrote: > I'm not really convinced this is a very good idea. What do we get out > of moving everything, or even anything, from contrib? The benefit of moving relevant stuff is that it'll actually be installed by default when installing postgres on many platforms. That's currently often not the case. The contrib umbrella, as used by many other projects, actually justifies not doing so. I don't think that's a good argument for moving everything, rather the contrary, but relevant stuff that we properly support should imo be moved. > It will make back-patching harder I think the amount of effort a simple renamed directory which wholly contains a binary creates is acceptable. Just use patch -p4 instead of patch -p1... > Right now, we've got stuff like vacuumlo in contrib which is > useful but, let's face it, also a cheap hack. On the other hand, we really don't provide any other solution. Since large objects are part of core we really ought to provide at least some support for cleanup. > If we decide that executables can no longer live in contrib, then > every time somebody submits something in the future, we've got to > decide whether it deserves parity with psql and pg_dump or whether we > shouldn't include it at all. contrib is a nice middle-ground. I think it makes sense to still have it as a middleground for future things. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Dec 12, 2014 at 11:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm not really convinced this is a very good idea. What do we get out >> of moving everything, or even anything, from contrib? It will make >> back-patching harder, but more importantly, it will possibly create >> the false impression that everything we distribute is on equal >> footing. Right now, we've got stuff like vacuumlo in contrib which is >> useful but, let's face it, also a cheap hack. If we decide that >> executables can no longer live in contrib, then every time somebody >> submits something in the future, we've got to decide whether it >> deserves parity with psql and pg_dump or whether we shouldn't include >> it at all. contrib is a nice middle-ground. > > Yeah, that's a good point. I think part of the motivation here is the > thought that some of these programs, like pg_upgrade, *should* now be > considered on par with pg_dump et al. But it does not follow that > everything in contrib is, or should be, on that level. Yeah. We have put enough effort collectively into pg_upgrade that I think it's fair to say that it is on a part with pg_dump. I still think the architecture there is awfully fragile and we should try to improve it, but it's very widely-used and people rely on it to work, which it generally does. And certainly we have put a lot of sweat into making it work. I would also say that pg_archivecleanup is a fundamental server tool and that it belongs in src/bin. But after that, I get fuzzy. For me, the next tier of things would consist of pgbench, pg_test_fsync, pg_test_timing, and pg_xlogdump. Those are all useful, but I would also classify them as optional. If you are running a PostgreSQL installation, you definitely need initdb and postgres and pg_dump and pg_dumpall and psql, but you don't definitely need these. I think they are all robust enough to go in src/bin, but they are not as necessary as much of the stuff that is in that directory today, so it's unclear to me whether we want to put them there. Finally, there is the stuff that is either hacky or deprecated: oid2name, pg_standby, vacuumlo. Putting that stuff in src/bin clearly makes no sense IMV. But I wouldn't necessarily want to remove it all either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Dec 12, 2014 at 11:40 AM, Andres Freund <andres@2ndquadrant.com> wrote: > The benefit of moving relevant stuff is that it'll actually be installed > by default when installing postgres on many platforms. That's currently > often not the case. The contrib umbrella, as used by many other > projects, actually justifies not doing so. Agreed. See my other response for my thoughts on that topic. >> It will make back-patching harder > > I think the amount of effort a simple renamed directory which wholly > contains a binary creates is acceptable. Just use patch -p4 instead of > patch -p1... That is fine if you are manually applying a patch that touches only that directory, but if the patch also touches other stuff then it's not as simple. And I don't know how well git cherry-pick will follow the moves. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-12-12 11:42:57 -0500, Robert Haas wrote: > > I think the amount of effort a simple renamed directory which wholly > > contains a binary creates is acceptable. Just use patch -p4 instead of > > patch -p1... > > That is fine if you are manually applying a patch that touches only > that directory, but if the patch also touches other stuff then it's > not as simple. I think backpatchable commits that touch individual binaries and other code at the same time are (and ought to be!) pretty rare. > And I don't know how well git cherry-pick will follow > the moves. Not well if the patch is done in master first. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-12-12 11:42:57 -0500, Robert Haas wrote:
>> And I don't know how well git cherry-pick will follow
>> the moves.
> Not well if the patch is done in master first.
FWIW, I always patch master first, and have zero intention of changing
that workflow. (I have given reasons for that in the past, and don't
feel like repeating them right now.) So I'm really not on board with
moving code around without *very* good reasons. This thread hasn't
done very well at coming up with good reasons to move stuff out of
contrib.
In the particular case of pg_upgrade, while it may be now on par
usefulness-wise with src/bin stuff, I think it is and always will
be a special case anyway so far as packagers are concerned; the
reason being that it needs to ride along with back-branch executables.
So I'm not sure that we're making their lives easier by moving it.
regards, tom lane
On Fri, Dec 12, 2014 at 10:16:05AM -0500, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > On 12/9/14 4:32 PM, Bruce Momjian wrote: > >> On Tue, Dec 9, 2014 at 06:10:02PM -0300, Alvaro Herrera wrote: > >>> (For pg_upgrade you also need to do something about pg_upgrade_support, > >>> which is good because that is one very ugly crock.) > > >> FYI, pg_upgrade_support was segregated from pg_upgrade only because we > >> wanted separate binary and shared object build/install targets. > > > I think the actual reason is that the makefile structure won't let you > > have them both in the same directory. I don't see why you would need > > separate install targets. > > > How about we move these support functions into the backend? It's not > > like we don't already have other pg_upgrade hooks baked in all over the > > place. > > I don't particularly object to having the C code built into the backend; > there's not that much of it, and if we could static-ize some of the global > variables that are involved presently, it'd be a Good Thing IMO. However, > the current arrangement makes sure that the function are not accessible > except during pg_upgrade, and that seems like a Good Thing as well. So > I think pg_upgrade should continue to have SQL scripts that create and > delete the SQL function definitions for these. Oh, hmmm, would pg_upgrade_support still be a separate shared object file, or would we just link to functions that already exist in the backend binary, i.e. it is just the SQL-callabiity you want pg_upgrade to do? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Robert Haas wrote: > On Fri, Dec 12, 2014 at 11:00 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > So let's put the whole bunch under src/bin/ and be done with it. > > I'm not really convinced this is a very good idea. What do we get out > of moving everything, or even anything, from contrib? We show that it's no longer "contrib" (== possibly low quality) stuff anymore. At the beginning of pg_upgrade, for example, we didn't want it in src/bin because it wasn't stable enough, it was full of bugs, there were always going to be scenarios it wouldn't handle. Now that is all gone, so we promote it to the next status level. > It will make back-patching harder, Yes. We can deal with that. It's not that hard anyway. > but more importantly, it will possibly create the false impression > that everything we distribute is on equal footing. Stuff in contrib is of lower quality. Some items have improved enough that we can let them out of that sack now. What we're doing is create the correct impression that stuff that's no longer in contrib is of better quality than what remains in contrib. > Right now, we've got stuff like vacuumlo in contrib which is > useful but, let's face it, also a cheap hack. Then we don't move vacuumlo. I agree we shouldn't move it. (And neither oid2names.) > If we decide that executables can no longer live in contrib, Nobody is saying that. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Andres Freund 2014-12-12 <20141212152723.GO31413@awork2.anarazel.de> > On 2014-12-12 10:20:58 -0500, Tom Lane wrote: > > Peter Eisentraut <peter_e@gmx.net> writes: > > > On 12/12/14 8:13 AM, Andres Freund wrote: > > >> Wouldn't a make install-server/client targets or something similar > > >> actually achieve the same thing? Seems simpler to maintain to me. Ack. The default install location would still be .../bin, but invoked from different targets. > > > Adding non-standard makefile targets comes with its own set of > > > maintenance issues. > > > > It would be of zero value to packagers anyway; certainly so for those > > following the Red Hat tradition, in which you tell the package Makefile > > to install everything and then what goes into which subpackage is > > sorted out in a separate, subsequent step. Possibly Debian or other > > packaging infrastructures do it differently, but I doubt that. > > Debian has that step as well - you don't really have to use it, but the > postgres debian packages do so. They already don't adhere to the current > distinction. The standard Debian package installs into debian/tmp/ and then picks files from there into individual packages. However, for PostgreSQL this means lengthy debian/*.install files (the equivalent of %files in rpm spec speak): $ wc -l debian/*.install 2 debian/libecpg6.install 1 debian/libecpg-compat3.install 17 debian/libecpg-dev.install 1 debian/libpgtypes3.install 2 debian/libpq5.install 14 debian/libpq-dev.install 39 debian/postgresql-9.4.install 40 debian/postgresql-client-9.4.install65 debian/postgresql-contrib-9.4.install 2 debian/postgresql-doc-9.4.install 3 debian/postgresql-plperl-9.4.install 2 debian/postgresql-plpython3-9.4.install 3 debian/postgresql-plpython-9.4.install 5 debian/postgresql-pltcl-9.4.install 3 debian/postgresql-server-dev-9.4.install199total If there were separate "install-client", "install-server", and "install-contrib" targets, that would probably shorten those files quite a bit. Especially messy is the part where *.so needs to be sorted into server/contrib, along with an similar large bunch of binaries. Of course that would only solve part of the problem (I'm not going to suggest creating 15 targets for the 15 binary packages we are building), but it would solve the uglier part. Christoph -- cb@df7cb.de | http://www.df7cb.de/
Christoph Berg <cb@df7cb.de> writes:
> However, for PostgreSQL this means lengthy debian/*.install files
> (the equivalent of %files in rpm spec speak):
Right ...
> If there were separate "install-client", "install-server", and
> "install-contrib" targets, that would probably shorten those files
> quite a bit. Especially messy is the part where *.so needs to be
> sorted into server/contrib, along with an similar large bunch of
> binaries.
Pardon me for not knowing much about Debian packages, but how would
that work exactly? Is it possible to do make install-client, then
package the installed files, then rm -rf the install tree, then
repeat for install-server and install-contrib? In the RPM world
this would never work because the build/install step happens in
toto before the packaging step. Even without that, it seems like
it'd be hard to make it entirely automatic since some files would
be installed in multiple cases (and directories even more so).
regards, tom lane
Tom Lane wrote: > Christoph Berg <cb@df7cb.de> writes: > > However, for PostgreSQL this means lengthy debian/*.install files > > (the equivalent of %files in rpm spec speak): > > Right ... > > > If there were separate "install-client", "install-server", and > > "install-contrib" targets, that would probably shorten those files > > quite a bit. Especially messy is the part where *.so needs to be > > sorted into server/contrib, along with an similar large bunch of > > binaries. > > Pardon me for not knowing much about Debian packages, but how would > that work exactly? Is it possible to do make install-client, then > package the installed files, then rm -rf the install tree, then > repeat for install-server and install-contrib? In the RPM world > this would never work because the build/install step happens in > toto before the packaging step. Uh, couldn't you just run "make install-client DESTDIR=.../client" for client-only files, and so on? You would end up with separate directories containing files for each subpackage. > Even without that, it seems like it'd be hard to make it entirely > automatic since some files would be installed in multiple cases (and > directories even more so). Yeah, you would need to fix that somehow. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 12, 2014 at 12:19 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> If we decide that executables can no longer live in contrib, > > Nobody is saying that. The reason I though that might be on the table is that the first post on this thread, at least as I understood it, proposed to move every executable out of contrib, which would seem to also imply that we wouldn't put any new ones there. I agree that course of action doesn't seem to be garnering many votes, but I think it does seem to have been proposed, unless I am confused. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Dec 13, 2014 at 1:00 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Tom Lane wrote: >> Heikki Linnakangas <hlinnakangas@vmware.com> writes: >> > On 12/12/2014 03:07 PM, Peter Eisentraut wrote: >> >> On 12/9/14 4:10 PM, Alvaro Herrera wrote: >> >>> Maybe it makes sense to have a distinction between client programs and >> >>> server programs. Can we have src/sbin/ and move stuff that involves the >> >>> server side in there? I think that'd be pg_xlogdump, pg_archivecleanup, >> >>> pg_upgrade, pg_test_timing, pg_test_fsync. (If we were feeling bold we >> >>> could also move pg_resetxlog, pg_controldata and initdb there.) >> >> >> I was thinking about that. What do others think? >> >> > Sounds good. We already separate server and client programs in the docs, >> > and packagers put them in different packages too. This should make >> > packagers' life a little bit easier in the long run. >> >> I'm pretty much -1 on relocating anything that's under src/bin already. > > So let's put the whole bunch under src/bin/ and be done with it. When doing so let's be careful with the MSVC specs, this will need a set of definitions with AddSimpleFrontend. This should ensure as well that versioning work that has been done on Windows for this release cycle is not broken for all the things moved. I don't mind double-checking that.. -- Michael
On 12/12/14 10:16 AM, Tom Lane wrote: > I don't particularly object to having the C code built into the backend; > there's not that much of it, and if we could static-ize some of the global > variables that are involved presently, it'd be a Good Thing IMO. However, > the current arrangement makes sure that the function are not accessible > except during pg_upgrade, and that seems like a Good Thing as well. So > I think pg_upgrade should continue to have SQL scripts that create and > delete the SQL function definitions for these. That won't actually work very easily. LANGUAGE internal functions need to be in fmgr_builtins, and the only way to get them there is by listing them in pg_proc.h. (We could drop the functions in initdb, but seems kind of silly.) The functions do already check themselves that they are called in binary upgrade mode, so exposing them in pg_proc doesn't seem risky.
On 12/12/14 3:20 PM, Tom Lane wrote: > Pardon me for not knowing much about Debian packages, but how would > that work exactly? Is it possible to do make install-client, then > package the installed files, then rm -rf the install tree, then > repeat for install-server and install-contrib? In the RPM world > this would never work because the build/install step happens in > toto before the packaging step. I don't know exactly what Christoph had in mind, but the short answer to your question is: Yes, that is possible. Almost anything is possible. There are many tools available at various levels of abstraction that facility common patterns, but you can also do everything by hand, as long as you produce the right *.deb files at the right location at the end. The only thing stopping you is your sanity. However, other packaging systems clearly don't work that way, and so designing something that would only work for one packaging system doesn't seem worthwhile. And we don't even know whether and how it would work anyway.
Peter Eisentraut <peter_e@gmx.net> writes:
> On 12/12/14 10:16 AM, Tom Lane wrote:
>> I think pg_upgrade should continue to have SQL scripts that create and
>> delete the SQL function definitions for these.
> That won't actually work very easily. LANGUAGE internal functions need
> to be in fmgr_builtins, and the only way to get them there is by listing
> them in pg_proc.h. (We could drop the functions in initdb, but seems
> kind of silly.)
Oh, good point.
> The functions do already check themselves that they are called in binary
> upgrade mode, so exposing them in pg_proc doesn't seem risky.
Fair enough ... binary upgrade mode is not readily accessible, right?
regards, tom lane
On 12/12/14 11:45 AM, Andres Freund wrote: > On 2014-12-12 11:42:57 -0500, Robert Haas wrote: >>> I think the amount of effort a simple renamed directory which wholly >>> contains a binary creates is acceptable. Just use patch -p4 instead of >>> patch -p1... >> >> That is fine if you are manually applying a patch that touches only >> that directory, but if the patch also touches other stuff then it's >> not as simple. > > I think backpatchable commits that touch individual binaries and other > code at the same time are (and ought to be!) pretty rare. > >> And I don't know how well git cherry-pick will follow >> the moves. > > Not well if the patch is done in master first. It does work. I have tried it with the changes I'm working on. There might very well be limits to the cleverness of these tools, but if you know of a fundamental reason why this would be a problem, let's please hear it.
On 12/12/14 11:14 AM, Robert Haas wrote: > contrib is a nice middle-ground. I respect that view. But if we consider contrib a place where things can try to mature, then there must be a way to move things out of there once they have matured. If it turns out that moving things out of contrib causes major problems, then we must reconsider that system and find a different process for maturing things (e.g., a makefile flag, just a random idea). Therefore, I think it's worth trying this. We can do it piece by piece.
On Fri, Dec 12, 2014 at 08:57:55PM -0500, Tom Lane wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > On 12/12/14 10:16 AM, Tom Lane wrote:
> >> I think pg_upgrade should continue to have SQL scripts that create and
> >> delete the SQL function definitions for these.
>
> > That won't actually work very easily. LANGUAGE internal functions need
> > to be in fmgr_builtins, and the only way to get them there is by listing
> > them in pg_proc.h. (We could drop the functions in initdb, but seems
> > kind of silly.)
>
> Oh, good point.
>
> > The functions do already check themselves that they are called in binary
> > upgrade mode, so exposing them in pg_proc doesn't seem risky.
>
> Fair enough ... binary upgrade mode is not readily accessible, right?
Well, the postmaster allows anyone to use the flag, while the backends
have:
case 'b': /* Undocumented flag used for binary upgrades */ if (secure)
IsBinaryUpgrade = true; break;
which means it can only be passed in from the postmaster. I think only
the super-user can set postmaster options.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +
Re: Alvaro Herrera 2014-12-12 <20141212203700.GB1768@alvh.no-ip.org> > > Pardon me for not knowing much about Debian packages, but how would > > that work exactly? Is it possible to do make install-client, then > > package the installed files, then rm -rf the install tree, then > > repeat for install-server and install-contrib? In the RPM world > > this would never work because the build/install step happens in > > toto before the packaging step. > > Uh, couldn't you just run "make install-client DESTDIR=.../client" for > client-only files, and so on? You would end up with separate > directories containing files for each subpackage. Exactly. You don't need to use DESTDIR=debian/tmp, you can "make install-client DESTDIR=$(CURDIR)/debian/postgresql-client-9.4" and skip the intermediate location for some of the files. > > Even without that, it seems like it'd be hard to make it entirely > > automatic since some files would be installed in multiple cases (and > > directories even more so). > > Yeah, you would need to fix that somehow. Directories shipped in multiple packages are not a problem for dpkg (/usr/bin!). Files must not be installed twice, but if that happened, I'd deem that a bug in the Makefile. The src/contrib/doc (and config) directories already have separate install targets. These do exactly what we need for server/contrib/doc installation, so I'll likely move to using these to remove some complexity from debian/*.install. On top of that, a separate "install-client" or "make -C client install" target would be very nice, as currently not only the client binaries have to be picked from the server tree, but also the manpages and locale files. The same would be nice for end-users who just want a client install when building from source. At the moment they need to install the full server. Christoph -- cb@df7cb.de | http://www.df7cb.de/
Here is a patch series to move pg_archivecleanup pg_standby pg_test_fsync pg_test_timing pg_upgrade pg_xlogdump pgbench from contrib/ to src/bin/. Move people didn't like moving oid2name and vacuumlo, which I understand. So I'll leave those for now. I have also included a patch to move pg_upgrade_support into the backend, as discussed. Open issues/discussion points: - no Windows build system support yet - I didn't move the source of the man pages to .../sgml/ref. This could be done. - I didn't rename to the SGML ids of the man pages to git the patter of the other applications, because that would change the name of the HTML pages. - We have traditionally kept an individual author acknowledgement in the man pages for contrib items. Should those be removed now?
Attachment
- 0001-Sort-SUBDIRS-variable-in-src-bin-Makefile.patch
- 0002-Move-pg_archivecleanup-from-contrib-to-src-bin.patch
- 0003-Move-pg_standby-from-contrib-to-src-bin.patch
- 0004-Move-pg_xlogdump-from-contrib-to-src-bin.patch
- 0005-Move-pgbench-from-contrib-to-src-bin.patch
- 0006-Move-pg_test_fsync-from-contrib-to-src-bin.patch
- 0007-Move-pg_test_timing-from-contrib-to-src-bin.patch
- 0008-Integrate-pg_upgrade_support-module-into-backend.patch
- 0009-Move-pg_upgrade-from-contrib-to-src-bin.patch
On Mon, Dec 15, 2014 at 10:59 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > - no Windows build system support yet Do you need some help here? I am getting worried about potential breakage with the version information built with MSVC and MinGW.. > - We have traditionally kept an individual author acknowledgement in the > man pages for contrib items. Should those be removed now? +1. -- Michael
On 12/14/14 9:08 PM, Michael Paquier wrote: >> - no Windows build system support yet > Do you need some help here? Sure. > I am getting worried about potential > breakage with the version information built with MSVC and MinGW.. I don't know what that is.
On Mon, Dec 15, 2014 at 11:45 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 12/14/14 9:08 PM, Michael Paquier wrote: >>> - no Windows build system support yet >> Do you need some help here? > > Sure. > >> I am getting worried about potential >> breakage with the version information built with MSVC and MinGW.. > > I don't know what that is. File version information for all the binaries and libraries produced by compilation, per se for example ee9569e. -- Michael
On Mon, Dec 15, 2014 at 11:53 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Dec 15, 2014 at 11:45 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On 12/14/14 9:08 PM, Michael Paquier wrote: >>>> - no Windows build system support yet >>> Do you need some help here? >> Sure. Peter, Attached is a patch that can be applied on your existing set to complete support on Windows for the move to src/bin. For MinGW there was nothing to do, but on MSVC we have to declare frontend utilities in the build scripts, which is roughly what this patch does. There are as well some updates needed for clean.bat and the regression test script. I did the following checks btw: - Checked the builds worked correctly - Checked that file version information was generated when building with either MinGW or MSVC - Checked that clean.bat ran correctly - Checked that vcregress upgradecheck was working correctly - Checked as well the other regression tests, but that's minor here... I also noticed when looking at your patches that we actually forgot to ignore the *.bat scripts generated by regressions of pg_upgrade when running vcregress upgradecheck with MSVC stuff, but that's a different issue that I reported on a new thread. It is included in my patch btw for completeness. Regards, -- Michael
Attachment
I know this is how it currently works, but it looks way too messy to me:
+ my $pgarchivecleanup = AddSimpleFrontend('pg_archivecleanup');
+ my $pgstandby = AddSimpleFrontend('pg_standby');
+ my $pgtestfsync = AddSimpleFrontend('pg_test_fsync');
+ my $pgtesttiming = AddSimpleFrontend('pg_test_timing');
+ my $pgbench = AddSimpleFrontend('pgbench', 1);
ISTM we should be something like
for each $elem in src/bin/Makefile:$(SUBDIRS)AddSimpleFrontend($elem)
and avoid having to list the modules one by one.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Dec 18, 2014 at 4:02 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>
> I know this is how it currently works, but it looks way too messy to me:
>
> + my $pgarchivecleanup = AddSimpleFrontend('pg_archivecleanup');
> + my $pgstandby = AddSimpleFrontend('pg_standby');
> + my $pgtestfsync = AddSimpleFrontend('pg_test_fsync');
> + my $pgtesttiming = AddSimpleFrontend('pg_test_timing');
> + my $pgbench = AddSimpleFrontend('pgbench', 1);
>
> ISTM we should be something like
>
> for each $elem in src/bin/Makefile:$(SUBDIRS)
> AddSimpleFrontend($elem)
>
> and avoid having to list the modules one by one.
If we take this road, I'd like to avoid a huge if/elseif scanning the
names of the submodules to do the necessary adjustments (Some need
FRONTEND defined, others ws2_32, etc.). Also, there is the case of
pg_basebackup where multiple binaries are included with pg_basebackup,
pg_recvlogical and pg_receivexlog. So I think that we'd need something
similar to what contrib does, aka:
my @frontend_excludes = ('pg_basebackup', 'pg_dump', 'pg_dumpall',
'pg_xlogdump', 'initdb' ...);
my frontend_extralibs = ('pgbench' => 'ws2_32.lib');
my @frontend_uselibpq = ('pgbench', 'pg_ctl', 'pg_upgrade');
And for each frontend name excluded we have an individual project
declaration with its own exceptions. With this way of doing when a new
frontend is added by default in src/bin it will be automatically
compiled. How does that sound?
--
Michael
On Thu, Dec 18, 2014 at 10:37 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Dec 18, 2014 at 4:02 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>>
>> I know this is how it currently works, but it looks way too messy to me:
>>
>> + my $pgarchivecleanup = AddSimpleFrontend('pg_archivecleanup');
>> + my $pgstandby = AddSimpleFrontend('pg_standby');
>> + my $pgtestfsync = AddSimpleFrontend('pg_test_fsync');
>> + my $pgtesttiming = AddSimpleFrontend('pg_test_timing');
>> + my $pgbench = AddSimpleFrontend('pgbench', 1);
>>
>> ISTM we should be something like
>>
>> for each $elem in src/bin/Makefile:$(SUBDIRS)
>> AddSimpleFrontend($elem)
>>
>> and avoid having to list the modules one by one.
>
> If we take this road, I'd like to avoid a huge if/elseif scanning the
> names of the submodules to do the necessary adjustments (Some need
> FRONTEND defined, others ws2_32, etc.). Also, there is the case of
> pg_basebackup where multiple binaries are included with pg_basebackup,
> pg_recvlogical and pg_receivexlog. So I think that we'd need something
> similar to what contrib does, aka:
> my @frontend_excludes = ('pg_basebackup', 'pg_dump', 'pg_dumpall',
> 'pg_xlogdump', 'initdb' ...);
> my frontend_extralibs = ('pgbench' => 'ws2_32.lib');
> my @frontend_uselibpq = ('pgbench', 'pg_ctl', 'pg_upgrade');
> And for each frontend name excluded we have an individual project
> declaration with its own exceptions. With this way of doing when a new
> frontend is added by default in src/bin it will be automatically
> compiled. How does that sound?
And here is an updated patch following those lines. Similarly to the
things in contrib/, a set of variables are used to define for each
module what are the extra libraries, include dirs, etc. This refactors
quite a bit of code, even if there are a couple of exceptions like
pg_xlogdump/ or pg_basebackup/.
--
Michael
Attachment
Michael Paquier wrote: > And here is an updated patch following those lines. Similarly to the > things in contrib/, a set of variables are used to define for each > module what are the extra libraries, include dirs, etc. This refactors > quite a bit of code, even if there are a couple of exceptions like > pg_xlogdump/ or pg_basebackup/. In a broad look, this looks a lot better. I think we should press forward with this whole set of patches and see what the buildfarm thinks. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Dec 22, 2014 at 11:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Michael Paquier wrote: > >> And here is an updated patch following those lines. Similarly to the >> things in contrib/, a set of variables are used to define for each >> module what are the extra libraries, include dirs, etc. This refactors >> quite a bit of code, even if there are a couple of exceptions like >> pg_xlogdump/ or pg_basebackup/. > > In a broad look, this looks a lot better. I think we should press > forward with this whole set of patches and see what the buildfarm > thinks. Here is a new series of patches for all those things, with the following additions: - Some cleanup for MSVC scripts compared to last patch - Moved documentation to ref/ - Removed mention to the authors of the utilities moved (?) This set of patches is basically made of the former set, with 2 additional patches for MSVC stuff and documentation pages moved to ref/. Regards, -- Michael
Attachment
On Tue, Dec 23, 2014 at 3:24 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Dec 22, 2014 at 11:30 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Michael Paquier wrote: >> >>> And here is an updated patch following those lines. Similarly to the >>> things in contrib/, a set of variables are used to define for each >>> module what are the extra libraries, include dirs, etc. This refactors >>> quite a bit of code, even if there are a couple of exceptions like >>> pg_xlogdump/ or pg_basebackup/. >> >> In a broad look, this looks a lot better. I think we should press >> forward with this whole set of patches and see what the buildfarm >> thinks. > Here is a new series of patches for all those things, with the > following additions: > - Some cleanup for MSVC scripts compared to last patch > - Moved documentation to ref/ > - Removed mention to the authors of the utilities moved (?) > This set of patches is basically made of the former set, with 2 > additional patches for MSVC stuff and documentation pages moved to > ref/. Where are we on this? This patch is waiting for input from author for the last couple of weeks. -- Michael
Hi, FWIW, I find it rather annoying if people attach patchsets as tarballs. That makes it impossible to look at them in the mailreader since I really don't have anything reasonable to go on to teach it to treat it as a set of patches. I'd also like to see patches that primarily move code around as git diff -M -C style diffs (can also be passed to format-patch). That will show the file move and then additionally the changes that have been made in addition to the rename. There's no sane way the current diffs can be reviewed without applying them to a tree. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2015-01-17 13:16:18 +0100, Andres Freund wrote: > I'd also like to see patches that primarily move code around as git diff > -M -C style diffs (can also be passed to format-patch). That will show > the file move and then additionally the changes that have been made in > addition to the rename. There's no sane way the current diffs can be > reviewed without applying them to a tree. Btw, this also avoids many trivial kind of conflicts... Which renames generally are very prone to. Rebased (fair amount of trivial conflicts due to the copyright year bump) and attached as -MC style format-patch. If you look at the content of the patches you can see that the diff makes more sense now. Observations: 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs? 2) I removed a stray 'port.c' and some whitespace as a fixup commit (git rebase --autosquash ...) from the pg_upgrade_support commit 3) pg_upgrade tests didn't run due to a /src/ to much in the path to pg_regress 4) I have doubts that it's ok to integrate the tests in src/bin just the way they were done in contrib. 5) Doing the msvc support for all intermediate commits in a separate commit strikes me as a bad idea. Essentially that makes the split pretty pointless. 6) Similarly I'd much rather see the doc movement in the same commit as the actually moved utility. Then we can start applying this one by one on whatever we have agreement. 7) Are we sure that the authors in the affected contrib modules are ok with their authorship notice being removed? I don't think Ants, Bruce or Simon have a problem with that, but ... 8) Why did you remove Peter as the git author? I've also pushed the git tree of these changes to http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary branch move-contrib-bins-to-bin Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
- 0001-Sort-SUBDIRS-variable-in-src-bin-Makefile.patch
- 0002-Move-pg_archivecleanup-from-contrib-to-src-bin.patch
- 0003-Move-pg_standby-from-contrib-to-src-bin.patch
- 0004-Move-pg_xlogdump-from-contrib-to-src-bin.patch
- 0005-Move-pgbench-from-contrib-to-src-bin.patch
- 0006-Move-pg_test_fsync-from-contrib-to-src-bin.patch
- 0007-Move-pg_test_timing-from-contrib-to-src-bin.patch
- 0008-Integrate-pg_upgrade_support-module-into-backend.patch
- 0009-Move-pg_upgrade-from-contrib-to-src-bin.patch
- 0010-Add-MSVC-support-for-new-modules-in-src-bin.patch
- 0011-Move-documentation-of-new-src-bin-utilities-to-ref.patch
- 0012-fixup-Integrate-pg_upgrade_support-module-into-backe.patch
- 0013-fixup-Move-pg_upgrade-from-contrib-to-src-bin.patch
On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres@2ndquadrant.com> wrote: > Observations: > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs? Yeah, this seems like a bad dependency, PGXS being made for contrib modules... So corrected in the patch attached (the headers of the Makefiles are improved as well to be consistent with the other utilities, btw there is code duplication in each Makefile if we do not use PGXS stuff in src/bin). > 4) I have doubts that it's ok to integrate the tests in src/bin just the > way they were done in contrib. Are you referring to the tests of pg_upgrade? > 5) Doing the msvc support for all intermediate commits in a separate > commit strikes me as a bad idea. Essentially that makes the split > pretty pointless. > 6) Similarly I'd much rather see the doc movement in the same commit as > the actually moved utility. Then we can start applying this one by one > on whatever we have agreement. Well, sure. The split was done just to facilitate review with stuff to be applied directly on top of what Peter already did. And note that I agree as well that everything should be done in a single commit. Separating things would break build on a platform or another if a build is done based on an intermediate state of this work, that would not be nice. > 7) Are we sure that the authors in the affected contrib modules are ok > with their authorship notice being removed? I don't think Ants, Bruce > or Simon have a problem with that, but ... Yeah, agreed that I have really too aggressive with what I did. Let's CC all the authors on this thread and get directly their permission to process then. Some people may accept, other no, so let's see. > 8) Why did you remove Peter as the git author? I applied on my local repo this series of patches after some bash-ing without preserving any meta data, so the author name has just been changed on the way, and then ran a simple git format to generate the whole set once again. Well, sorry if this was confusing, but let's be clear anyway: I have no intention to make mine the work of Peter (or any other people). > I've also pushed the git tree of these changes to > http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary > branch move-contrib-bins-to-bin That's helpful. I just picked it up and built the patch attached that can be applied on top of it, correcting the Makefiles and the reference to the authors in the docs. FWIW, my branch, based on yours is here: https://github.com/michaelpq/postgres/tree/contrib_to_bin Regards, -- Michael
Attachment
Simon, Bruce, Ants, (CCing..) On Sun, Jan 18, 2015 at 9:05 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> 7) Are we sure that the authors in the affected contrib modules are ok >> with their authorship notice being removed? I don't think Ants, Bruce >> or Simon have a problem with that, but ... > Yeah, agreed that I have really too aggressive with what I did. Let's > CC all the authors on this thread and get directly their permission to > process then. Some people may accept, other no, so let's see. Just to give some background if you didn't follow closely this thread: we are discussing about moving a couple of binary utilities from contrib/ to src/bin/, utilities whose author is one of you. To make documentation consistent with all the other bin/ utilities we are thinking about removing the mention to the authors. Would you agree with that? Here are the utilities impacted: - pg_archivecleanup (Simon) - pg_standby (Simon) - pg_test_fsync (Bruce) - pg_test_timing (Ants) Regards, -- Michael
Correct me if I'm wrong, but is it not the case that: 1) pg_standby was intended to be customizable code, even if usable as distributed, and 2) in any event it is essentially deprecated in favour of standby_mode and therefore moving it to src/bin seems like a wrong move on two counts? -- Andrew (irc:RhodiumToad)
On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Correct me if I'm wrong, but is it not the case that: > 1) pg_standby was intended to be customizable code, even if usable as > distributed, and I am not sure about that. > 2) in any event it is essentially deprecated in favour of standby_mode > and therefore moving it to src/bin seems like a wrong move on two counts? There were some discussions about that a couple of months ago, and the conclusion was to not remove it. Please see here for more information: http://www.postgresql.org/message-id/flat/545946E9.8060504@gmx.net#545946E9.8060504@gmx.net Regards, -- Michael
Hi, On 2015-01-18 12:56:59 +0000, Andrew Gierth wrote: > Correct me if I'm wrong, but is it not the case that: > 1) pg_standby was intended to be customizable code, even if usable as > distributed, and I don't think that really happened in reality though... > 2) in any event it is essentially deprecated in favour of standby_mode Right. > and therefore moving it to src/bin seems like a wrong move on two counts? I personally agree that that pg_standby shouldn't be moved. Even if we could not find agreement to remove it, there's little reason to move it imo. My reading of the previous discussion was that we're far from alone with that position. That's why I'd like to have the patchseries in a way that individual moves can be applied separately. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2015-01-18 22:02:13 +0900, Michael Paquier wrote: > On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth > > 2) in any event it is essentially deprecated in favour of standby_mode > > and therefore moving it to src/bin seems like a wrong move on two counts? > There were some discussions about that a couple of months ago, and the > conclusion was to not remove it. Please see here for more information: > http://www.postgresql.org/message-id/flat/545946E9.8060504@gmx.net#545946E9.8060504@gmx.net Sure, but not removing doesn't imply moving it. The other tools don't have a replacement and are more or less actively maintained. I don't think that's really the case for pg_standby. We do *not* want to give it more credence by declaring it to be part of core. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2015-01-18 21:05:29 +0900, Michael Paquier wrote: > On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres@2ndquadrant.com> wrote: > > Observations: > > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs? > Yeah, this seems like a bad dependency, PGXS being made for contrib > modules... So corrected in the patch attached (the headers of the > Makefiles are improved as well to be consistent with the other > utilities, btw there is code duplication in each Makefile if we do not > use PGXS stuff in src/bin). Yes, there's a fair amount of duplication. I do think there's a good case for making the Makefiles less redundant, but we should do that in all src/bin binaries, and in a separate patch. > > 4) I have doubts that it's ok to integrate the tests in src/bin just the > > way they were done in contrib. > Are you referring to the tests of pg_upgrade? Yes. > > 5) Doing the msvc support for all intermediate commits in a separate > > commit strikes me as a bad idea. Essentially that makes the split > > pretty pointless. > > 6) Similarly I'd much rather see the doc movement in the same commit as > > the actually moved utility. Then we can start applying this one by one > > on whatever we have agreement. > Well, sure. The split was done just to facilitate review with stuff to > be applied directly on top of what Peter already did. And note that I > agree as well that everything should be done in a single commit. I don't think all of the moves should be done in one commit. I'd much rather do e.g. all the pg_upgrade stuff in one, and the rest in another. > Separating things would break build on a platform or another if a > build is done based on an intermediate state of this work, that would > not be nice. Yes, that's why commits in a series should be standalone. I.e. all should work on all platforms. Possibly individual ones don't add features, but they shouldn't break things. > > 8) Why did you remove Peter as the git author? > I applied on my local repo this series of patches after some bash-ing > without preserving any meta data, so the author name has just been > changed on the way, and then ran a simple git format to generate the > whole set once again. Well, sorry if this was confusing, but let's be > clear anyway: I have no intention to make mine the work of Peter (or > any other people). You know that you can apply patch series like Peter's (or from your tar archive) using 'git am'? Which preserves the author, message and such? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund wrote: > Hi, > > On 2015-01-18 12:56:59 +0000, Andrew Gierth wrote: > > and therefore moving it to src/bin seems like a wrong move on two counts? > > I personally agree that that pg_standby shouldn't be moved. Even if we > could not find agreement to remove it, there's little reason to move it > imo. My reading of the previous discussion was that we're far from alone > with that position. There was clear agreement on a set of things tomove, and as I recall pg_standby was not in it. Wasn't this patch series updated to comply with what was agreed? > That's why I'd like to have the patchseries in a way that individual > moves can be applied separately. Yeah. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 1/17/15 8:08 AM, Andres Freund wrote: > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs? I am. Why not?
On 1/18/15 8:16 AM, Andres Freund wrote: > On 2015-01-18 22:02:13 +0900, Michael Paquier wrote: >> On Sun, Jan 18, 2015 at 9:56 PM, Andrew Gierth >>> 2) in any event it is essentially deprecated in favour of standby_mode >>> and therefore moving it to src/bin seems like a wrong move on two counts? >> There were some discussions about that a couple of months ago, and the >> conclusion was to not remove it. Please see here for more information: >> http://www.postgresql.org/message-id/flat/545946E9.8060504@gmx.net#545946E9.8060504@gmx.net > > Sure, but not removing doesn't imply moving it. The other tools don't > have a replacement and are more or less actively maintained. I don't > think that's really the case for pg_standby. We do *not* want to give > it more credence by declaring it to be part of core. I don't really care much, but the discussion appeared to indicate that pg_standby currently does not have a full replacement.
On Sun, Jan 18, 2015 at 10:21 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2015-01-18 21:05:29 +0900, Michael Paquier wrote: >> On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres@2ndquadrant.com> wrote: >> > Observations: >> > 1) Are we sure it's a good idea to rely on pgxs.mk in src/bin programs? > >> Yeah, this seems like a bad dependency, PGXS being made for contrib >> modules... So corrected in the patch attached (the headers of the >> Makefiles are improved as well to be consistent with the other >> utilities, btw there is code duplication in each Makefile if we do not >> use PGXS stuff in src/bin). > > Yes, there's a fair amount of duplication. I do think there's a good > case for making the Makefiles less redundant, but we should do that in > all src/bin binaries, and in a separate patch. Agreed on that. pg_ctl is a good candidate for improvement as well. >> > 4) I have doubts that it's ok to integrate the tests in src/bin just the >> > way they were done in contrib. >> Are you referring to the tests of pg_upgrade? > Yes. I am not foreseeing any problems with the way they are done now as long as they continue to use pg_regress to initialize the cluster. Perhaps you have something on top of your mind? >> > 5) Doing the msvc support for all intermediate commits in a separate >> > commit strikes me as a bad idea. Essentially that makes the split >> > pretty pointless. >> > 6) Similarly I'd much rather see the doc movement in the same commit as >> > the actually moved utility. Then we can start applying this one by one >> > on whatever we have agreement. > >> Well, sure. The split was done just to facilitate review with stuff to >> be applied directly on top of what Peter already did. And note that I >> agree as well that everything should be done in a single commit. > > I don't think all of the moves should be done in one commit. I'd much > rather do e.g. all the pg_upgrade stuff in one, and the rest in another. OK. I am fine with that. pg_upgrade move touches backend code. Now the remaining point seems to be: do we move pg_standby as well? Seeing the last emails of this thread the answer would be to let it end its life happily in contrib/. -- Michael
On Sat, Jan 17, 2015 at 01:16:18PM +0100, Andres Freund wrote: > Hi, > > FWIW, I find it rather annoying if people attach patchsets as > tarballs. That makes it impossible to look at them in the mailreader > since I really don't have anything reasonable to go on to teach it to > treat it as a set of patches. > > I'd also like to see patches that primarily move code around as git diff > -M -C style diffs (can also be passed to format-patch). That will show > the file move and then additionally the changes that have been made in > addition to the rename. There's no sane way the current diffs can be > reviewed without applying them to a tree. FYI, the .gitconfig setting is 'renames': [diff] renames = copies -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Sat, Jan 17, 2015 at 02:08:34PM +0100, Andres Freund wrote: > 7) Are we sure that the authors in the affected contrib modules are ok > with their authorship notice being removed? I don't think Ants, Bruce > or Simon have a problem with that, but ... I am fine. It means others can be blamed. ;-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Andres Freund wrote: > Rebased (fair amount of trivial conflicts due to the copyright year > bump) and attached as -MC style format-patch. If you look at the content > of the patches you can see that the diff makes more sense now. Ah --- I just realized that the change Peter is proposing from backslash to forward slashes in the MSVC build stuff is related to testability of this patch set in non-Windows environments. Can we get that patch done please? It seems there's only a small bit missing there. How do we go about getting these patches pushed? Note that each utility we move to src/bin will create a new translatable module for 9.5. It would be better to do it soon rather than waiting at the very end of the cycle, so that translators have time to work through the bunch of extra files. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 10, 2015 at 10:48 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Andres Freund wrote: > >> Rebased (fair amount of trivial conflicts due to the copyright year >> bump) and attached as -MC style format-patch. If you look at the content >> of the patches you can see that the diff makes more sense now. > > Ah --- I just realized that the change Peter is proposing from backslash > to forward slashes in the MSVC build stuff is related to testability of > this patch set in non-Windows environments. Can we get that patch done > please? It seems there's only a small bit missing there. > > How do we go about getting these patches pushed? I think that one of the last point raised (by Andres) was if the Makefiles in src/bin should depend on pgxs.mk or not. FWIW, I think as well that it should be a subsequent patch not related to this one as it is a different debate to have stuff of src/bin depend on contrib infrastructure. Now, the pushing plan was to have the stuff of pg_upgrade done in a separate commit. Note that I am fine helping out wrapping up things particularly on Windows if that helps. > Note that each utility we move to src/bin will create a new translatable > module for 9.5. It would be better to do it soon rather than waiting at > the very end of the cycle, so that translators have time to work through > the bunch of extra files. That's a point. -- Michael
Michael Paquier wrote: > On Tue, Mar 10, 2015 at 10:48 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > How do we go about getting these patches pushed? > > I think that one of the last point raised (by Andres) was if the > Makefiles in src/bin should depend on pgxs.mk or not. FWIW, I think as > well that it should be a subsequent patch not related to this one as > it is a different debate to have stuff of src/bin depend on contrib > infrastructure. I don't think we care one bit whether these modules use pgxs, at least not currently. If we find any issues later on, it should be an easy fix anyway. > Now, the pushing plan was to have the stuff of pg_upgrade done in a > separate commit. Note that I am fine helping out wrapping up things > particularly on Windows if that helps. I vote for moving one module per commmit. Probably the first commit will be the hardest one to get right, so let's pick an easy module -- say, pgbench. That way there are less gotchas hopefully. Once we have that working everywhere we look into moving another one. > > Note that each utility we move to src/bin will create a new translatable > > module for 9.5. It would be better to do it soon rather than waiting at > > the very end of the cycle, so that translators have time to work through > > the bunch of extra files. > > That's a point. I'm a translator ;-) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 11, 2015 at 10:06 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Michael Paquier wrote: >> On Tue, Mar 10, 2015 at 10:48 PM, Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: > >> > How do we go about getting these patches pushed? >> >> I think that one of the last point raised (by Andres) was if the >> Makefiles in src/bin should depend on pgxs.mk or not. FWIW, I think as >> well that it should be a subsequent patch not related to this one as >> it is a different debate to have stuff of src/bin depend on contrib >> infrastructure. > > I don't think we care one bit whether these modules use pgxs, at least > not currently. If we find any issues later on, it should be an easy fix > anyway. > >> Now, the pushing plan was to have the stuff of pg_upgrade done in a >> separate commit. Note that I am fine helping out wrapping up things >> particularly on Windows if that helps. > > I vote for moving one module per commmit. Probably the first commit > will be the hardest one to get right, so let's pick an easy module -- > say, pgbench. That way there are less gotchas hopefully. Once we have > that working everywhere we look into moving another one. Fine for me. You want a cross-OS patch? -- Michael
Michael Paquier wrote: > On Wed, Mar 11, 2015 at 10:06 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > >> Now, the pushing plan was to have the stuff of pg_upgrade done in a > >> separate commit. Note that I am fine helping out wrapping up things > >> particularly on Windows if that helps. > > > > I vote for moving one module per commmit. Probably the first commit > > will be the hardest one to get right, so let's pick an easy module -- > > say, pgbench. That way there are less gotchas hopefully. Once we have > > that working everywhere we look into moving another one. > > Fine for me. You want a cross-OS patch? Yep, I'm willing to testing and pushing such a patch. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here is a rebase of my previous patch set. I have integrated the various minor fixes from Michael and Andres. I have dropped moving pg_standby, because no one seemed to like that. I wasn't able to do anything with Michael's Mkvcbuild.pm patch, since that appeared to include a significant refactoring along with the actual move. Maybe the refactoring can be done separately first? Otherwise, I suggest we start with the first patch, pg_archivecleanup, fix up the Windows build system for it, and commit it, and repeat.
Attachment
- 0001-Move-pg_archivecleanup-from-contrib-to-src-bin.patch
- 0002-Move-pg_xlogdump-from-contrib-to-src-bin.patch
- 0003-Move-pgbench-from-contrib-to-src-bin.patch
- 0004-Move-pg_test_fsync-from-contrib-to-src-bin.patch
- 0005-Move-pg_test_timing-from-contrib-to-src-bin.patch
- 0006-Integrate-pg_upgrade_support-module-into-backend.patch
- 0007-Move-pg_upgrade-from-contrib-to-src-bin.patch
On Wed, Mar 11, 2015 at 12:00 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > Here is a rebase of my previous patch set. I have integrated the > various minor fixes from Michael and Andres. I have dropped moving > pg_standby, because no one seemed to like that. > > I wasn't able to do anything with Michael's Mkvcbuild.pm patch, since > that appeared to include a significant refactoring along with the actual > move. Maybe the refactoring can be done separately first? Yes. I was just looking at the refactoring part so as it could be applied before the rest first. > Otherwise, I suggest we start with the first patch, pg_archivecleanup, > fix up the Windows build system for it, and commit it, and repeat. I'd rather vote for having the Windows-side stuff integrated with each patch. Mind if I rebase what you just sent with the Windows things added? -- Michael
On Tue, Mar 10, 2015 at 8:07 PM, Michael Paquier wrote: > I'd rather vote for having the Windows-side stuff integrated with each > patch. Mind if I rebase what you just sent with the Windows things > added? And here is the rebased series with the MSVC changes included for each module in its individual patch. 0001 is the refactoring patch for MSVC scripts which should be applied before the rest. -- Michael
Attachment
- 0001-Refactor-MSVC-scripts-for-modules-of-src-bin-similar.patch
- 0002-Move-pg_archivecleanup-from-contrib-to-src-bin.patch
- 0003-Move-pg_xlogdump-from-contrib-to-src-bin.patch
- 0004-Move-pgbench-from-contrib-to-src-bin.patch
- 0005-Move-pg_test_fsync-from-contrib-to-src-bin.patch
- 0006-Move-pg_test_timing-from-contrib-to-src-bin.patch
- 0007-Integrate-pg_upgrade_support-module-into-backend.patch
- 0008-Move-pg_upgrade-from-contrib-to-src-bin.patch
Michael Paquier wrote: > On Tue, Mar 10, 2015 at 8:07 PM, Michael Paquier wrote: > > I'd rather vote for having the Windows-side stuff integrated with each > > patch. Mind if I rebase what you just sent with the Windows things > > added? > > And here is the rebased series with the MSVC changes included for each > module in its individual patch. 0001 is the refactoring patch for MSVC > scripts which should be applied before the rest. Thanks. I have pushed 0001; let's see what the buildfarm thinks of it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote: > I don't think we care one bit whether these modules use pgxs, at least > not currently. If we find any issues later on, it should be an easy fix > anyway. I personally find it quite ugly to use pgxs for stuff in src/bin. pgxs.mk says: # This file contains generic rules to build many kinds of simple # extension modules. You only need to set a few variables and include # this file, the rest will be done here. I don't object at all to introducing more generic rules for src/bin, but that seems like a separate task. And one that should be done right not just use some convenient hack. And you can't tell me that +NO_PGXS = 1 +include $(top_srcdir)/src/makefiles/pgxs.mk isn't a hack... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote:
>> I don't think we care one bit whether these modules use pgxs, at least
>> not currently. If we find any issues later on, it should be an easy fix
>> anyway.
> I personally find it quite ugly to use pgxs for stuff in
> src/bin. pgxs.mk says:
> # This file contains generic rules to build many kinds of simple
> # extension modules. You only need to set a few variables and include
> # this file, the rest will be done here.
> I don't object at all to introducing more generic rules for src/bin, but
> that seems like a separate task. And one that should be done right not
> just use some convenient hack. And you can't tell me that
> +NO_PGXS = 1
> +include $(top_srcdir)/src/makefiles/pgxs.mk
> isn't a hack...
I'm with Andres on this. If we can't take the time to make a moved module
look like it actually belongs to src/bin, we shouldn't do it at all.
Code should look like it's always been wherever it is --- obvious evidence
of some nonlinear historical development path just confuses readers.
Less abstractly, this sort of shortcut is likely to be a problem for future
development of pgxs.mk, in that now it will have to support an abuse
it was never intended for; one that works only accidentally (if indeed
it works at all, which I have doubts about for corner cases).
regards, tom lane
Andres Freund wrote: > On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote: > > I don't think we care one bit whether these modules use pgxs, at least > > not currently. If we find any issues later on, it should be an easy fix > > anyway. > > I personally find it quite ugly to use pgxs for stuff in > src/bin. pgxs.mk says: > # This file contains generic rules to build many kinds of simple > # extension modules. You only need to set a few variables and include > # this file, the rest will be done here. I think if you s/extension// in the above paragraph, it makes complete sense to use it for the new src/bin modules. > I don't object at all to introducing more generic rules for src/bin, but > that seems like a separate task. And one that should be done right not > just use some convenient hack. And you can't tell me that > +NO_PGXS = 1 > +include $(top_srcdir)/src/makefiles/pgxs.mk > isn't a hack... Why not? It's the standard procedure for building modules outside the contrib/ source tree. I'm okay with reformulating the makefiles after the move, so that these modules are built in our standard, simpler makefile conventions. Let's do that in a followup commit -- then each change is simpler. It's hard enough with all the ugly MSVC stuff. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2015-03-11 11:19:24 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote: > > > I don't think we care one bit whether these modules use pgxs, at least > > > not currently. If we find any issues later on, it should be an easy fix > > > anyway. > > > > I personally find it quite ugly to use pgxs for stuff in > > src/bin. pgxs.mk says: > > # This file contains generic rules to build many kinds of simple > > # extension modules. You only need to set a few variables and include > > # this file, the rest will be done here. > > I think if you s/extension// in the above paragraph, it makes complete > sense to use it for the new src/bin modules. No. The dependencies here are becoming completely ridiculous: include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk -> NO_PGXS = 1 include $(top_srcdir)/src/makefiles/pgxs.mk -> ifndef PGXS ifndef NO_PGXS $(errorpgxs error: makefile variable PGXS or NO_PGXS must be set) endif endif ... ifdef PGXS # We assume that we are in src/makefiles/, so top is ... top_builddir := $(dir $(PGXS))../.. include $(top_builddir)/src/Makefile.global .... This is just an absurd maze. Not something we should spread even further. It's bad enough that contrib uses it. But the purportedly higher quality code (that's why we're moving it, right?) shoulnd't do these hacks. > > I don't object at all to introducing more generic rules for src/bin, but > > that seems like a separate task. And one that should be done right not > > just use some convenient hack. And you can't tell me that > > +NO_PGXS = 1 > > +include $(top_srcdir)/src/makefiles/pgxs.mk > > isn't a hack... > > Why not? It's the standard procedure for building modules outside the > contrib/ source tree. NO_PGXS = 1 is? Huh? You probably mean USE_PGXS? That's something different. > I'm okay with reformulating the makefiles after the move, so that these > modules are built in our standard, simpler makefile conventions. Let's > do that in a followup commit -- then each change is simpler. It's hard > enough with all the ugly MSVC stuff. -1. Greetings, Andres Freund --Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 3/11/15 10:00 AM, Andres Freund wrote: > On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote: >> I don't think we care one bit whether these modules use pgxs, at least >> not currently. If we find any issues later on, it should be an easy fix >> anyway. > > I personally find it quite ugly to use pgxs for stuff in > src/bin. pgxs.mk says: > # This file contains generic rules to build many kinds of simple > # extension modules. You only need to set a few variables and include > # this file, the rest will be done here. Let's get history straight. pgxs was not initially an external extension building framework. It was a refactoring of our own internal makefile rules, because a lot of code under contrib had the same rules copy-and-pasted. It was only much later that it was rebranded for external use. It's debatable why it wasn't expanded to also be used in src/bin/, but I attribute that to a combination of boredom, complicated special cases under src/bin/, less frequent additions under src/bin/, and said rebranding -- not because it would have been a bad idea. You effectively suggest that we are not allowed to use our own code and propose that we undo that refactoring, and then what? I'll just resubmit the same patch from 2001 to refactor it again? > I don't object at all to introducing more generic rules for src/bin, but > that seems like a separate task. And one that should be done right not > just use some convenient hack. And you can't tell me that > +NO_PGXS = 1 > +include $(top_srcdir)/src/makefiles/pgxs.mk > isn't a hack... Well, that's unfortunate, but I'd rather live with one line of hack for a while that I can easily fix later, instead of writing completely new makefiles from scratch. Who is going to want to debug those, by the way? The turnaround time for makefile changes in this project is one month per line, because we can't get anything reviewed across all platforms any faster.
On Thu, Mar 12, 2015 at 8:50 AM, Peter Eisentraut wrote: > On 3/11/15 10:00 AM, Andres Freund wrote: >> On 2015-03-10 22:06:37 -0300, Alvaro Herrera wrote: >>> I don't think we care one bit whether these modules use pgxs, at least >>> not currently. If we find any issues later on, it should be an easy fix >>> anyway. >> >> I personally find it quite ugly to use pgxs for stuff in >> src/bin. pgxs.mk says: >> # This file contains generic rules to build many kinds of simple >> # extension modules. You only need to set a few variables and include >> # this file, the rest will be done here. > > Let's get history straight. pgxs was not initially an external > extension building framework. It was a refactoring of our own internal > makefile rules, because a lot of code under contrib had the same rules > copy-and-pasted. It was only much later that it was rebranded for > external use. It's debatable why it wasn't expanded to also be used in > src/bin/, but I attribute that to a combination of boredom, complicated > special cases under src/bin/, less frequent additions under src/bin/, > and said rebranding -- not because it would have been a bad idea. > > You effectively suggest that we are not allowed to use our own code and > propose that we undo that refactoring, and then what? I'll just > resubmit the same patch from 2001 to refactor it again? > >> I don't object at all to introducing more generic rules for src/bin, but >> that seems like a separate task. And one that should be done right not >> just use some convenient hack. And you can't tell me that >> +NO_PGXS = 1 >> +include $(top_srcdir)/src/makefiles/pgxs.mk >> isn't a hack... > > Well, that's unfortunate, but I'd rather live with one line of hack for > a while that I can easily fix later, instead of writing completely new > makefiles from scratch. Who is going to want to debug those, by the > way? The turnaround time for makefile changes in this project is one > month per line, because we can't get anything reviewed across all > platforms any faster. Well, TBH, I am on Andres' and Tom's side for this stuff. It feels that using a PGXS rule like that is a trap for circular dependencies and I am sure we do not want that. I think as well that we should involve all the other utilities in src/bin if we do such a refactoring, so that's definitely a next step, not this one obviously. Attached is a series of patch rebased on current HEAD, there were some conflicts after perl-tidying the refactoring patch for MSVC. Note that this series still uses PGXS in the Makefiles, I am fine to update them if necessary once this matter is set (already did this stuff upthread with a previous version). -- Michael
Attachment
- 0001-Move-pg_archivecleanup-from-contrib-to-src-bin.patch
- 0002-Move-pg_xlogdump-from-contrib-to-src-bin.patch
- 0003-Move-pgbench-from-contrib-to-src-bin.patch
- 0004-Move-pg_test_fsync-from-contrib-to-src-bin.patch
- 0005-Move-pg_test_timing-from-contrib-to-src-bin.patch
- 0006-Integrate-pg_upgrade_support-module-into-backend.patch
- 0007-Move-pg_upgrade-from-contrib-to-src-bin.patch
On 3/11/15 8:21 PM, Michael Paquier wrote: > Attached is a series of patch rebased on current HEAD, there were some > conflicts after perl-tidying the refactoring patch for MSVC. Note that > this series still uses PGXS in the Makefiles, I am fine to update them > if necessary once this matter is set (already did this stuff upthread > with a previous version). OK, here we go. I have committed the pg_archivecleanup move, with a complete makefile and your Windows help. Let's see how it builds.
Peter Eisentraut wrote: > On 3/11/15 8:21 PM, Michael Paquier wrote: > > Attached is a series of patch rebased on current HEAD, there were some > > conflicts after perl-tidying the refactoring patch for MSVC. Note that > > this series still uses PGXS in the Makefiles, I am fine to update them > > if necessary once this matter is set (already did this stuff upthread > > with a previous version). > > OK, here we go. I have committed the pg_archivecleanup move, with a > complete makefile and your Windows help. Let's see how it builds. Hmm, should it appear automatically in babel.pg.org? I don't see it there yet. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 4/12/15 8:00 PM, Alvaro Herrera wrote: > Peter Eisentraut wrote: >> On 3/11/15 8:21 PM, Michael Paquier wrote: >>> Attached is a series of patch rebased on current HEAD, there were some >>> conflicts after perl-tidying the refactoring patch for MSVC. Note that >>> this series still uses PGXS in the Makefiles, I am fine to update them >>> if necessary once this matter is set (already did this stuff upthread >>> with a previous version). >> >> OK, here we go. I have committed the pg_archivecleanup move, with a >> complete makefile and your Windows help. Let's see how it builds. > > Hmm, should it appear automatically in babel.pg.org? I don't see it > there yet. It needs an nls.mk file first. Feel free to add them.
On Sun, Apr 12, 2015 at 12:36 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 3/11/15 8:21 PM, Michael Paquier wrote: >> Attached is a series of patch rebased on current HEAD, there were some >> conflicts after perl-tidying the refactoring patch for MSVC. Note that >> this series still uses PGXS in the Makefiles, I am fine to update them >> if necessary once this matter is set (already did this stuff upthread >> with a previous version). > > OK, here we go. I have committed the pg_archivecleanup move, with a > complete makefile and your Windows help. Let's see how it builds. All the patches have been committed, finishing the work on this thread. -- Michael
Peter, Michael, On 2015-04-22 16:13:15 +0900, Michael Paquier wrote: > All the patches have been committed, finishing the work on this thread. Many thanks for that effort! Andres
On Wed, Apr 22, 2015 at 09:18:40AM +0200, Andres Freund wrote: > Peter, Michael, > > On 2015-04-22 16:13:15 +0900, Michael Paquier wrote: > > All the patches have been committed, finishing the work on this thread. > > Many thanks for that effort! And pg_upgrade thanks you. ;-) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +