Re: 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(at)gmx(dot)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(at)2ndquadrant(dot)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(at)gmx(dot)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(at)momjian(dot)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/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/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: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(at)momjian(dot)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(at)momjian(dot)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(at)vmware(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)vmware(dot)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(at)2ndquadrant(dot)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(at)gmx(dot)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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)gmx(dot)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(at)momjian(dot)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(at)2ndquadrant(dot)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(dot)GO31413(at)awork2(dot)anarazel(dot)de>
> On 2014-12-12 10:20:58 -0500, Tom Lane wrote:
> > Peter Eisentraut <peter_e(at)gmx(dot)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.install
65 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.install
199 total

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(at)df7cb(dot)de | http://www.df7cb.de/

Christoph Berg <cb(at)df7cb(dot)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(at)df7cb(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com> wrote:
> Tom Lane wrote:
>> Heikki Linnakangas <hlinnakangas(at)vmware(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Re: Alvaro Herrera 2014-12-12 <20141212203700(dot)GB1768(at)alvh(dot)no-ip(dot)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(at)df7cb(dot)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?

On Mon, Dec 15, 2014 at 10:59 AM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)gmx(dot)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(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Dec 15, 2014 at 11:45 AM, Peter Eisentraut <peter_e(at)gmx(dot)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

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(at)2ndquadrant(dot)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(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Dec 18, 2014 at 4:02 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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

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(at)2ndquadrant(dot)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

On Tue, Dec 23, 2014 at 3:24 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Dec 22, 2014 at 11:30 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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

On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres(at)2ndquadrant(dot)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

Simon, Bruce, Ants,
(CCing..)

On Sun, Jan 18, 2015 at 9:05 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Jan 17, 2015 at 10:08 PM, Andres Freund <andres(at)2ndquadrant(dot)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(at)tao11(dot)riddles(dot)org(dot)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(dot)8060504(at)gmx(dot)net#545946E9(dot)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(dot)8060504(at)gmx(dot)net#545946E9(dot)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(at)2ndquadrant(dot)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(dot)8060504(at)gmx(dot)net#545946E9(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)momjian(dot)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(at)momjian(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com> wrote:
> Michael Paquier wrote:
>> On Tue, Mar 10, 2015 at 10:48 PM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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.

On Wed, Mar 11, 2015 at 12:00 PM, Peter Eisentraut <peter_e(at)gmx(dot)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

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(at)2ndquadrant(dot)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
$(error pgxs 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

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(at)gmx(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +