Thread: includedir_internal headers are not self-contained

Debian is shipping client headers in /usr/include/postgresql in the
libpq-dev package. The server headers go into
/usr/include/postgresql/<major>/server in postgresql-server-dev-<major>,
so we can have the headers for several majors installed in parallel.

Historically, a few server headers were also included in libpq-dev
because 9 years ago, there were some client apps that needed them.
We've finally got around to fix that [1], now the layout is:

libpq-dev: /usr/include/postgresql/internal/* /usr/include/postgresql/libpq-fe.h /usr/include/postgresql/libpq-events.h
/usr/include/postgresql/libpq/libpq-fs.h/usr/include/postgresql/pg_config*.h /usr/include/postgresql/postgres_ext.h
 

postgresql-server-dev-<major>: /usr/include/postgresql/<major>/server/*

Unfortunately the files in internal/ are not self-contained: internal/postgres_fe.h includes common/fe_memutils.h which
includesutils/palloc.h
 

Both common/ and utils/ are server-only, so you can't build client
apps which need postgres_fe.h with only libpq-dev installed.

common/ was introduced in 8396447cdbdff0b62914748de2fec04281dc9114,
and added to src/include/Makefile in c153530dc10bf5ff6dc5a89249f9cb596dd71a63.

I believe common/ should be also be installed by includedir_internal.
utils/ should probably also be installed there, alternatively only the
headers referred to from common/, the files directly referred being:

$ grep -r include 9.4/server/common/ | grep \"
9.4/server/common/fe_memutils.h:#include "utils/palloc.h"
9.4/server/common/relpath.h:#include "catalog/catversion.h" /* pgrminclude ignore */
9.4/server/common/relpath.h:#include "storage/relfilenode.h"

I'd write a patch for src/include/Makefile, but we'd need to sort out
the layout first.

On a sidenote, I don't see why utils/errcodes.h and utils/fmgroids.h
need a separate INSTALL_DATA call when they are installed by into
utils/ anyway.

(Another issue is that client apps frequently seem to want
catalog/pg_type.h to get the OID definitions, it might make sense to
move that also to internal/.)

Christoph

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=314427
-- 
cb@df7cb.de | http://www.df7cb.de/

Christoph Berg <cb@df7cb.de> writes:
>   internal/postgres_fe.h includes
>   common/fe_memutils.h which includes
>   utils/palloc.h

Hm.  It seems rather fundamentally broken to me that frontend code is
including palloc.h --- that file was never intended to be frontend-safe,
and the #ifdefs that I see in it today don't fill me with any feeling of
quality workmanship.

I think what we ought to do about this is get rid of the dependency
on palloc.h.

> Both common/ and utils/ are server-only, so you can't build client
> apps which need postgres_fe.h with only libpq-dev installed.

Clearly, the idea that common/ is server-only is broken.

> I believe common/ should be also be installed by includedir_internal.
> utils/ should probably also be installed there, alternatively only the
> headers referred to from common/, the files directly referred being:

> $ grep -r include 9.4/server/common/ | grep \"
> 9.4/server/common/fe_memutils.h:#include "utils/palloc.h"
> 9.4/server/common/relpath.h:#include "catalog/catversion.h" /* pgrminclude ignore */
> 9.4/server/common/relpath.h:#include "storage/relfilenode.h"

The catversion dependency also seems pretty damn brain-dead in this
context.  Let's see if we can get rid of that.  As for relfilenode,
if we need that in relpath.h maybe the answer is that relfilenode.h
has to be in common/.

Anyway, the bottom line for me is that utils/ is a server-only area and
therefore nothing in common/ ought to depend on it.

> (Another issue is that client apps frequently seem to want
> catalog/pg_type.h to get the OID definitions, it might make sense to
> move that also to internal/.)

That's not happening.  We do need some better solution for letting client
apps get hold of fixed type oids, but moving a catalog header someplace
else is not it.
        regards, tom lane

I wrote:
> Christoph Berg <cb@df7cb.de> writes:
>> $ grep -r include 9.4/server/common/ | grep \"
>> 9.4/server/common/fe_memutils.h:#include "utils/palloc.h"
>> 9.4/server/common/relpath.h:#include "catalog/catversion.h" /* pgrminclude ignore */
>> 9.4/server/common/relpath.h:#include "storage/relfilenode.h"

> The catversion dependency also seems pretty damn brain-dead in this
> context.  Let's see if we can get rid of that.  As for relfilenode,
> if we need that in relpath.h maybe the answer is that relfilenode.h
> has to be in common/.

On closer inspection, the issue here is really that putting relpath.h/.c
in common/ was completely misguided from the get-go.  It's unnecessary:
there's nothing outside the backend that uses it, except for
contrib/pg_xlogdump which could very easily do without it.  And relpath.h
is a serious failure from a modularity standpoint anyway, because there is
code all over the backend that has intimate familiarity with the pathname
construction rules.  We could possibly clean that up to the extent of
being able to hide TABLESPACE_VERSION_DIRECTORY inside relpath.c, but what
then?  We'd still be talking about having CATALOG_VERSION_NO compiled into
frontend code for any frontend code that actually made use of relpath.c,
which is surely not such a great idea.

So it seems to me the right fix for the relpath end of it is to push most
of relpath.c back where it came from, which I think was backend/catalog/.

There might be some value in keeping the forkname-related code in common/;
that's not quite so intimately tied to the backend version as relpath()
itself.  (And indeed forkNames[] is the only thing that pg_xlogdump.c
needs.)  But I'm not really convinced that a module encapsulating just the
fork names is worth the trouble, and especially not convinced that 
frontend code needs to be dealing with fork names.  Thoughts?
        regards, tom lane

Re: Tom Lane 2014-04-26 <21449.1398524746@sss.pgh.pa.us>
> >   internal/postgres_fe.h includes
> >   common/fe_memutils.h which includes
> >   utils/palloc.h
> 
> Hm.  It seems rather fundamentally broken to me that frontend code is
> including palloc.h --- that file was never intended to be frontend-safe,
> and the #ifdefs that I see in it today don't fill me with any feeling of
> quality workmanship.
> 
> I think what we ought to do about this is get rid of the dependency
> on palloc.h.

Thanks for the first tranche of patches on this.

> > Both common/ and utils/ are server-only, so you can't build client
> > apps which need postgres_fe.h with only libpq-dev installed.
> 
> Clearly, the idea that common/ is server-only is broken.

The next step should probably be something like this:

diff --git a/src/include/Makefile b/src/include/Makefile
new file mode 100644
index 578a778..d39aa97
*** a/src/include/Makefile
--- b/src/include/Makefile
*************** include $(top_builddir)/src/Makefile.glo
*** 16,21 ****
--- 16,23 ---- all: pg_config.h pg_config_ext.h pg_config_os.h  
+ # Subdirectories containing headers for client- and server-side dev
+ SUBDIRS_INTERNAL = common # Subdirectories containing headers for server-side dev SUBDIRS = access bootstrap catalog
commandscommon datatype executor foreign \     lib libpq mb nodes optimizer parser postmaster regex replication \
 
*************** install: all installdirs
*** 38,50 ****     $(INSTALL_DATA) $(srcdir)/port.h         '$(DESTDIR)$(includedir_internal)'     $(INSTALL_DATA)
$(srcdir)/postgres_fe.h '$(DESTDIR)$(includedir_internal)'     $(INSTALL_DATA) $(srcdir)/libpq/pqcomm.h
'$(DESTDIR)$(includedir_internal)/libpq'# These headers are needed for server-side development     $(INSTALL_DATA)
pg_config.h    '$(DESTDIR)$(includedir_server)'     $(INSTALL_DATA) pg_config_ext.h '$(DESTDIR)$(includedir_server)'
$(INSTALL_DATA) pg_config_os.h  '$(DESTDIR)$(includedir_server)'     $(INSTALL_DATA) utils/errcodes.h
'$(DESTDIR)$(includedir_server)/utils'    $(INSTALL_DATA) utils/fmgroids.h '$(DESTDIR)$(includedir_server)/utils'
 
