Re: [HACKERS] includedir_internal headers are not self-contained

2014-05-02 Thread Christoph Berg
Re: Tom Lane 2014-05-02 9995.1398994...@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
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-05-01 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-05-01 Thread Tom Lane
Alvaro Herrera alvhe...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-30 Thread Heikki Linnakangas

On 04/29/2014 06:00 PM, Tom Lane wrote:

Andrew Dunstan and...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-30 Thread Tom Lane
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 hlinnakan...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-30 Thread Tom Lane
Andres Freund and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-29 Thread Heikki Linnakangas

On 04/28/2014 10:32 PM, Tom Lane wrote:

Robert Haas robertmh...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-29 Thread Andrew Dunstan


On 04/29/2014 02:56 AM, Heikki Linnakangas wrote:

On 04/28/2014 10:32 PM, Tom Lane wrote:

Robert Haas robertmh...@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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-29 Thread Tom Lane
Andrew Dunstan and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Heikki Linnakangas

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Christoph Berg
Re: Heikki Linnakangas 2014-04-28 535e09b7.3090...@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
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Heikki Linnakangas

On 04/28/2014 03:29 PM, Christoph Berg wrote:

Re: Heikki Linnakangas 2014-04-28 535e09b7.3090...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Heikki Linnakangas hlinnakan...@vmware.com writes:
 On 04/28/2014 03:29 PM, Christoph Berg wrote:
 Re: Heikki Linnakangas 2014-04-28 535e09b7.3090...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Heikki Linnakangas

On 04/28/2014 04:51 PM, Tom Lane wrote:

Heikki Linnakangas hlinnakan...@vmware.com writes:

On 04/28/2014 03:29 PM, Christoph Berg wrote:

Re: Heikki Linnakangas 2014-04-28 535e09b7.3090...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Heikki Linnakangas hlinnakan...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Alvaro Herrera
Tom Lane wrote:
 Heikki Linnakangas hlinnakan...@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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Alvaro Herrera alvhe...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Robert Haas
On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Alvaro Herrera alvhe...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Robert Haas
On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes:
 On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Andres Freund
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Robert Haas
On Mon, Apr 28, 2014 at 2:50 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Mon, Apr 28, 2014 at 1:20 PM, Tom Lane t...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Tom Lane
Robert Haas robertmh...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-28 Thread Andres Freund
On 2014-04-28 13:20:47 -0400, Tom Lane wrote:
 Robert Haas robertmh...@gmail.com writes:
  On Mon, Apr 28, 2014 at 12:14 PM, Tom Lane t...@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
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4493930..0b9731b 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -1362,14 +1362,14 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
 		change-data.tp.oldtuple == NULL)
 		continue;
 	else if (reloid == InvalidOid)
-		elog(ERROR, could not lookup relation %s,
+		elog(ERROR, could not map filenode %s to its oid,
 			 relpathperm(change-data.tp.relnode, MAIN_FORKNUM));
 
 	relation = RelationIdGetRelation(reloid);
 
 	if (relation == NULL)
-		elog(ERROR, could open relation descriptor %s,
-			 relpathperm(change-data.tp.relnode, MAIN_FORKNUM));
+		elog(ERROR, could not open relation with oid %u, filenode %s,
+			 reloid, relpathperm(change-data.tp.relnode, MAIN_FORKNUM));
 
 	if (RelationIsLogicallyLogged(relation))
 	{

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread Christoph Berg
Re: Tom Lane 2014-04-26 21449.1398524...@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 commands common 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
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread Tom Lane
Christoph Berg c...@df7cb.de writes:
 Re: Tom Lane 2014-04-26 21449.1398524...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread Andres Freund
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread Tom Lane
Andres Freund and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-27 Thread Andres Freund
On 2014-04-27 16:55:51 -0400, Tom Lane wrote:
 Andres Freund and...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-26 Thread Tom Lane
Christoph Berg c...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] includedir_internal headers are not self-contained

2014-04-26 Thread Tom Lane
I wrote:
 Christoph Berg c...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers