Обсуждение: pg_xlogdump

Here's an updated version of pg_xlogdump.  This is rebased on top of the
committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
stuff, and is basically a revamped version of what Andres submitted in
http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-andres@2ndquadrant.com

I also attach a patch to move the relpathbackend() function to
src/common.  On top of that, it's trivial to change pg_xlogdump and
remove the "uninplemented" stub we're currently using.  (I also tried it
with a real implementation of relpath() that was mostly a duplicate of
the backend's relpath(), but I didn't like the duplication at all even
though I stripped the unnecessary bits).

(The more adventorous might think about moving timestamp_to_str to
src/common, as well, but this isn't a simple task: it depends on some
backend global state variables such as GUC vars, so it requires detailed
surgery.  I think it's a worthy goal nonetheless, because it'd allow us
to reduce useless duplication in ECPG, but it's not a 9.3 project)

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> Here's an updated version of pg_xlogdump.  This is rebased on top of the
> committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> stuff, and is basically a revamped version of what Andres submitted in
> http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-andres@2ndquadrant.com

Two tiny followup bits, I had fixed since:
* one copy-and-paste-o in an error message
* replace stupid directory verification implementation
* fix include in compat.c to include utils/timestamp.h instead of
  datatype/

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Just a heads up: I intend to push this shortly, and after it has seen
some BF activity, pg_xlogdump as well.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund wrote:
> On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> > Here's an updated version of pg_xlogdump.  This is rebased on top of the
> > committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> > stuff, and is basically a revamped version of what Andres submitted in
> > http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-andres@2ndquadrant.com
>
> Two tiny followup bits, I had fixed since:
> * one copy-and-paste-o in an error message
> * replace stupid directory verification implementation
> * fix include in compat.c to include utils/timestamp.h instead of
>   datatype/

Applied with some additional fixes.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2013-02-22 16:58:37 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2013-02-13 12:09:37 -0300, Alvaro Herrera wrote:
> > > Here's an updated version of pg_xlogdump.  This is rebased on top of the
> > > committed xlogreader, palloc restructuring and libpgcommon, PG_RMGR
> > > stuff, and is basically a revamped version of what Andres submitted in
> > > http://www.postgresql.org/message-id/1357672187-7693-5-git-send-email-andres@2ndquadrant.com
> > 
> > Two tiny followup bits, I had fixed since:
> > * one copy-and-paste-o in an error message
> > * replace stupid directory verification implementation
> > * fix include in compat.c to include utils/timestamp.h instead of
> >   datatype/
> 
> Applied with some additional fixes.

Thanks!

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 2013-02-23 14:54:51 -0800, Jeff Janes wrote:
> If I run "make clean", or "make maintainer-clean", this deletes the file
> contrib/pg_xlogdump/rmgrdesc.c.   And then config/make doesn't know how to
> get it back again.
>
> I don't know if the Makefile needs to be taught not to delete it, or taught
> how to recreate it once deleted.

It shouldn't be deleted, I think neither Alvaro nor me did notice this
because it only matters in non-vpath builds...

Patch attached.

I independently wonder whether we should remove the PGXS stub from
xlogdump, given it relies on a full sourcetree available?

Andres

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-02-23 14:54:51 -0800, Jeff Janes wrote:
>> I don't know if the Makefile needs to be taught not to delete it, or taught
>> how to recreate it once deleted.

> It shouldn't be deleted,

I came to the same conclusion and committed that a few minutes before
you posted.

> I independently wonder whether we should remove the PGXS stub from
> xlogdump, given it relies on a full sourcetree available?

I'd just as soon keep its Makefile looking like all the others.
        regards, tom lane

On Sun, 2013-02-24 at 09:34 -0500, Tom Lane wrote:
> > I independently wonder whether we should remove the PGXS stub from
> > xlogdump, given it relies on a full sourcetree available?
> 
> I'd just as soon keep its Makefile looking like all the others.
> 
If the code doesn't actually work, it should be deleted.  There is no
value in keeping the Makefile looking like the others, because it
doesn't in fact look like the others.

I for one wonder why we even have PGXS support in contrib at all.  It's
not documented or tested anywhere, so it might as well not exist.

Peter Eisentraut <peter_e@gmx.net> writes:
> I for one wonder why we even have PGXS support in contrib at all.  It's
> not documented or tested anywhere, so it might as well not exist.

I think I did about the same comment back when cooking the extension
patch, and the answer then was all about providing PGXS usage examples.
Now if none of the buildfarm animals are actually building our contribs
out of tree, maybe we should just remove those examples.

The cost of keeping them is that they double-up the Makefile content and
lots of users do think they need their extension's Makefile to be
structured the same. The common effect before the extension availability
was for people to provide extensions that would only build in tree.

I don't want to kill cleaning up those Makefiles, but I still want to
make a strong correlation in between that point and providing core
maintained extensions. I don't think extensions should have support for
being built in-tree at all.

My proposal: paint them extension rather than contrib modules, then
cleanup Makefiles so as to stop building them in-tree.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Peter Eisentraut <peter_e@gmx.net> writes:
>> I for one wonder why we even have PGXS support in contrib at all.  It's
>> not documented or tested anywhere, so it might as well not exist.

> I think I did about the same comment back when cooking the extension
> patch, and the answer then was all about providing PGXS usage examples.
> Now if none of the buildfarm animals are actually building our contribs
> out of tree, maybe we should just remove those examples.

> The cost of keeping them is that they double-up the Makefile content and
> lots of users do think they need their extension's Makefile to be
> structured the same. The common effect before the extension availability
> was for people to provide extensions that would only build in tree.

> I don't want to kill cleaning up those Makefiles, but I still want to
> make a strong correlation in between that point and providing core
> maintained extensions. I don't think extensions should have support for
> being built in-tree at all.

> My proposal: paint them extension rather than contrib modules, then
> cleanup Makefiles so as to stop building them in-tree.

[ Sigh... ]  Why this eagerness to fix what isn't broken?

Leave the Makefiles alone.  They're not broken and they provide useful
examples, plus a sense of continuity between in-tree and not-in-tree
extensions.  Any change here will likely break build scenarios that
work today --- in particular, this proposal will break building contrib
before the main tree has been installed.

If somebody wants to set up a buildfarm member that occasionally tests
PGXS building of contrib/, that's fine with me.  But it isn't, and never
will be, the main build scenario for contrib/ IMO.
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> work today --- in particular, this proposal will break building contrib
> before the main tree has been installed.

True that.

> If somebody wants to set up a buildfarm member that occasionally tests
> PGXS building of contrib/, that's fine with me.  But it isn't, and never
> will be, the main build scenario for contrib/ IMO.

I guess you just made a final vote on the whole idea of making contrib a
set of extensions maintained by the PostgreSQL commiters, then. It might
be good to document that position, so that the topic isn't raised again
each year?

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On 2013-02-26 17:23:01 +0100, Dimitri Fontaine wrote:
> Peter Eisentraut <peter_e@gmx.net> writes:
> > I for one wonder why we even have PGXS support in contrib at all.  It's
> > not documented or tested anywhere, so it might as well not exist.

Agreed. I personally think we just should build contrib/ as part of the
normal build and be done with it.

There's currently the argument that building them separately is
interesting because you can do it from a separate sourcetree if you have
a precompiled postgres around. But whats the usecase for that?

I would much rather designate one example module that is always built
with PGXS (possibly only during regress's install?), so we reliably test that.

> I think I did about the same comment back when cooking the extension
> patch, and the answer then was all about providing PGXS usage examples.
> Now if none of the buildfarm animals are actually building our contribs
> out of tree, maybe we should just remove those examples.
> 
> The cost of keeping them is that they double-up the Makefile content and
> lots of users do think they need their extension's Makefile to be
> structured the same. The common effect before the extension availability
> was for people to provide extensions that would only build in tree.
> 
> I don't want to kill cleaning up those Makefiles, but I still want to
> make a strong correlation in between that point and providing core
> maintained extensions. I don't think extensions should have support for
> being built in-tree at all.

Wait what? So I need to make install before I can compile extensions?
That doesn't seem to be something realistic.

> My proposal: paint them extension rather than contrib modules, then
> cleanup Makefiles so as to stop building them in-tree.

Imo painting them extensions is something unrelated. Which doesn't apply
to everything in contrib/, there are several modules that don't make
sense as extensions (chkpass, oid2name, passwordcheck,
pg_archivecleanup, pgbench, ...).

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On 2013-02-26 11:33:48 -0500, Tom Lane wrote:
> [ Sigh... ]  Why this eagerness to fix what isn't broken?

Well, it is broken for xlogdump because it needs a sourcetree arround.

All I personally really want to do is to replace the usual stanza for
pg_xlogdump with something like:

ifdef USE_PGXS
$(error Building pg_xlogdump with PGXS is not supported)
include $(PGXS)
else
...

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

Andres Freund <andres@2ndquadrant.com> writes:
> Well, it is broken for xlogdump because it needs a sourcetree arround.

> All I personally really want to do is to replace the usual stanza for
> pg_xlogdump with something like:

> ifdef USE_PGXS
> $(error Building pg_xlogdump with PGXS is not supported)
> else
> ...

Seems reasonable.  But let's not break the cases that do work.  One
of the functions of contrib/ is to serve as models/skeletons for
external modules.  If we pull out the "useless" PGXS support then we'll
just be making it that much harder to copy a contrib module and start
doing something useful.
        regards, tom lane

Andres Freund <andres@2ndquadrant.com> writes:
> Wait what? So I need to make install before I can compile extensions?
> That doesn't seem to be something realistic.

You know, any extension that's not in our source tree out there is
maintained in a way to target all supported versions of PostgreSQL from
the same sources. I understand that we want to continue applying our
versioning rules to contribs, and I think that it's a good idea.

But now that we're making a real cut decision about that, I think
contribs should be documented as NOT extensions.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

On 2/26/13 11:45 AM, Tom Lane wrote:
> But let's not break the cases that do work.  One
> of the functions of contrib/ is to serve as models/skeletons for
> external modules.  If we pull out the "useless" PGXS support then we'll
> just be making it that much harder to copy a contrib module and start
> doing something useful.

Well, this is exactly the problem.  Because of this skeleton idea, most
external extension modules do not build unless you set USE_PGXS=1 before
building, because they think that they live in contrib by default, which
is completely bizarre and user-unfriendly.

We could have an actual example or skeleton whose purpose is to teach
extension authors.  The actual contrib module makefiles should just do
their job and don't pretend to teach things that are misguided and/or
don't work.

On 26 February 2013 17:02, Peter Eisentraut <peter_e@gmx.net> wrote:
> Well, this is exactly the problem.  Because of this skeleton idea, most
> external extension modules do not build unless you set USE_PGXS=1 before
> building, because they think that they live in contrib by default, which
> is completely bizarre and user-unfriendly.

repmgr is a popular external extension module. The README actually
suggests moving the entire source tree into /contrib as an alternative
to setting USE_PGXS=1. That does seem kind of weird, and yet I can
understand the train of thought.

My advice to others working on external modules would not be to
generalise from the example of /contrib, but to generalise from the
example of popular external modules. This is particularly important
when targeting multiple Postgres versions.


-- 
Regards,
Peter Geoghegan

On Tue, Feb 26, 2013 at 12:02 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 2/26/13 11:45 AM, Tom Lane wrote:
>> But let's not break the cases that do work.  One
>> of the functions of contrib/ is to serve as models/skeletons for
>> external modules.  If we pull out the "useless" PGXS support then we'll
>> just be making it that much harder to copy a contrib module and start
>> doing something useful.
>
> Well, this is exactly the problem.  Because of this skeleton idea, most
> external extension modules do not build unless you set USE_PGXS=1 before
> building, because they think that they live in contrib by default, which
> is completely bizarre and user-unfriendly.
>
> We could have an actual example or skeleton whose purpose is to teach
> extension authors.  The actual contrib module makefiles should just do
> their job and don't pretend to teach things that are misguided and/or
> don't work.

I couldn't agree more.  I can't think how much time I've wasted by
forgetting (or remembering) to type USE_PGXS=1 over the years, or not
realizing that I needed to.  I'm sure there are many other people in
the same boat.

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