- # We don't use INSTALL_DATA for performance reasons --- there are a lot of files     cp $(srcdir)/*.h
'$(DESTDIR)$(includedir_server)'/|| exit; \     chmod $(INSTALL_DATA_MODE) '$(DESTDIR)$(includedir_server)'/*.h  ||
exit;\     for dir in $(SUBDIRS); do \
 
--- 40,56 ----     $(INSTALL_DATA) $(srcdir)/port.h         '$(DESTDIR)$(includedir_internal)'     $(INSTALL_DATA)
$(srcdir)/postgres_fe.h '$(DESTDIR)$(includedir_internal)'     $(INSTALL_DATA) $(srcdir)/libpq/pqcomm.h
'$(DESTDIR)$(includedir_internal)/libpq'
+ # We don't use INSTALL_DATA for performance reasons --- there are a lot of files
+     for dir in $(SUBDIRS_INTERNAL); do \
+       cp $(srcdir)/$$dir/*.h '$(DESTDIR)$(includedir_internal)'/$$dir/ || exit; \
+       chmod $(INSTALL_DATA_MODE) '$(DESTDIR)$(includedir_internal)'/$$dir/*.h  || exit; \
+     done # These headers are needed for server-side development     $(INSTALL_DATA) pg_config.h
'$(DESTDIR)$(includedir_server)'    $(INSTALL_DATA) pg_config_ext.h '$(DESTDIR)$(includedir_server)'
$(INSTALL_DATA)pg_config_os.h  '$(DESTDIR)$(includedir_server)'     $(INSTALL_DATA) utils/errcodes.h
'$(DESTDIR)$(includedir_server)/utils'    $(INSTALL_DATA) utils/fmgroids.h '$(DESTDIR)$(includedir_server)/utils'
cp$(srcdir)/*.h '$(DESTDIR)$(includedir_server)'/ || exit; \     chmod $(INSTALL_DATA_MODE)
'$(DESTDIR)$(includedir_server)'/*.h || exit; \     for dir in $(SUBDIRS); do \
 
*************** ifeq ($(vpath_build),yes)
*** 59,65 **** endif  installdirs:
!     $(MKDIR_P) '$(DESTDIR)$(includedir)/libpq' '$(DESTDIR)$(includedir_internal)/libpq'     $(MKDIR_P) $(addprefix
'$(DESTDIR)$(includedir_server)'/,$(SUBDIRS))  
 
--- 65,72 ---- endif  installdirs:
!     $(MKDIR_P) '$(DESTDIR)$(includedir)/libpq'
!     $(MKDIR_P) '$(DESTDIR)$(includedir_internal)/common' '$(DESTDIR)$(includedir_internal)/libpq'     $(MKDIR_P)
$(addprefix'$(DESTDIR)$(includedir_server)'/, $(SUBDIRS))  
 

Depending on when 9.4 is coming out, Debian Jessie will probably be
releasing with 9.3. How much of these fixes could we expect to be
backported to 9.3?

> > (Another issue is that client apps frequently seem to want
> > catalog/pg_type.h to get the OID definitions, it might make sense to
> > move that also to internal/.)
> 
> That's not happening.  We do need some better solution for letting client
> apps get hold of fixed type oids, but moving a catalog header someplace
> else is not it.

Maybe a derived header file generated at build time?

grep '^#define.*OID\>' catalog/pg_type.h > common/pg_staticoids.h


Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

Christoph Berg <cb@df7cb.de> writes:
> Re: Tom Lane 2014-04-26 <21449.1398524746@sss.pgh.pa.us>
>> Clearly, the idea that common/ is server-only is broken.

> The next step should probably be something like this:

Somebody who's spent more time than I have on the "make install" support
will have to comment on this patch (Peter?).  I'll just note that the MSVC
build scripts would presumably need parallel fixes.

> Depending on when 9.4 is coming out, Debian Jessie will probably be
> releasing with 9.3. How much of these fixes could we expect to be
> backported to 9.3?

As you saw, I backported the palloc.h fix already.  My current thought
about the relpath.h mess is to fix it in HEAD, but not in 9.3: it'll be
rather invasive for a back-patch, and I doubt it'd be solving a real
problem since IMO no client-side code should be including that header
anyway.  You could just look the other way as far as the dangling
#includes go, or you could choose to remove relpath.h from the installed
non-server header fileset in your package-building script.

As for the proposed change in the set of installed header files,
my vote would probably be not to backport that; I think the risk of
breaking existing packaging recipes in a minor release outweighs
any likely benefit of adding these headers.


>>> (Another issue is that client apps frequently seem to want
>>> catalog/pg_type.h to get the OID definitions, it might make sense to
>>> move that also to internal/.)

>> That's not happening.  We do need some better solution for letting client
>> apps get hold of fixed type oids, but moving a catalog header someplace
>> else is not it.

> Maybe a derived header file generated at build time?
> grep '^#define.*OID\>' catalog/pg_type.h > common/pg_staticoids.h

Well, we need a debate first about what we're going to do and what set
of type OIDs we want to expose this way.  There might be a case for
exposing everything listed in pg_type.h (a la fmgroids.h), or maybe it
had better be more restricted.  But a simple grep as above would make
the exposed set dependent on the historical whims of backend coding,
which doesn't seem like a great plan.

There's also reason to think about whether we shouldn't expose
fixed function OIDs in the same way.  One need look no further than
src/interfaces/libpq/fe-lobj.c for client-side code that would greatly
appreciate being allowed to depend on constants from fmgroids.h.

In any case, this issue has been complained of off-and-on for at least
a dozen years.  I feel no urgency to fix it in 9.4, which is already
past feature freeze, so there's not time for a well-considered solution.
        regards, tom lane

I wrote:
> On closer inspection, the issue here is really that putting relpath.h/.c
> in common/ was completely misguided from the get-go.  It's unnecessary:
> there's nothing outside the backend that uses it, except for
> contrib/pg_xlogdump which could very easily do without it.  And relpath.h
> is a serious failure from a modularity standpoint anyway, because there is
> code all over the backend that has intimate familiarity with the pathname
> construction rules.  We could possibly clean that up to the extent of
> being able to hide TABLESPACE_VERSION_DIRECTORY inside relpath.c, but what
> then?  We'd still be talking about having CATALOG_VERSION_NO compiled into
> frontend code for any frontend code that actually made use of relpath.c,
> which is surely not such a great idea.

> So it seems to me the right fix for the relpath end of it is to push most
> of relpath.c back where it came from, which I think was backend/catalog/.

In short, what I'm proposing here is to revert commit
a73018392636ce832b09b5c31f6ad1f18a4643ea, lock stock n barrel.
According to the commit message, the point of that was to allow
pg_xlogdump to use relpath(), but I do not see it doing so; and
if it tried, I don't know where it would get an accurate value of
CATALOG_VERSION_NO from.  So I think that was just poorly thought out.

What contrib/pg_xlogdump *is* using is the forkNames[] array, which
it uses to print the fork-number field of a BkpBlock symbolically.
I'm inclined to think that printing numerically is good enough there,
and a lot more robust.

Comments?  If there's anyone who has a really good use-case for using
relpath() from outside the backend, better speak up.
        regards, tom lane

Hi,

On 2014-04-27 16:33:29 -0400, Tom Lane wrote:
> > So it seems to me the right fix for the relpath end of it is to push most
> > of relpath.c back where it came from, which I think was backend/catalog/.
> 
> In short, what I'm proposing here is to revert commit
> a73018392636ce832b09b5c31f6ad1f18a4643ea, lock stock n barrel.

That was Alvaro's, so he's probably going to need to comment here.

> According to the commit message, the point of that was to allow
> pg_xlogdump to use relpath(), but I do not see it doing so;

Well, pg_xlogdump.c itself doesn't use it, but some of the desc routines
do. Like e.g. xact_desc_commit().
So just reverting isn't going to do the trick. IIRC I just had a copy of
relpathbackend() in pg_xlogdump/compat.c, but that certainly doesn't
seem to be better than the current solution.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-27 16:33:29 -0400, Tom Lane wrote:
>> According to the commit message, the point of that was to allow
>> pg_xlogdump to use relpath(), but I do not see it doing so;

> Well, pg_xlogdump.c itself doesn't use it, but some of the desc routines
> do. Like e.g. xact_desc_commit().

Well, that can be undone easily enough.  I frankly think that printing a
file path there is more verbose and less readable than just printing the
db/ts/rel OIDs.  In any case, the current situation with having
include/common referencing backend-only headers is simply broken and
unacceptable.
        regards, tom lane

On 2014-04-27 16:55:51 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-04-27 16:33:29 -0400, Tom Lane wrote:
> >> According to the commit message, the point of that was to allow
> >> pg_xlogdump to use relpath(), but I do not see it doing so;
> 
> > Well, pg_xlogdump.c itself doesn't use it, but some of the desc routines
> > do. Like e.g. xact_desc_commit().
> 
> Well, that can be undone easily enough.  I frankly think that printing a
> file path there is more verbose and less readable than just printing the
> db/ts/rel OIDs.

Would at least also need to include the fork name.

>  In any case, the current situation with having
> include/common referencing backend-only headers is simply broken and
> unacceptable.

Agreed, I am not arguing that point. I guess the easiest fix here would
be to just move relpath.c into src/backend and ln -s it into
pg_xlogdump. Not pretty, but there's precedent.

Greetings,

Andres Freund

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

On 04/27/2014 11:33 PM, Tom Lane wrote:
> I wrote:
>> On closer inspection, the issue here is really that putting relpath.h/.c
>> in common/ was completely misguided from the get-go.  It's unnecessary:
>> there's nothing outside the backend that uses it, except for
>> contrib/pg_xlogdump which could very easily do without it.  And relpath.h
>> is a serious failure from a modularity standpoint anyway, because there is
>> code all over the backend that has intimate familiarity with the pathname
>> construction rules.  We could possibly clean that up to the extent of
>> being able to hide TABLESPACE_VERSION_DIRECTORY inside relpath.c, but what
>> then?  We'd still be talking about having CATALOG_VERSION_NO compiled into
>> frontend code for any frontend code that actually made use of relpath.c,
>> which is surely not such a great idea.
>
>> So it seems to me the right fix for the relpath end of it is to push most
>> of relpath.c back where it came from, which I think was backend/catalog/.
>
> In short, what I'm proposing here is to revert commit
> a73018392636ce832b09b5c31f6ad1f18a4643ea, lock stock n barrel.
> According to the commit message, the point of that was to allow
> pg_xlogdump to use relpath(), but I do not see it doing so; and
> if it tried, I don't know where it would get an accurate value of
> CATALOG_VERSION_NO from.  So I think that was just poorly thought out.
>
> What contrib/pg_xlogdump *is* using is the forkNames[] array, which
> it uses to print the fork-number field of a BkpBlock symbolically.
> I'm inclined to think that printing numerically is good enough there,
> and a lot more robust.
>
> Comments?  If there's anyone who has a really good use-case for using
> relpath() from outside the backend, better speak up.

I'm using it in the pg_rewind tool. It needs to know how to map 
relfilenodes to physical files.

It has quite intimate knowledge of the on-disk layout anyway, so it 
wouldn't be the end of the world to just copy-paste that code. But would 
rather not, of course.

- Heikki

Re: Heikki Linnakangas 2014-04-28 <535E09B7.3090706@vmware.com>
> >Comments?  If there's anyone who has a really good use-case for using
> >relpath() from outside the backend, better speak up.
> 
> I'm using it in the pg_rewind tool. It needs to know how to map relfilenodes
> to physical files.
> 
> It has quite intimate knowledge of the on-disk layout anyway, so it wouldn't
> be the end of the world to just copy-paste that code. But would rather not,
> of course.

Isn't pg_rewind so low-level server-close that it needs tons of server
headers anyway, including one that would still have relpath()? We are
talking here about what headers pure client apps need.

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/

On 04/28/2014 03:29 PM, Christoph Berg wrote:
> Re: Heikki Linnakangas 2014-04-28 <535E09B7.3090706@vmware.com>
>>> Comments?  If there's anyone who has a really good use-case for using
>>> relpath() from outside the backend, better speak up.
>>
>> I'm using it in the pg_rewind tool. It needs to know how to map relfilenodes
>> to physical files.
>>
>> It has quite intimate knowledge of the on-disk layout anyway, so it wouldn't
>> be the end of the world to just copy-paste that code. But would rather not,
>> of course.
>
> Isn't pg_rewind so low-level server-close that it needs tons of server
> headers anyway, including one that would still have relpath()? We are
> talking here about what headers pure client apps need.

It knows how to decode WAL, similar to pg_xlogdump. And it knows about 
the data directory layout, in particular, how relfilenodes are mapped to 
physical files. Those are the low-level parts. So, it certainly needs 
some server headers, but I wouldn't call it tons.

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 04/28/2014 03:29 PM, Christoph Berg wrote:
>> Re: Heikki Linnakangas 2014-04-28 <535E09B7.3090706@vmware.com>
>>> I'm using it in the pg_rewind tool. It needs to know how to map relfilenodes
>>> to physical files.

>> Isn't pg_rewind so low-level server-close that it needs tons of server
>> headers anyway, including one that would still have relpath()? We are
>> talking here about what headers pure client apps need.

> It knows how to decode WAL, similar to pg_xlogdump. And it knows about 
> the data directory layout, in particular, how relfilenodes are mapped to 
> physical files. Those are the low-level parts. So, it certainly needs 
> some server headers, but I wouldn't call it tons.

I'm not even worried about which headers this program uses.  What I'm
worried about is that you've got CATALOG_VERSION_NO compiled into a
non-server executable.  Is that really such a great idea?  Wouldn't it be
better if pg_rewind did not depend on that?  (Perhaps it should get the
database's catalog version out of the pg_control file, for example.)

In short, while I don't deny that there may be non-server programs
that need to know about physical file paths, I do strongly doubt
that relpath.h/.c in their current form are a good solution to that
problem.
        regards, tom lane

On 04/28/2014 04:51 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 04/28/2014 03:29 PM, Christoph Berg wrote:
>>> Re: Heikki Linnakangas 2014-04-28 <535E09B7.3090706@vmware.com>
>>>> I'm using it in the pg_rewind tool. It needs to know how to map relfilenodes
>>>> to physical files.
>
>>> Isn't pg_rewind so low-level server-close that it needs tons of server
>>> headers anyway, including one that would still have relpath()? We are
>>> talking here about what headers pure client apps need.
>
>> It knows how to decode WAL, similar to pg_xlogdump. And it knows about
>> the data directory layout, in particular, how relfilenodes are mapped to
>> physical files. Those are the low-level parts. So, it certainly needs
>> some server headers, but I wouldn't call it tons.
>
> I'm not even worried about which headers this program uses.  What I'm
> worried about is that you've got CATALOG_VERSION_NO compiled into a
> non-server executable.  Is that really such a great idea?  Wouldn't it be
> better if pg_rewind did not depend on that?  (Perhaps it should get the
> database's catalog version out of the pg_control file, for example.)

Sure, that would be better. Although I don't have much hope to make it 
completely version-independent. At the moment, pg_rewind explicitly 
reads the control file (yeah, it knows about that too), and checks that 
the catalog version matches what pg_rewind was compiled with.

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 04/28/2014 04:51 PM, Tom Lane wrote:
>> I'm not even worried about which headers this program uses.  What I'm
>> worried about is that you've got CATALOG_VERSION_NO compiled into a
>> non-server executable.  Is that really such a great idea?  Wouldn't it be
>> better if pg_rewind did not depend on that?  (Perhaps it should get the
>> database's catalog version out of the pg_control file, for example.)

> Sure, that would be better. Although I don't have much hope to make it 
> completely version-independent. At the moment, pg_rewind explicitly 
> reads the control file (yeah, it knows about that too), and checks that 
> the catalog version matches what pg_rewind was compiled with.

... which might or might not be the same one that libpgcommon was compiled
with, no?  I don't think you're really protecting yourself against version
skew that way.
        regards, tom lane

Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> > On 04/28/2014 04:51 PM, Tom Lane wrote:
> >> I'm not even worried about which headers this program uses.  What I'm
> >> worried about is that you've got CATALOG_VERSION_NO compiled into a
> >> non-server executable.  Is that really such a great idea?  Wouldn't it be
> >> better if pg_rewind did not depend on that?  (Perhaps it should get the
> >> database's catalog version out of the pg_control file, for example.)
> 
> > Sure, that would be better. Although I don't have much hope to make it 
> > completely version-independent. At the moment, pg_rewind explicitly 
> > reads the control file (yeah, it knows about that too), and checks that 
> > the catalog version matches what pg_rewind was compiled with.
> 
> ... which might or might not be the same one that libpgcommon was compiled
> with, no?  I don't think you're really protecting yourself against version
> skew that way.

The CATALOG_VERSION dependency in that code is a mistake which I didn't
notice back then.  I can't put too much thought into this issue at this
time, but printing fork numbers rather than names seems pretty
user-unfriendly to me.  Rather than a revert of the whole patch I
would hope for some different solution, if possible, though I can't
offer anything right now.

I don't think it's very likely that we would renumber forks; so the only
possible problem would be that pg_rewind is linked with an older
libpgcommon than the server which doesn't know some newer fork
name/number and fails to produce a correct result.  But ISTM we can
rightly consider that as pilot error, right?

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> ... which might or might not be the same one that libpgcommon was compiled
>> with, no?  I don't think you're really protecting yourself against version
>> skew that way.

> The CATALOG_VERSION dependency in that code is a mistake which I didn't
> notice back then.  I can't put too much thought into this issue at this
> time, but printing fork numbers rather than names seems pretty
> user-unfriendly to me.  Rather than a revert of the whole patch I
> would hope for some different solution, if possible, though I can't
> offer anything right now.

I think it would be okay to have a common/ module that encapsulates
fork names/numbers.  It's relpath() and particularly
TABLESPACE_VERSION_DIRECTORY that bother me from a dependency standpoint.

As far as pg_xlogdump goes, I agree that symbolic fork names are probably
nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
whole lot weaker --- to my taste, that's actually an anti-feature.  So I
wouldn't mind dropping that dependency on relpath.  Not sure what to do
for Heikki's pg_rewind, though.
        regards, tom lane

On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Tom Lane wrote:
>>> ... which might or might not be the same one that libpgcommon was compiled
>>> with, no?  I don't think you're really protecting yourself against version
>>> skew that way.
>
>> The CATALOG_VERSION dependency in that code is a mistake which I didn't
>> notice back then.  I can't put too much thought into this issue at this
>> time, but printing fork numbers rather than names seems pretty
>> user-unfriendly to me.  Rather than a revert of the whole patch I
>> would hope for some different solution, if possible, though I can't
>> offer anything right now.
>
> I think it would be okay to have a common/ module that encapsulates
> fork names/numbers.  It's relpath() and particularly
> TABLESPACE_VERSION_DIRECTORY that bother me from a dependency standpoint.
>
> As far as pg_xlogdump goes, I agree that symbolic fork names are probably
> nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
> whole lot weaker --- to my taste, that's actually an anti-feature.

I might be missing something, but, why?

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As far as pg_xlogdump goes, I agree that symbolic fork names are probably
>> nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
>> whole lot weaker --- to my taste, that's actually an anti-feature.

> I might be missing something, but, why?

It's more verbose, it's not actually any more information, and in many
cases it's actively misleading, because what's printed is NOT the real
file name --- it omits segment numbers for instance.  As a particularly
egregious example, in xact_desc_commit() we print a pathname including
MAIN_FORKNUM, which is a flat out lie to the reader, because what will
actually get deleted is all forks.

So yeah, it's an anti-feature.

BTW, for the same reasons I'm less than impressed with the uses of
relpath in error messages in, eg, reorderbuffer.c:
                   relation = RelationIdGetRelation(reloid);
                   if (relation == NULL)                       elog(ERROR, "could open relation descriptor %s",
                  relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
 

Printing anything other than the relation OID here is irrelevant,
misleading, and inconsistent with our practice everywhere else.
Let's not even mention the missing "not" in the message text.
        regards, tom lane

On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> As far as pg_xlogdump goes, I agree that symbolic fork names are probably
>>> nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
>>> whole lot weaker --- to my taste, that's actually an anti-feature.
>
>> I might be missing something, but, why?
>
> It's more verbose, it's not actually any more information, and in many
> cases it's actively misleading, because what's printed is NOT the real
> file name --- it omits segment numbers for instance.  As a particularly
> egregious example, in xact_desc_commit() we print a pathname including
> MAIN_FORKNUM, which is a flat out lie to the reader, because what will
> actually get deleted is all forks.

Yeah, technically it's a lie, but ls <copy-and-paste-here>* is pretty
handy.  If you format it some other way it's annoying to reformat it.

> So yeah, it's an anti-feature.
>
> BTW, for the same reasons I'm less than impressed with the uses of
> relpath in error messages in, eg, reorderbuffer.c:
>
>                     relation = RelationIdGetRelation(reloid);
>
>                     if (relation == NULL)
>                         elog(ERROR, "could open relation descriptor %s",
>                              relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
>
> Printing anything other than the relation OID here is irrelevant,
> misleading, and inconsistent with our practice everywhere else.
> Let's not even mention the missing "not" in the message text.

Uggh.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It's more verbose, it's not actually any more information, and in many
>> cases it's actively misleading, because what's printed is NOT the real
>> file name --- it omits segment numbers for instance.  As a particularly
>> egregious example, in xact_desc_commit() we print a pathname including
>> MAIN_FORKNUM, which is a flat out lie to the reader, because what will
>> actually get deleted is all forks.

> Yeah, technically it's a lie, but ls <copy-and-paste-here>* is pretty
> handy.  If you format it some other way it's annoying to reformat it.

Handy for what?  How often do you need to do that?  (And if you do do it,
how often will you remember that the filename is only approximate?)
        regards, tom lane

On 2014-04-28 14:44:16 -0400, Robert Haas wrote:
> > BTW, for the same reasons I'm less than impressed with the uses of
> > relpath in error messages in, eg, reorderbuffer.c:
> >
> >                     relation = RelationIdGetRelation(reloid);
> >
> >                     if (relation == NULL)
> >                         elog(ERROR, "could open relation descriptor %s",
> >                              relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
> >
> > Printing anything other than the relation OID here is irrelevant,
> > misleading, and inconsistent with our practice everywhere else.
> > Let's not even mention the missing "not" in the message text.
> 
> Uggh.

I'll send a fix once I am home (~3h).

Greetings,

Andres Freund

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

On Mon, Apr 28, 2014 at 2:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> It's more verbose, it's not actually any more information, and in many
>>> cases it's actively misleading, because what's printed is NOT the real
>>> file name --- it omits segment numbers for instance.  As a particularly
>>> egregious example, in xact_desc_commit() we print a pathname including
>>> MAIN_FORKNUM, which is a flat out lie to the reader, because what will
>>> actually get deleted is all forks.
>
>> Yeah, technically it's a lie, but ls <copy-and-paste-here>* is pretty
>> handy.  If you format it some other way it's annoying to reformat it.
>
> Handy for what?  How often do you need to do that?  (And if you do do it,
> how often will you remember that the filename is only approximate?)

*shrug*

I think if you're looking at the output of xact_desc_commit() and you
*don't* know that the filenames are approximate (or at least that you
should Use The Source, Luke) then you're probably in way over your
head.

I have to admit it's been a few years since I've had to play with
WAL_DEBUG, so I don't really remember what I was trying to do.  But I
don't see any real argument that three slash-separated numbers will be
more useful to somebody who has to dig through this than a pathname,
even an approximate pathname, and I think people wanting to figure out
approximately where they need to look to find the data affected by the
WAL record will be pretty common among people decoding WAL records.

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

Robert Haas <robertmhaas@gmail.com> writes:
> I have to admit it's been a few years since I've had to play with
> WAL_DEBUG, so I don't really remember what I was trying to do.  But I
> don't see any real argument that three slash-separated numbers will be
> more useful to somebody who has to dig through this than a pathname,
> even an approximate pathname, and I think people wanting to figure out
> approximately where they need to look to find the data affected by the
> WAL record will be pretty common among people decoding WAL records.

Meh.  I still think it's a bad idea to have CATALOG_VERSION_NO getting
compiled into libpgcommon.a, where there will be no way to cross-check
that it matches anything.  But I guess I'm losing this argument.

However, we've absolutely got to get that reference out of
common/relpath.h.  The usage of RelFileNode there is problematic
as well.

How about we change common/relpath.[hc] to export a single version of
relpath() that takes its arguments as separate plain OIDs, and then
make the other versions wrappers that are only declared in some
backend header?  The wrappers could possibly be macros, allowing
things like pg_xlogdump to keep using them as long as they didn't
mind importing backend headers.  (Though for the RelFileNode case this
would imply multiple evaluation of the macro argument, so maybe it's
not such a great idea.)
        regards, tom lane

On 2014-04-28 13:20:47 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> As far as pg_xlogdump goes, I agree that symbolic fork names are probably
> >> nice, but I think the case for printing db/ts/rel OIDs as a pathname is a
> >> whole lot weaker --- to my taste, that's actually an anti-feature.
>
> > I might be missing something, but, why?
>
> It's more verbose, it's not actually any more information, and in many
> cases it's actively misleading, because what's printed is NOT the real
> file name --- it omits segment numbers for instance.  As a particularly
> egregious example, in xact_desc_commit() we print a pathname including
> MAIN_FORKNUM, which is a flat out lie to the reader, because what will
> actually get deleted is all forks.
>
> So yeah, it's an anti-feature.
>
> BTW, for the same reasons I'm less than impressed with the uses of
> relpath in error messages in, eg, reorderbuffer.c:
>
>                     relation = RelationIdGetRelation(reloid);
>
>                     if (relation == NULL)
>                         elog(ERROR, "could open relation descriptor %s",
>                              relpathperm(change->data.tp.relnode, MAIN_FORKNUM));
>
> Printing anything other than the relation OID here is irrelevant,
> misleading, and inconsistent with our practice everywhere else.

I don't think it's really comparable to the other scenarios. We should
print the oid, just as relation_open() does, but the filenode is also
rather helpful here. How about the attached?

If we had a version of relpath() that didn't require the fsm, I'd use
it. If you prefer, I'd be happy enough to replace it with
spcNode/dbNode/relNode. That's more than sufficient here.

> Let's not even mention the missing "not" in the message text.

That's clearly wrong.

Greetings,

Andres Freund

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

On 04/28/2014 10:32 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I have to admit it's been a few years since I've had to play with
>> WAL_DEBUG, so I don't really remember what I was trying to do.  But I
>> don't see any real argument that three slash-separated numbers will be
>> more useful to somebody who has to dig through this than a pathname,
>> even an approximate pathname, and I think people wanting to figure out
>> approximately where they need to look to find the data affected by the
>> WAL record will be pretty common among people decoding WAL records.
>
> Meh.  I still think it's a bad idea to have CATALOG_VERSION_NO getting
> compiled into libpgcommon.a, where there will be no way to cross-check
> that it matches anything.  But I guess I'm losing this argument.

FWIW, I agree it's a bad idea. I just have no better ideas (and haven't 
given it much thought anyway).

- Heikki

On 04/29/2014 02:56 AM, Heikki Linnakangas wrote:
> On 04/28/2014 10:32 PM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> I have to admit it's been a few years since I've had to play with
>>> WAL_DEBUG, so I don't really remember what I was trying to do.  But I
>>> don't see any real argument that three slash-separated numbers will be
>>> more useful to somebody who has to dig through this than a pathname,
>>> even an approximate pathname, and I think people wanting to figure out
>>> approximately where they need to look to find the data affected by the
>>> WAL record will be pretty common among people decoding WAL records.
>>
>> Meh.  I still think it's a bad idea to have CATALOG_VERSION_NO getting
>> compiled into libpgcommon.a, where there will be no way to cross-check
>> that it matches anything.  But I guess I'm losing this argument.
>
> FWIW, I agree it's a bad idea. I just have no better ideas (and 
> haven't given it much thought anyway).
>
>

Sure sounds like a bad idea.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 04/29/2014 02:56 AM, Heikki Linnakangas wrote:
>> On 04/28/2014 10:32 PM, Tom Lane wrote:
>>> Meh.  I still think it's a bad idea to have CATALOG_VERSION_NO getting
>>> compiled into libpgcommon.a, where there will be no way to cross-check
>>> that it matches anything.  But I guess I'm losing this argument.

>> FWIW, I agree it's a bad idea. I just have no better ideas (and 
>> haven't given it much thought anyway).

> Sure sounds like a bad idea.

One idea would be to have relpath.h/.c expose a function (not a #define)
that returns the value of CATALOG_VERSION_NO compiled into it.  Then,
if pg_rewind were to replace all its direct references to
CATALOG_VERSION_NO (including the value it checks against pg_control)
with calls of that function, consistency would be ensured.

A notational problem is that if pg_rewind or similar program is directly
using the TABLESPACE_VERSION_DIRECTORY macro anywhere, this wouldn't
work.  But we could perhaps expose a function to return that string too.
        regards, tom lane

On 04/29/2014 06:00 PM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 04/29/2014 02:56 AM, Heikki Linnakangas wrote:
>>> On 04/28/2014 10:32 PM, Tom Lane wrote:
>>>> Meh.  I still think it's a bad idea to have CATALOG_VERSION_NO getting
>>>> compiled into libpgcommon.a, where there will be no way to cross-check
>>>> that it matches anything.  But I guess I'm losing this argument.
>
>>> FWIW, I agree it's a bad idea. I just have no better ideas (and
>>> haven't given it much thought anyway).
>
>> Sure sounds like a bad idea.
>
> One idea would be to have relpath.h/.c expose a function (not a #define)
> that returns the value of CATALOG_VERSION_NO compiled into it.  Then,
> if pg_rewind were to replace all its direct references to
> CATALOG_VERSION_NO (including the value it checks against pg_control)
> with calls of that function, consistency would be ensured.

In pg_rewind, I'd like to compile CATALOG_VERSION_NO into the binary 
itself, because pg_rewind is quite version-specific. Even if it happens 
to work with libpgport from a different version, I would worry that 
there are directory layout changes that would need to be handled in 
pg_rewind for it to work safely. So I would like to lock it to a 
specific catalog version.

To lock it down, I could call the function and check that it matches the 
compiled-in value of CATALOG_VERSION_NO, though. So a function works for 
me, even though I don't really need the flexibility.

> A notational problem is that if pg_rewind or similar program is directly
> using the TABLESPACE_VERSION_DIRECTORY macro anywhere, this wouldn't
> work.  But we could perhaps expose a function to return that string too.

pg_rewind doesn't use TABLESPACE_VERSION_DIRECTORY directly.

- Heikki

I wrote:
> How about we change common/relpath.[hc] to export a single version of
> relpath() that takes its arguments as separate plain OIDs, and then
> make the other versions wrappers that are only declared in some
> backend header?  The wrappers could possibly be macros, allowing
> things like pg_xlogdump to keep using them as long as they didn't
> mind importing backend headers.  (Though for the RelFileNode case this
> would imply multiple evaluation of the macro argument, so maybe it's
> not such a great idea.)

Since nobody objected, I've committed something along this line.
include/common/ is now free of references to backend headers.
The patch is certainly too invasive to consider back-patching into
9.3, though.

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> One idea would be to have relpath.h/.c expose a function (not a #define)
>> that returns the value of CATALOG_VERSION_NO compiled into it.  Then,
>> if pg_rewind were to replace all its direct references to
>> CATALOG_VERSION_NO (including the value it checks against pg_control)
>> with calls of that function, consistency would be ensured.

> In pg_rewind, I'd like to compile CATALOG_VERSION_NO into the binary 
> itself, because pg_rewind is quite version-specific. Even if it happens 
> to work with libpgport from a different version, I would worry that 
> there are directory layout changes that would need to be handled in 
> pg_rewind for it to work safely. So I would like to lock it to a 
> specific catalog version.

> To lock it down, I could call the function and check that it matches the 
> compiled-in value of CATALOG_VERSION_NO, though. So a function works for 
> me, even though I don't really need the flexibility.

I didn't do anything about this idea, but if you want to follow through
with it, feel free to add such a function.
        regards, tom lane

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-28 13:20:47 -0400, Tom Lane wrote:
>> Printing anything other than the relation OID here is irrelevant,
>> misleading, and inconsistent with our practice everywhere else.

> I don't think it's really comparable to the other scenarios. We should
> print the oid, just as relation_open() does, but the filenode is also
> rather helpful here. How about the attached?

Applied along with a bit of other cleanup.
        regards, tom lane

Tom Lane wrote:
> I wrote:
> > How about we change common/relpath.[hc] to export a single version of
> > relpath() that takes its arguments as separate plain OIDs, and then
> > make the other versions wrappers that are only declared in some
> > backend header?  The wrappers could possibly be macros, allowing
> > things like pg_xlogdump to keep using them as long as they didn't
> > mind importing backend headers.  (Though for the RelFileNode case this
> > would imply multiple evaluation of the macro argument, so maybe it's
> > not such a great idea.)
> 
> Since nobody objected, I've committed something along this line.
> include/common/ is now free of references to backend headers.

Many thanks for the extra effort.

> The patch is certainly too invasive to consider back-patching into
> 9.3, though.

I feel unsure about this.  I agree the patch is quite invasive.  Leaving
9.3 in a broken state seems problematic.  In particular I'm not sure
what would Debian do about the whole issue; would they have to carry the
patch for their 9.3 packages?

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> The patch is certainly too invasive to consider back-patching into
>> 9.3, though.

> I feel unsure about this.  I agree the patch is quite invasive.  Leaving
> 9.3 in a broken state seems problematic.  In particular I'm not sure
> what would Debian do about the whole issue; would they have to carry the
> patch for their 9.3 packages?

My recommendation to Christoph upthread was that they just look the
other way for the time being, ie, ignore the fact that relpath.h is
unusable by freestanding apps in 9.3.  Backpatching what I did for
9.4 would be an ABI break, so that seems to me to be out of the
question in 9.3.  And it's not clear that anybody outside core+contrib
really needs relpath.h yet, anyway.  (Of course, you could argue that
if there are no external users then the ABI break isn't a problem;
but if there are, then it is.)
        regards, tom lane

Re: Tom Lane 2014-05-02 <9995.1398994851@sss.pgh.pa.us>
> >> The patch is certainly too invasive to consider back-patching into
> >> 9.3, though.

Understood.

> > I feel unsure about this.  I agree the patch is quite invasive.  Leaving
> > 9.3 in a broken state seems problematic.  In particular I'm not sure
> > what would Debian do about the whole issue; would they have to carry the
> > patch for their 9.3 packages?
> 
> My recommendation to Christoph upthread was that they just look the
> other way for the time being, ie, ignore the fact that relpath.h is
> unusable by freestanding apps in 9.3.  Backpatching what I did for
> 9.4 would be an ABI break, so that seems to me to be out of the
> question in 9.3.  And it's not clear that anybody outside core+contrib
> really needs relpath.h yet, anyway.  (Of course, you could argue that
> if there are no external users then the ABI break isn't a problem;
> but if there are, then it is.)

We are certainly not going to replace the old mess by a custom new
one ;)

The original problem that postgres_fe.h wasn't usable is already fixed
for 9.3, so afaict the only remaining problem there seems the
installation {rule, location} of common/, which is either taken care
of by the patch I've sent, or a trivial addition to the packaging
files on our side.

As long as there's no complaints, we'll simply ignore the fact that
the other headers in 9.3's common/ aren't self-contained, the
workaround to simply install the server headers seems easy enough.

We should probably be able to move to 9.4 in time for the freeze of
Debian Jessie in November, so backports won't matter that much. (As
long as the 9.3-and-older server-headers are self-contained and/or
compatible with what 9.4 provides...)

Christoph
-- 
cb@df7cb.de | http://www.df7cb.de/