Re: [HACKERS] includedir_internal headers are not self-contained
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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