Re: [HACKERS] changeset generation v5-01 - Patches git tree
Andres Freund wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 I gave this recently rebased branch a skim. In general, the separation between decode.c/reorderbuffer.c/snapbuild.c seems a lot nicer now than on previous iterations -- good job there. Here are some quick notes I took while reading the patch itself. I haven't gone through it really carefully, yet. - I wonder whether DecodeCommit and DecodeAbort should really be a single routine. Right now, the former might call the later; and the latter is aware of this. Seems awkward. - We skip insert/update/delete if not my database Id; however, we don't skip commit in the same case. If there are two walrecvrs on a cluster, on different databases, does this lead to us trying to remove files twice, if a xact commits which deleted some files? Is this a problem? Should we try to skip such database-specific actions in global WAL records? - There's rmgr-specific knowledge in decode.c. I wonder if, similar to redo and desc routines, that shouldn't instead be pluggable functions for each rmgr. - What's with ReorderBufferRestoreCleanup()? Shouldn't it be in logical.c? - reorderbuffer.c does several different things. Can it be split? Perhaps in pieces such as * stuff to manage memory (slab cache thingies) * TXN iterator * other logically separate parts? * the rest - Having to expose LocalExecuteInvalidationMessage() looks awkward. Is there another way? - I think we need a better name for treat_as_catalog_table (and RelationIsTreatedAsCatalogTable). Maybe replication_catalog or something similar? - Don't do this: + * RecentGlobal(Data)?Xmin is initialized to InvalidTransactionId, to ensure that no because later greps for RecentGlobalDataXmin and RecentGlobalXmin will fail to find it. It seems better to spell both names, so RecentGlobalDataXmin and RecentGlobalXmin are initialized to ... - the pg_receivellog command line is strange. Apparently I need one or more of --start,--init,--stop, but if stop, then the other two must not be present; and if startpos, then init and stop cannot be specified. (There's a typo there that says cannot combine with --start when it really means cannot combine with --stop, BTW). I think this would make more sense to have init,start,stop be commands, in pg_ctl's spirit; so there would be no double-dash. IOW SOMEPATH/pg_receivellog --startpos=123 start and so on. Also, we need SGML docs for this new utility. Any particular reason for removing this line: -/* Get a new XLogReader */ + extern XLogReaderState *XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data); Typo here (2n*d*Quadrant): += Snapshot Building = +:author: Andres Freund, 2nQuadrant Ltd I don't see the point of XLogRecordBuffer.record_data; we already have a pointer to the XLogRecord, and the data can readily be obtained using XLogRecGetData. So why provide the same thing twice? It seems to me that if instead of passing the XLogRecordBuffer we just provide the XLogRecord, and separately the origptr where needed, we could avoid having to expose the XLogRecordBuffer stuff unnecessarily. In this comment: + * FIXME: We need something resembling the real SnapshotNow to handle things + * like enum lookups from indices correctly. what do we need consider in light of the new comment proposed by Robert ca+tgmobvtjrj_doxxq0wga1a1jlypvyqtr3m+cou_ousabn...@mail.gmail.com -- Á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] changeset generation v5-01 - Patches git tree
On 2013-08-27 11:32:30 -0400, Alvaro Herrera wrote: Andres Freund wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 I gave this recently rebased branch a skim. In general, the separation between decode.c/reorderbuffer.c/snapbuild.c seems a lot nicer now than on previous iterations -- good job there. Thanks for having a look! Here are some quick notes I took while reading the patch itself. I haven't gone through it really carefully, yet. - I wonder whether DecodeCommit and DecodeAbort should really be a single routine. Right now, the former might call the later; and the latter is aware of this. Seems awkward. Yes, I am not happy with that either. I'll play with combining them and check whether that looks beter. - We skip insert/update/delete if not my database Id; however, we don't skip commit in the same case. If there are two walrecvrs on a cluster, on different databases, does this lead to us trying to remove files twice, if a xact commits which deleted some files? Is this a problem? Should we try to skip such database-specific actions in global WAL records? Hm. We should be able to skip it for long commit records at least. I think I lost that code along the unification. There's no danger of removing anything global afaics since we're not replaying using the original replay routines and all the slot/sender specific stuff has unique names. - There's rmgr-specific knowledge in decode.c. I wonder if, similar to redo and desc routines, that shouldn't instead be pluggable functions for each rmgr. I don't think that's a good idea. I've quickly played with it before and it doesn't seem to end happy. It would require opening up more semi-public interfaces and in the end, we're only interested of in-core stuff. Even if it were possible to add new indexes by plugging new rmgrs, we wouldn't care. - What's with ReorderBufferRestoreCleanup()? Shouldn't it be in logical.c? No, that's just for removing ondisk data at the end of a transaction. I'll improve the comment. - reorderbuffer.c does several different things. Can it be split? Perhaps in pieces such as * stuff to manage memory (slab cache thingies) * TXN iterator * other logically separate parts? * the rest Hm. I don't really see much point in splitting it along those lines. None of those really makes sense without the other parts and the file isn't *that* huge. - Having to expose LocalExecuteInvalidationMessage() looks awkward. Is there another way? Hm. I don't immediately see any way. We need to execute invalidation messages just within one backend. There just is no exposed functionality for that yet since it wasn't needed so far. We could expose something like LocalExecuteInvalidationMessage*s*() instead of doing the loop in reorderbuffer.c, but that's about it. - I think we need a better name for treat_as_catalog_table (and RelationIsTreatedAsCatalogTable). Maybe replication_catalog or something similar? I think we're going to end up needing that for more than just replication, so I'd like to keep replication out of the name. I don't like the current name either though, so any other ideas? - Don't do this: + * RecentGlobal(Data)?Xmin is initialized to InvalidTransactionId, to ensure that no because later greps for RecentGlobalDataXmin and RecentGlobalXmin will fail to find it. It seems better to spell both names, so RecentGlobalDataXmin and RecentGlobalXmin are initialized to ... Ok. - the pg_receivellog command line is strange. Apparently I need one or more of --start,--init,--stop, but if stop, then the other two must not be present; and if startpos, then init and stop cannot be specified. (There's a typo there that says cannot combine with --start when it really means cannot combine with --stop, BTW). I think this would make more sense to have init,start,stop be commands, in pg_ctl's spirit; so there would be no double-dash. IOW SOMEPATH/pg_receivellog --startpos=123 start and so on. The reasoning here is somewhat complex and I am not happy with the status quo, so I like getting input here. The individual verbs mean: * init: create a replication slot * start: continue streaming in an existing replication slot * stop: remove replication slot The reason you cannot specify anything with --stop is that a) --start streams until you abort the utility. So there's no chance of running --stop after it. b) --init and --stop seems like a pointless combination since you cannot actually do anything with the slot. --init and --start combined, on the other hand are useful for testing, which is why I allow them so far, but I wouldn't have problems removing that capability. The reason you cannot combine --init or --init
Re: [HACKERS] changeset generation v5-01 - Patches git tree
On 2013-07-22 13:50:08 -0400, Robert Haas wrote: On Fri, Jun 14, 2013 at 6:51 PM, Andres Freund and...@2ndquadrant.com wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 On 2013-06-15 00:48:17 +0200, Andres Freund wrote: Overview of the attached patches: 0001: indirect toast tuples; required but submitted independently 0002: functions for testing; not required, 0003: (tablespace, filenode) syscache; required 0004: RelationMapFilenodeToOid: required, simple 0005: pg_relation_by_filenode() function; not required but useful 0006: Introduce InvalidCommandId: required, simple 0007: Adjust Satisfies* interface: required, mechanical, 0008: Allow walsender to attach to a database: required, needs review 0009: New GetOldestXmin() parameter; required, pretty boring 0010: Log xl_running_xact regularly in the bgwriter: required 0011: make fsync_fname() public; required, needs to be in a different file 0012: Relcache support for an Relation's primary key: required 0013: Actual changeset extraction; required 0014: Output plugin demo; not required (except for testing) but useful 0015: Add pg_receivellog program: not required but useful 0016: Add test_logical_decoding extension; not required, but contains the tests for the feature. Uses 0014 0017: Snapshot building docs; not required I've now also committed patch #7 from this series. My earlier commit fulfilled the needs of patches #3, #4, and #5; and somewhat longer ago I committed #1. Thanks! I am not entirely convinced of the necessity or desirability of patch #6, but as of now I haven't studied the issues closely. Fair enough. It's certainly possible to work around not having it, but it seems cleaner to introduce the notion of an invalid CommandId like we have for transaction ids et al. Allowing 2^32-2 instead of 2^32-1 subtransactions doesn't seem like a problem to me ;) Patch #2 does not seem useful in isolation; it adds new regression-testing stuff but doesn't use it anywhere. Yes. I found it useful to test stuff around making replication synchronous or such, but while I think we should have a facility like it in core for both, logical and physical replication, I don't think this patch is ready for prime time due to it's busy looping. I've even marked it as such above ;) My first idea to properly implement that seems to be to reuse the syncrep infrastructure but that doesn't look trivial. I doubt that any of the remaining patches (#8-#17) can be applied separately without understanding the shape of the whole patch set, so I think I, or someone else, will need to set aside more time for detailed review before proceeding further with this patch set. I suggest that we close out the CommitFest entry for this patch set one way or another, as there is no way we're going to get the whole thing done under the auspices of CF1. Generally agreed. The biggest chunk of the code is in #13 anyway... Some may be applyable independently: 0010: Log xl_running_xact regularly in the bgwriter: required Should be useful independently since it can significantly speed up startup of physical replicas. Ony many systems checkpoint_timeout will be set to an hour which can make the time till a standby gets consistent be quite high since that will be first time it sees a xl_running_xacts again. 0011: make fsync_fname() public; required, needs to be in a different file Isn't in the shape for it atm, but could be applied as an independent infrastructure patch. And it should be easy enough to clean it up. 0012: Relcache support for an Relation's primary key: required Might actually be a good idea independently as well. E.g. the materalized key patch could use the information that there's a candidate key around to avoid a good bit of useless work. I'll try to find some more time to spend on this relatively soon, but I think this is about as far as I can take this today. Was pretty helpful already, so ... ;) 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] changeset generation v5-01 - Patches git tree
On Tue, Jul 16, 2013 at 9:00 AM, Robert Haas robertmh...@gmail.com wrote: On Sun, Jul 7, 2013 at 4:34 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-07-07 15:43:17 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: 3b) Add catcache 'filter' that ensures the cache stays unique and use that for the mapping I slightly prefer 3b) because it's smaller, what's your opinions? This is just another variation on the theme of kluging the catcache to do something it shouldn't. You're still building a catcache on a non-unique index, and that is going to lead to trouble. I don't think the lurking dangers really are present. The index essentially *is* unique since we filter away anything non-unique. The catcache code hardly can be confused by tuples it never sees. That would even work if we started preloading catcaches by doing scans of the entire underlying relation or by caching all of a page when reading one of its tuples. I can definitely see that there are aesthetical reasons against doing 3b), that's why I've also done 3a). So I'll chalk you up to voting for that... I also vote for (3a). I did a quick once over of 1, 2, and 3a and they look reasonable. Barring strenuous objections, I'd like to go ahead and commit these, or perhaps an updated version of them. Hearing no objections, I have done this. Per off-list discussion with Andres, I also included patch 4, which gives us regression test coverage for this code, and have fixed a few bugs and a bunch of stylistic things that bugged me. -- 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] changeset generation v5-01 - Patches git tree
On Fri, Jun 14, 2013 at 6:51 PM, Andres Freund and...@2ndquadrant.com wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 On 2013-06-15 00:48:17 +0200, Andres Freund wrote: Overview of the attached patches: 0001: indirect toast tuples; required but submitted independently 0002: functions for testing; not required, 0003: (tablespace, filenode) syscache; required 0004: RelationMapFilenodeToOid: required, simple 0005: pg_relation_by_filenode() function; not required but useful 0006: Introduce InvalidCommandId: required, simple 0007: Adjust Satisfies* interface: required, mechanical, 0008: Allow walsender to attach to a database: required, needs review 0009: New GetOldestXmin() parameter; required, pretty boring 0010: Log xl_running_xact regularly in the bgwriter: required 0011: make fsync_fname() public; required, needs to be in a different file 0012: Relcache support for an Relation's primary key: required 0013: Actual changeset extraction; required 0014: Output plugin demo; not required (except for testing) but useful 0015: Add pg_receivellog program: not required but useful 0016: Add test_logical_decoding extension; not required, but contains the tests for the feature. Uses 0014 0017: Snapshot building docs; not required I've now also committed patch #7 from this series. My earlier commit fulfilled the needs of patches #3, #4, and #5; and somewhat longer ago I committed #1. I am not entirely convinced of the necessity or desirability of patch #6, but as of now I haven't studied the issues closely. Patch #2 does not seem useful in isolation; it adds new regression-testing stuff but doesn't use it anywhere. I doubt that any of the remaining patches (#8-#17) can be applied separately without understanding the shape of the whole patch set, so I think I, or someone else, will need to set aside more time for detailed review before proceeding further with this patch set. I suggest that we close out the CommitFest entry for this patch set one way or another, as there is no way we're going to get the whole thing done under the auspices of CF1. I'll try to find some more time to spend on this relatively soon, but I think this is about as far as I can take this today. -- 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] changeset generation v5-01 - Patches git tree
On Sun, Jul 7, 2013 at 4:34 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-07-07 15:43:17 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: 3b) Add catcache 'filter' that ensures the cache stays unique and use that for the mapping I slightly prefer 3b) because it's smaller, what's your opinions? This is just another variation on the theme of kluging the catcache to do something it shouldn't. You're still building a catcache on a non-unique index, and that is going to lead to trouble. I don't think the lurking dangers really are present. The index essentially *is* unique since we filter away anything non-unique. The catcache code hardly can be confused by tuples it never sees. That would even work if we started preloading catcaches by doing scans of the entire underlying relation or by caching all of a page when reading one of its tuples. I can definitely see that there are aesthetical reasons against doing 3b), that's why I've also done 3a). So I'll chalk you up to voting for that... I also vote for (3a). I did a quick once over of 1, 2, and 3a and they look reasonable. Barring strenuous objections, I'd like to go ahead and commit these, or perhaps an updated version of them. -- 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] changeset generation v5-01 - Patches git tree
Sorry for the delay in reviewing this. I must make sure never to take another vacation during a commitfest -- the backlog upon return is a killer Kevin Grittner kgri...@ymail.com wrote: Andres Freund and...@2ndquadrant.com wrote: Otherwise, could you try applying my git tree so we are sure we test the same thing? $ git remote add af git://git.postgresql.org/git/users/andresfreund/postgres.git $ git fetch af $ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4 $ ./configure ... $ make Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. The good news: the regression tests now work for me, and I'm back on testing this at a high level. The bad news: (1) The code checked out from that branch does not merge with master. Not surprisingly, given the recent commits, xlog.c is a problem. Is there another branch I should now be using? If not, please let me know when I can test with something that applies on top of the master branch. (2) An initial performance test didn't look very good. I will be running a more controlled test to confirm but the logical replication of a benchmark with a lot of UPDATEs of compressed text values seemed to suffer with the logical replication turned on. Any suggestions or comments on that front, before I run the more controlled benchmarks? -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
On 2013-07-10 12:21:23 -0700, Kevin Grittner wrote: Sorry for the delay in reviewing this. I must make sure never to take another vacation during a commitfest -- the backlog upon return is a killer Heh. Yes. Been through it before... Kevin Grittner kgri...@ymail.com wrote: Andres Freund and...@2ndquadrant.com wrote: Otherwise, could you try applying my git tree so we are sure we test the same thing? $ git remote add af git://git.postgresql.org/git/users/andresfreund/postgres.git $ git fetch af $ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4 $ ./configure ... $ make Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. The good news: the regression tests now work for me, and I'm back on testing this at a high level. The bad news: (1) The code checked out from that branch does not merge with master. Not surprisingly, given the recent commits, xlog.c is a problem. Is there another branch I should now be using? If not, please let me know when I can test with something that applies on top of the master branch. That one is actually relatively easy to resolve. The mvcc catalog scan patch is slightly harder. I've pushed an updated patch that fixes the latter in a slightly not-so-nice way. I am not sure yet how the final fix for that's going to look like, depends on whether we will get rid of SnapshotNow alltogether... I'll push my local tree with that fixed in a sec. (2) An initial performance test didn't look very good. I will be running a more controlled test to confirm but the logical replication of a benchmark with a lot of UPDATEs of compressed text values seemed to suffer with the logical replication turned on. Any suggestions or comments on that front, before I run the more controlled benchmarks? Hm. There theoretically shouldn't actually be anything added in that path. Could you roughly sketch what that test is doing? Do you actually stream those changes out or did you just turn on wal_level=logical? 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: Kevin Grittner kgri...@ymail.com wrote: (2) An initial performance test didn't look very good. I will be running a more controlled test to confirm but the logical replication of a benchmark with a lot of UPDATEs of compressed text values seemed to suffer with the logical replication turned on. Any suggestions or comments on that front, before I run the more controlled benchmarks? Hm. There theoretically shouldn't actually be anything added in that path. Could you roughly sketch what that test is doing? Do you actually stream those changes out or did you just turn on wal_level=logical? It was an update of a every row in a table of 72 rows, with each row updated by primary key using a separate UPDATE statement, modifying a large text column with a lot of repeating characters (so compressed well). I got a timing on a master build and I got a timing with the patch in the environment used by test_logical_decoding. It took several times as long in the latter run, but it was very much a preliminary test in preparation for getting real numbers. (I'm sure you know how much work it is to set up for a good run of tests.) I'm not sure that (for example) the synchronous_commit setting was the same, which could matter a lot. I wouldn't put a lot of stock in it until I can re-create it under a much more controlled test. The one thing about the whole episode that gave me pause was that the compression and decompression routines were very high on the `perf top` output in the patched run and way down the list on the run based on master. I don't have a ready explanation for that, unless your branch was missing a recent commit for speeding compression which was present on master. It might be worth checking that you're not detoasting more often than you need. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
On 2013-07-10 15:14:58 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: Kevin Grittner kgri...@ymail.com wrote: (2) An initial performance test didn't look very good. I will be running a more controlled test to confirm but the logical replication of a benchmark with a lot of UPDATEs of compressed text values seemed to suffer with the logical replication turned on. Any suggestions or comments on that front, before I run the more controlled benchmarks? Hm. There theoretically shouldn't actually be anything added in that path. Could you roughly sketch what that test is doing? Do you actually stream those changes out or did you just turn on wal_level=logical? It was an update of a every row in a table of 72 rows, with each row updated by primary key using a separate UPDATE statement, modifying a large text column with a lot of repeating characters (so compressed well). I got a timing on a master build and I got a timing with the patch in the environment used by test_logical_decoding. It took several times as long in the latter run, but it was very much a preliminary test in preparation for getting real numbers. (I'm sure you know how much work it is to set up for a good run of tests.) I'm not sure that (for example) the synchronous_commit setting was the same, which could matter a lot. I wouldn't put a lot of stock in it until I can re-create it under a much more controlled test. So you didn't explicitly start anything to consume those changes? I.e. using pg_receivellog or SELECT * FROM start/init_logical_replication(...)? Any chance there still was an old replication slot around? SELECT * FROM pg_stat_logical_decoding; should show them. But theoretically the make check in test_logical_decoding should finish without one active... The one thing about the whole episode that gave me pause was that the compression and decompression routines were very high on the `perf top` output in the patched run and way down the list on the run based on master. That's interesting. Unless there's something consuming the changestream and the output plugin does something that actually requests decompression of the Datums there shouldn't be *any* added/removed calls to toast (de-)compression... While consuming the changes there could be ReorderBufferToast* calls in the profile. I haven't yet seem them in profiles, but that's not saying all that much. So: I don't have a ready explanation for that, unless your branch was missing a recent commit for speeding compression which was present on master. It didn't have 031cc55bbea6b3a6b67c700498a78fb1d4399476 - but I can't really imagine that making *such* a big difference. But maybe you hit some sweet spot with the data? 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: Any chance there still was an old replication slot around? It is quite likely that there was. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
On 2013-06-28 21:47:47 +0200, Andres Freund wrote:
So, from what I gather there's a slight leaning towards *not* storing
the relation's oid in the WAL. Which means the concerns about the
uniqueness issues with the syscaches need to be addressed. So far I know
of three solutions:
1) develop a custom caching/mapping module
2) Make sure InvalidOid's (the only possible duplicate) can't end up the
syscache by adding a hook that prevents that on the catcache level
3) Make sure that there can't be any duplicates by storing the oid of
the relation in a mapped relations relfilenode
So, here's 4 patches:
1) add RelationMapFilenodeToOid()
2) Add pg_class index on (reltablespace, relfilenode)
3a) Add custom cache that maps from filenode to oid
3b) Add catcache 'filter' that ensures the cache stays unique and use
that for the mapping
4) Add pg_relation_by_filenode() and use it in a regression test
3b) adds an optional 'filter' attribute to struct cachedesc in
syscache.c which is then passed to catcache.c. If it's existant
catcache.c uses it - after checking for a match in the cache - to
check whether the queried-for value possibly should end up in the
cache. If not it stores a whiteout entry as currently already done
for nonexistant entries.
It also reorders some catcache.h struct attributes to make sure
we're not growing them. Might make sense to apply that
independently, those are rather heavily used.
I slightly prefer 3b) because it's smaller, what's your opinions?
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From cbedebac6a8d449a5127befe1525230c2132e06f Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Tue, 11 Jun 2013 23:25:26 +0200
Subject: [PATCH] wal_decoding: Add RelationMapFilenodeToOid function to
relmapper.c
This function maps (reltablespace, relfilenode) to the table oid and thus acts
as a reverse of RelationMapOidToFilenode.
---
src/backend/utils/cache/relmapper.c | 53 +
src/include/utils/relmapper.h | 2 ++
2 files changed, 55 insertions(+)
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 2c7d9f3..039aa29 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -180,6 +180,59 @@ RelationMapOidToFilenode(Oid relationId, bool shared)
return InvalidOid;
}
+/* RelationMapFilenodeToOid
+ *
+ * Do the reverse of the normal direction of mapping done in
+ * RelationMapOidToFilenode.
+ *
+ * This is not supposed to be used during normal running but rather for
+ * information purposes when looking at the filesystem or the xlog.
+ *
+ * Returns InvalidOid if the OID is not know which can easily happen if the
+ * filenode is not of a relation that is nailed or shared or if it simply
+ * doesn't exists anywhere.
+ */
+Oid
+RelationMapFilenodeToOid(Oid filenode, bool shared)
+{
+ const RelMapFile *map;
+ int32 i;
+
+ /* If there are active updates, believe those over the main maps */
+ if (shared)
+ {
+ map = active_shared_updates;
+ for (i = 0; i map-num_mappings; i++)
+ {
+ if (filenode == map-mappings[i].mapfilenode)
+return map-mappings[i].mapoid;
+ }
+ map = shared_map;
+ for (i = 0; i map-num_mappings; i++)
+ {
+ if (filenode == map-mappings[i].mapfilenode)
+return map-mappings[i].mapoid;
+ }
+ }
+ else
+ {
+ map = active_local_updates;
+ for (i = 0; i map-num_mappings; i++)
+ {
+ if (filenode == map-mappings[i].mapfilenode)
+return map-mappings[i].mapoid;
+ }
+ map = local_map;
+ for (i = 0; i map-num_mappings; i++)
+ {
+ if (filenode == map-mappings[i].mapfilenode)
+return map-mappings[i].mapoid;
+ }
+ }
+
+ return InvalidOid;
+}
+
/*
* RelationMapUpdateMap
*
diff --git a/src/include/utils/relmapper.h b/src/include/utils/relmapper.h
index 8f0b438..071bc98 100644
--- a/src/include/utils/relmapper.h
+++ b/src/include/utils/relmapper.h
@@ -36,6 +36,8 @@ typedef struct xl_relmap_update
extern Oid RelationMapOidToFilenode(Oid relationId, bool shared);
+extern Oid RelationMapFilenodeToOid(Oid relationId, bool shared);
+
extern void RelationMapUpdateMap(Oid relationId, Oid fileNode, bool shared,
bool immediate);
--
1.8.3.251.g1462b67
From fc6022fcc9ba8394069870b0b2b0e32a4a648c70 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Sun, 7 Jul 2013 18:38:56 +0200
Subject: [PATCH] Add index on pg_class(reltablespace, relfilenode)
Used by RelidByRelfilenode either via relfilenodemap.c or via a special
syscache.
Needs a CATVERSION bump.
---
src/include/catalog/indexing.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/include/catalog/indexing.h b/src/include/catalog/indexing.h
index 19268fb..4860e98 100644
--- a/src/include/catalog/indexing.h
+++ b/src/include/catalog/indexing.h
@@ -106,6 +106,8 @@
Re: [HACKERS] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com writes: 3b) Add catcache 'filter' that ensures the cache stays unique and use that for the mapping I slightly prefer 3b) because it's smaller, what's your opinions? This is just another variation on the theme of kluging the catcache to do something it shouldn't. You're still building a catcache on a non-unique index, and that is going to lead to trouble. (I'm a bit surprised that there is no Assert in catcache.c checking that the index nominated to support a catcache is unique ...) 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] changeset generation v5-01 - Patches git tree
On 2013-07-07 15:43:17 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: 3b) Add catcache 'filter' that ensures the cache stays unique and use that for the mapping I slightly prefer 3b) because it's smaller, what's your opinions? This is just another variation on the theme of kluging the catcache to do something it shouldn't. You're still building a catcache on a non-unique index, and that is going to lead to trouble. I don't think the lurking dangers really are present. The index essentially *is* unique since we filter away anything non-unique. The catcache code hardly can be confused by tuples it never sees. That would even work if we started preloading catcaches by doing scans of the entire underlying relation or by caching all of a page when reading one of its tuples. I can definitely see that there are aesthetical reasons against doing 3b), that's why I've also done 3a). So I'll chalk you up to voting for that... 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] changeset generation v5-01 - Patches git tree
On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: Hm. There were some issues with the test_logical_decoding Makefile not cleaning up the regression installation properly. Which might have caused the issue. Could you try after applying the patches and executing a clean and then rebuild? Tried, and problem persists. Otherwise, could you try applying my git tree so we are sure we test the same thing? $ git remote add af git://git.postgresql.org/git/users/andresfreund/postgres.git $ git fetch af $ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4 $ ./configure ... $ make Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. Ok. I think I have a slight idea what's going on. Could you check whether recompiling with -O0 fixes the issue? There's something strange going on here, not sure whether it's just a bug that's hidden, by either not doing optimizations or by adding more elog()s, or wheter it's a compiler bug. 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] changeset generation v5-01 - Patches git tree
On 2013-07-05 14:03:56 +0200, Andres Freund wrote:
On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote:
Andres Freund and...@2ndquadrant.com wrote:
Hm. There were some issues with the test_logical_decoding
Makefile not cleaning up the regression installation properly.
Which might have caused the issue.
Could you try after applying the patches and executing a clean
and then rebuild?
Tried, and problem persists.
Otherwise, could you try applying my git tree so we are sure we
test the same thing?
$ git remote add af
git://git.postgresql.org/git/users/andresfreund/postgres.git
$ git fetch af
$ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4
$ ./configure ...
$ make
Tried that, too, and problem persists. The log shows the last
commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb.
Ok. I think I have a slight idea what's going on. Could you check
whether recompiling with -O0 fixes the issue?
There's something strange going on here, not sure whether it's just a
bug that's hidden, by either not doing optimizations or by adding more
elog()s, or wheter it's a compiler bug.
Ok. It was supreme stupidity on my end. Sorry for the time you spent on
it.
Some versions of gcc (and probably other compilers) were removing
sections of code when optimizing because the code was doing undefined
things. Parts of the rdata chain were allocated locally in an
if (needs_key). Which obviously is utterly bogus... A warning would have
been nice though.
Fix pushed and attached.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
From ddbaa1dbf8e0283b41098f5a08a8d21d809b9a63 Mon Sep 17 00:00:00 2001
From: Andres Freund and...@anarazel.de
Date: Fri, 5 Jul 2013 15:07:19 +0200
Subject: [PATCH] wal_decoding: mergme: Don't use out-of-scope local variables
as part of the rdata chain
Depending on optimization level and other configuration flags removed the
sections of code doing that sinced doing so invokes undefined behaviour making
it legal for the compiler to do so.
---
src/backend/access/heap/heapam.c | 37 ++---
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index f51b73f..f9f1705 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5987,9 +5987,10 @@ log_heap_update(Relation reln, Buffer oldbuf,
{
xl_heap_update xlrec;
xl_heap_header_len xlhdr;
+ xl_heap_header_len xlhdr_idx;
uint8 info;
XLogRecPtr recptr;
- XLogRecData rdata[4];
+ XLogRecData rdata[7];
Page page = BufferGetPage(newbuf);
/*
@@ -6054,40 +6055,38 @@ log_heap_update(Relation reln, Buffer oldbuf,
*/
if(need_tuple_data)
{
- XLogRecData rdata_logical[4];
-
- rdata[3].next = (rdata_logical[0]);
+ rdata[3].next = (rdata[4]);
- rdata_logical[0].data = NULL,
- rdata_logical[0].len = 0;
- rdata_logical[0].buffer = newbuf;
- rdata_logical[0].buffer_std = true;
- rdata_logical[0].next = NULL;
+ rdata[4].data = NULL,
+ rdata[4].len = 0;
+ rdata[4].buffer = newbuf;
+ rdata[4].buffer_std = true;
+ rdata[4].next = NULL;
xlrec.flags |= XLOG_HEAP_CONTAINS_NEW_TUPLE;
/* candidate key changed and we have a candidate key */
if (idx_tuple)
{
/* don't really need this, but its more comfy */
- xl_heap_header_len xlhdr_idx;
xlhdr_idx.header.t_infomask2 = idx_tuple-t_data-t_infomask2;
xlhdr_idx.header.t_infomask = idx_tuple-t_data-t_infomask;
xlhdr_idx.header.t_hoff = idx_tuple-t_data-t_hoff;
xlhdr_idx.t_len = idx_tuple-t_len;
- rdata_logical[0].next = (rdata_logical[1]);
- rdata_logical[1].data = (char *) xlhdr_idx;
- rdata_logical[1].len = SizeOfHeapHeaderLen;
- rdata_logical[1].buffer = InvalidBuffer;
- rdata_logical[1].next = (rdata_logical[2]);
+ rdata[4].next = (rdata[5]);
+ rdata[5].data = (char *) xlhdr_idx;
+ rdata[5].len = SizeOfHeapHeaderLen;
+ rdata[5].buffer = InvalidBuffer;
+ rdata[5].next = (rdata[6]);
/* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
- rdata_logical[2].data = (char *) idx_tuple-t_data
+ rdata[6].data = (char *) idx_tuple-t_data
+ offsetof(HeapTupleHeaderData, t_bits);
- rdata_logical[2].len = idx_tuple-t_len
+ rdata[6].len = idx_tuple-t_len
- offsetof(HeapTupleHeaderData, t_bits);
- rdata_logical[2].buffer = InvalidBuffer;
- rdata_logical[2].next = NULL;
+ rdata[6].buffer = InvalidBuffer;
+ rdata[6].next = NULL;
+
xlrec.flags |= XLOG_HEAP_CONTAINS_OLD_KEY;
}
}
--
1.8.2.rc2.4.g7799588.dirty
--
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] changeset generation v5-01 - Patches git tree
On 07/05/2013 08:03 AM, Andres Freund wrote: On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. Ok. I think I have a slight idea what's going on. Could you check whether recompiling with -O0 fixes the issue? There's something strange going on here, not sure whether it's just a bug that's hidden, by either not doing optimizations or by adding more elog()s, or wheter it's a compiler bug. I am getting the same test failure Kevin is seeing. This is on a x64 Debian wheezy machine with gcc (Debian 4.7.2-5) 4.7.2 Building with -O0 results in passing tests. Greetings, Andres Freund -- 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] changeset generation v5-01 - Patches git tree
On 2013-07-05 09:28:45 -0400, Steve Singer wrote: On 07/05/2013 08:03 AM, Andres Freund wrote: On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. Ok. I think I have a slight idea what's going on. Could you check whether recompiling with -O0 fixes the issue? There's something strange going on here, not sure whether it's just a bug that's hidden, by either not doing optimizations or by adding more elog()s, or wheter it's a compiler bug. I am getting the same test failure Kevin is seeing. This is on a x64 Debian wheezy machine with gcc (Debian 4.7.2-5) 4.7.2 Building with -O0 results in passing tests. Does the patch from http://archives.postgresql.org/message-id/20130705132513.GB11640%40awork2.anarazel.de or the git tree (which is rebased ontop of the mvcc catalog commit from robert which needs some changes) fix it, even with optimizations? 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] changeset generation v5-01 - Patches git tree
On 07/05/2013 09:34 AM, Andres Freund wrote: On 2013-07-05 09:28:45 -0400, Steve Singer wrote: On 07/05/2013 08:03 AM, Andres Freund wrote: On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. Ok. I think I have a slight idea what's going on. Could you check whether recompiling with -O0 fixes the issue? There's something strange going on here, not sure whether it's just a bug that's hidden, by either not doing optimizations or by adding more elog()s, or wheter it's a compiler bug. I am getting the same test failure Kevin is seeing. This is on a x64 Debian wheezy machine with gcc (Debian 4.7.2-5) 4.7.2 Building with -O0 results in passing tests. Does the patch from http://archives.postgresql.org/message-id/20130705132513.GB11640%40awork2.anarazel.de or the git tree (which is rebased ontop of the mvcc catalog commit from robert which needs some changes) fix it, even with optimizations? Yes your latest git tree the tests pass with -O2 Greetings, Andres Freund -- 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] changeset generation v5-01 - Patches git tree
On 06/14/2013 06:51 PM, Andres Freund wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 We discussed issues related to passing options to the plugins a number of months ago ( http://www.postgresql.org/message-id/20130129015732.ga24...@awork2.anarazel.de) I'm still having issues with the syntax you describe there. START_LOGICAL_REPLICATION 1 0/0 (foo,bar) unexpected termination of replication stream: ERROR: foo requires a parameter START_LOGICAL_REPLICATION 1 0/0 (foo bar) START_LOGICAL_REPLICATION 1 0/0 (foo bar): ERROR: syntax error START_LOGICAL_REPLICATION 1 0/0 (foo) works okay Steve On 2013-06-15 00:48:17 +0200, Andres Freund wrote: Overview of the attached patches: 0001: indirect toast tuples; required but submitted independently 0002: functions for testing; not required, 0003: (tablespace, filenode) syscache; required 0004: RelationMapFilenodeToOid: required, simple 0005: pg_relation_by_filenode() function; not required but useful 0006: Introduce InvalidCommandId: required, simple 0007: Adjust Satisfies* interface: required, mechanical, 0008: Allow walsender to attach to a database: required, needs review 0009: New GetOldestXmin() parameter; required, pretty boring 0010: Log xl_running_xact regularly in the bgwriter: required 0011: make fsync_fname() public; required, needs to be in a different file 0012: Relcache support for an Relation's primary key: required 0013: Actual changeset extraction; required 0014: Output plugin demo; not required (except for testing) but useful 0015: Add pg_receivellog program: not required but useful 0016: Add test_logical_decoding extension; not required, but contains the tests for the feature. Uses 0014 0017: Snapshot building docs; not required Version v5-01 attached Greetings, Andres Freund -- 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] changeset generation v5-01 - Patches git tree
On 2013-07-05 11:33:20 -0400, Steve Singer wrote:
On 06/14/2013 06:51 PM, Andres Freund wrote:
The git tree is at:
git://git.postgresql.org/git/users/andresfreund/postgres.git branch
xlog-decoding-rebasing-cf4
http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4
We discussed issues related to passing options to the plugins a number of
months ago (
http://www.postgresql.org/message-id/20130129015732.ga24...@awork2.anarazel.de)
I'm still having issues with the syntax you describe there.
START_LOGICAL_REPLICATION 1 0/0 (foo,bar)
unexpected termination of replication stream: ERROR: foo requires a
parameter
I'd guess that's coming from your output plugin? You're using
defGetString() on DefElem without a value?
START_LOGICAL_REPLICATION 1 0/0 (foo bar)
Yes, the option *names* are identifiers, together with plugin slot
names. The passed values need to be SCONSTs atm
(src/backend/replication/repl_gram.y):
plugin_opt_elem:
IDENT plugin_opt_arg
{
$$ = makeDefElem($1, $2);
}
;
plugin_opt_arg:
SCONST
{ $$ = (Node *) makeString($1); }
| /* EMPTY */ { $$ =
NULL; }
;
So, it would have to be:
START_LOGICAL_REPLICATION 1 0/0 (foo 'bar blub frob', sup 'star', noarg)
Now that's not completely obvious, I admit :/. Better suggestions?
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] changeset generation v5-01 - Patches git tree
Since this discussion seems to have stalled, let me do a quick summary. The goal of this subset of patches is to allow retroactive look up of relations starting from a WAL record. Currently, the WAL record only tracks the relfilenode that it affects, so there are two possibilities: 1. we add some way to find out the relation OID from the relfilenode, 2. we augment the WAL record with the relation OID. Each solution has its drawbacks. For the former, * we need a new cache * we need a new pg_class index * looking up the relation OID still requires some CPU runtime and memory to keep the caches in; run invalidations, etc. For the latter, * each WAL record would become somewhat bigger. For WAL records with a payload of 25 bytes (say insert a tuple which is 25 bytes long) this means about 7% overhead. There are some other issues, but these can be solved. For instance Tom doesn't want a syscache on top of a non-unique index, and I agree on that. But if we agree on this way forward, we can just go a different route by keeping a separate cache layer. So the question is, do we take the overhead of the new index (which means overhead on DML operations -- supposedly rare) or do we take the overhead of larger WAL records (which means overhead on all DDL operations)? Note we can make either thing apply to only people running logical replication. -- Á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] changeset generation v5-01 - Patches git tree
Alvaro Herrera alvhe...@2ndquadrant.com writes: So the question is, do we take the overhead of the new index (which means overhead on DML operations -- supposedly rare) or do we take the overhead of larger WAL records (which means overhead on all DDL operations)? Note we can make either thing apply to only people running logical replication. I don't believe you can have or not have an index on pg_class as easily as all that. The choice would have to be frozen at initdb time, so people would have to pay the overhead if they thought there was even a small possibility that they'd want logical replication later. Flipping the content of WAL records might not be a terribly simple thing to do either, but at least in principle it could be done during a postmaster restart, without initdb. 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] changeset generation v5-01 - Patches git tree
On 2013-07-01 14:16:55 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: So the question is, do we take the overhead of the new index (which means overhead on DML operations -- supposedly rare) or do we take the overhead of larger WAL records (which means overhead on all DDL operations)? Note we can make either thing apply to only people running logical replication. I don't believe you can have or not have an index on pg_class as easily as all that. The choice would have to be frozen at initdb time, so people would have to pay the overhead if they thought there was even a small possibility that they'd want logical replication later. It should be possible to create the index in a single database when we start logical replication in that database? Running the index creation with a fixed oid shouldn't require too much code. The oid won't be reused by other pg_class entries since it would be a system one. Alternatively we could always create the index's pg_class/index entry but mark it as !indislive when logical replication isn't active for that database. Then activating it would just require rebuilding that index. But then, I am not fully convinced that's worth the trouble since I don't think pg_class index maintenance is the painspot in DDL atm. Flipping the content of WAL records might not be a terribly simple thing to do either, but at least in principle it could be done during a postmaster restart, without initdb. The main patch combines various booleans in the heap wal records into a flags variable, so there should be enough space to keep track of it without increasing size. Makes size calculations a bit more annoying though as we use the xlog record length to calculate the heap tuple's length, but that's not a large problem. So we could just set the XLOG_HEAP_CONTAINS_CLASSOID flag if wal_level = WAL_LEVEL_LOGICAL. Wal decoding then can throw a tantrum if it finds a record without it and we're done. We could even make that per database, but that seems to be something for the future. 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] changeset generation v5-01 - Patches git tree
On 27 June 2013 23:18, Tom Lane t...@sss.pgh.pa.us wrote: Exactly what is the argument that says performance of this function is sufficiently critical to justify adding both the maintenance overhead of a new pg_class index, *and* a broken-by-design syscache? I think we all agree on changing the syscache. I'm not clear why adding a new permanent index to pg_class is such a problem. It's going to be a very thin index. I'm trying to imagine a use case that has pg_class index maintenance as a major part of its workload and I can't. An extra index on pg_attribute and I might agree with you. The pg_class index would only be a noticeable % of catalog rows for very thin temp tables, but would still even then be small; that isn't even necessary work since we all agree that temp table overheads could and should be optimised away somwhere. So blocking a new index because of that sounds strange. What issues do you foresee? How can we test them? Or perhaps we should just add the index and see if we later discover a measurable problem workload? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] changeset generation v5-01 - Patches git tree
On 2013-06-27 18:18:50 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm looking at the combined patches 0003-0005, which are essentially all about adding a function to obtain relation OID from (tablespace, filenode). It takes care to look through the relation mapper, and uses a new syscache underneath for performance. One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. ... which, I assume, is on top of a pg_class index that doesn't exist today. Exactly what is the argument that says performance of this function is sufficiently critical to justify adding both the maintenance overhead of a new pg_class index, *and* a broken-by-design syscache? Ok, so this requires some context. When we do the changeset extraction we build a mvcc snapshot that for every heap wal record is consistent with one made at the time the record has been inserted. Then, when we've built that snapshot, we can use it to turn heap wal records into the representation the user wants: For that we first need to know which table a change comes from, since otherwise we obviously cannot interpret the HeapTuple that's essentially contained in the wal record without it. Since we have a correct mvcc snapshot we can query pg_class for (tablespace, relfilenode) to get back the relation. When we know the relation, the user (i.e. the output pluggin) can use normal backend code to transform the HeapTuple into the target representation, e.g. SQL, since we can build a TupleDesc. Since the syscaches are synchronized with the built snapshot normal output functions can be used. What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. 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] changeset generation v5-01 - Patches git tree
On Fri, Jun 28, 2013 at 3:32 AM, Andres Freund and...@2ndquadrant.com wrote: What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. -- 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] changeset generation v5-01 - Patches git tree
On 2013-06-28 08:41:46 -0400, Robert Haas wrote: On Fri, Jun 28, 2013 at 3:32 AM, Andres Freund and...@2ndquadrant.com wrote: What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. I don't think index maintenance itself comes close to the biggest cost for DDL we have atm. It also increases the modifications needed to imporantant heap_* functions which doesn't make me happy. How do others see this tradeoff? 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] changeset generation v5-01 - Patches git tree
On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. I get the same failure, with primary key or unique index column showing as 0 in results. I have run enough iterations of the test suite locally now that I am confident it's not just happenstance that I don't see this :/. I am going to clone your environment as closely as I can to see where the issue might be as well as going over those codepaths... 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] changeset generation v5-01 - Patches git tree
On 6/28/13 8:46 AM, Andres Freund wrote: I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. I don't think index maintenance itself comes close to the biggest cost for DDL we have atm. That makes sense to me in principle. -- 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com writes: On 2013-06-28 08:41:46 -0400, Robert Haas wrote: The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. This argument is nonsense, since it conveniently ignores the added WAL entries created as a result of additional pg_class index manipulations. Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, and it would probably ease many debugging tasks as well as what you want to do. So I'd vote for including the rel OID all the time, not conditionally. The real performance argument against the patch as you have it is that it saddles every PG installation with extra overhead for pg_class updates whether or not that installation ever has or ever will make use of changeset generation --- unlike including rel OIDs in WAL entries, which might be merely difficult to handle conditionally, it's flat-out impossible to turn such an index on or off. Moreover, even if one is using changeset generation, the overhead is being imposed at the wrong place, ie the master not the slave doing changeset extraction. But that's not the only problem, nor even the worst one IMO. I said before that a syscache with a non-unique key is broken by design, and I stand by that estimate. Even assuming that this usage doesn't create bugs in the code as it stands, it might well foreclose future changes or optimizations that we'd like to make in the catcache code. If you don't want to change WAL contents, what I think you should do is create a new cache mechanism (perhaps by extending the relmapper) that caches relfilenode to OID lookups and acts entirely inside the changeset-generating slave. Hacking up the catcache instead of doing that is an expedient kluge that will come back to bite us. 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] changeset generation v5-01 - Patches git tree
On 2013-06-28 10:49:26 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-06-28 08:41:46 -0400, Robert Haas wrote: The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. This argument is nonsense, since it conveniently ignores the added WAL entries created as a result of additional pg_class index manipulations. Huh? Sure, pg_class manipulations get more expensive. But in most clusters pg_class modifications are by far the minority compared to the rest of the updates performed. Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, and it would probably ease many debugging tasks as well as what you want to do. So I'd vote for including the rel OID all the time, not conditionally. Ok, I can sure live with that. I don't think it's a problem to make it conditionally if we want to. Making it unconditional would sure make WAL debugging in general more pleasant though. The real performance argument against the patch as you have it is that it saddles every PG installation with extra overhead for pg_class updates whether or not that installation ever has or ever will make use of changeset generation --- unlike including rel OIDs in WAL entries, which might be merely difficult to handle conditionally, it's flat-out impossible to turn such an index on or off. Moreover, even if one is using changeset generation, the overhead is being imposed at the wrong place, ie the master not the slave doing changeset extraction. There's no required slaves for doing changeset extraction anymore. Various people opposed that pretty violently, so it's now all happening on the master. Which IMHO turned out to be the right decision. We can do it on Hot Standby nodes, but its absolutely not required. But that's not the only problem, nor even the worst one IMO. I said before that a syscache with a non-unique key is broken by design, and I stand by that estimate. Even assuming that this usage doesn't create bugs in the code as it stands, it might well foreclose future changes or optimizations that we'd like to make in the catcache code. Since the only duplicate key that possibly can occur in that cache is InvalidOid, I wondered whether we could define a 'filter' that prohibits those ending up in the cache? Then the cache would be unique. 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] changeset generation v5-01 - Patches git tree
On Fri, Jun 28, 2013 at 10:49 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, ... How big a deal is it? This is a serious question, because I don't know. Let's suppose that the average size of an XLOG_HEAP_INSERT record is 100 bytes. Then if we add 4 bytes, isn't that a 4% overhead? And doesn't that seem significant? I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. -- 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] changeset generation v5-01 - Patches git tree
Robert Haas escribió:
On Fri, Jun 28, 2013 at 10:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert's idea sounds fairly reasonable to me; another 4 bytes per
insert/update/delete WAL entry isn't that big a deal, ...
How big a deal is it? This is a serious question, because I don't
know. Let's suppose that the average size of an XLOG_HEAP_INSERT
record is 100 bytes. Then if we add 4 bytes, isn't that a 4%
overhead? And doesn't that seem significant?
An INSERT wal record is:
typedef struct xl_heap_insert
{
xl_heaptid target; /* inserted tuple id */
boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
/* xl_heap_header TUPLE DATA FOLLOWS AT END OF STRUCT */
} xl_heap_insert;
typedef struct xl_heap_header
{
uint16 t_infomask2;
uint16 t_infomask;
uint8 t_hoff;
} xl_heap_header;
So the fixed part is just 7 bytes + 5 bytes; tuple data follows that.
So adding four more bytes could indeed be significant (but by how much,
depends on the size of the tuple data). Adding a new pg_class index
would be larger in the sense that there are more WAL records, and
there's the extra vacuuming traffic; but on the other hand that would
only happen when tables are created. It seems safe to assume that in
normal use cases the ratio of tuple insertion vs. table creation is
large.
The only idea that springs to mind is to have the new pg_class index be
created conditionally, i.e. only when logical replication is going to be
used.
--
Á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] changeset generation v5-01 - Patches git tree
Alvaro Herrera escribió:
An INSERT wal record is:
typedef struct xl_heap_insert
{
xl_heaptid target; /* inserted tuple id */
boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
/* xl_heap_header TUPLE DATA FOLLOWS AT END OF STRUCT */
} xl_heap_insert;
Oops. xl_heaptid is not 6 bytes, but instead:
typedef struct xl_heaptid
{
RelFileNode node;
ItemPointerData tid;
} xl_heaptid;
typedef struct RelFileNode
{
Oid spcNode;
Oid dbNode;
Oid relNode;
} RelFileNode; /* 12 bytes */
typedef struct ItemPointerData
{
BlockIdData ip_blkid;
OffsetNumber ip_posid;
}; /* 6 bytes */
typedef struct BlockIdData
{
uint16 bi_hi;
uint16 bi_lo;
} BlockIdData; /* 4 bytes */
typedef uint16 OffsetNumber;
There's purposely no alignment padding anywhere, so xl_heaptid totals 22 bytes.
Therefore,
So the fixed part is just 22 bytes + 5 bytes; tuple data follows that.
So adding four more bytes could indeed be significant (but by how much,
depends on the size of the tuple data).
4 extra bytes on top of 27 is 14% of added overhead (considering only
the xlog header.)
--
Á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] changeset generation v5-01 - Patches git tree
Robert Haas robertmh...@gmail.com writes: I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. Certainly, any of these solutions are going to cost us somewhere --- either up-front cost or more expensive (and less reliable?) changeset extraction, take your choice. I will note that somehow tablespaces got put in despite having to add 4 bytes to every WAL record for that feature, which was probably of less use than logical changeset extraction will be. But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* 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] changeset generation v5-01 - Patches git tree
On Fri, Jun 28, 2013 at 11:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. Certainly, any of these solutions are going to cost us somewhere --- either up-front cost or more expensive (and less reliable?) changeset extraction, take your choice. I will note that somehow tablespaces got put in despite having to add 4 bytes to every WAL record for that feature, which was probably of less use than logical changeset extraction will be. Right. I actually think we booted that one. The database ID is a constant for most people. The tablespace ID is not technically redundant, but in 99.99% of cases you could figure it out from the database ID + relation ID. The relation ID is where 99% of the entropy is, but it probably only has 8-16 bits of entropy in most real-world use cases. If we were doing this over we might want to think about storing a proxy for the relfilenode rather than the relfilenode itself, but there's not much good crying over it now. But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* idea. +1. I don't think the extra index on pg_class is going to hurt that much, even if we create it always, as long as we use a purpose-built caching mechanism for it rather than forcing it through catcache. The people who are going to suffer are the ones who create and drop a lot of temporary tables, but even there I'm not sure how visible the overhead will be on real-world workloads, and maybe the solution is to work towards not having permanent catalog entries for temporary tables in the first place. In any case, hurting people who use temporary tables heavily seems better than adding overhead to every insert/update/delete operation, which will hit all users who are not read-only. On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. -- 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] changeset generation v5-01 - Patches git tree
Robert Haas robertmh...@gmail.com writes: On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. That feeling has been nagging at me too. I can't demonstrate that there's a problem when an ALTER TABLE is in process of rewriting a table into a new relfilenode number, but I don't have a warm fuzzy feeling about the reliability of reverse lookups for this. At the very least it's going to require some hard-to-verify restriction about how we can't start doing changeset reconstruction in the middle of a transaction that's doing DDL. 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] changeset generation v5-01 - Patches git tree
On 28 June 2013 17:10, Robert Haas robertmh...@gmail.com wrote: But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* idea. +1. I don't think the extra index on pg_class is going to hurt that much, even if we create it always, as long as we use a purpose-built caching mechanism for it rather than forcing it through catcache. Hmm, does seem like that would be better. The people who are going to suffer are the ones who create and drop a lot of temporary tables, but even there I'm not sure how visible the overhead will be on real-world workloads, and maybe the solution is to work towards not having permanent catalog entries for temporary tables in the first place. In any case, hurting people who use temporary tables heavily seems better than adding overhead to every insert/update/delete operation, which will hit all users who are not read-only. Thinks... If we added a trigger that fired a NOTIFY for any new rows in pg_class that relate to non-temporary relations that would optimise away any overhead for temporary tables or when no changeset extraction was in progress. The changeset extraction could build a private hash table to perform the lookup and then LISTEN on a specific channel for changes. That might work better than an index-plus-syscache. On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. I don't really like the idea of requiring the relid on the WAL record. WAL is big enough already and we want people to turn this on, not avoid it. This is just an index lookup. We do them all the time without any fear of reliability issues. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] changeset generation v5-01 - Patches git tree
On 2013-06-28 12:26:52 -0400, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. That feeling has been nagging at me too. I can't demonstrate that there's a problem when an ALTER TABLE is in process of rewriting a table into a new relfilenode number, but I don't have a warm fuzzy feeling about the reliability of reverse lookups for this. I am pretty sure the mapping thing works, but it indeed requires some complexity. And it's harder to debug because when you want to understand what's going on the relfilenodes involved aren't in the catalog anymore. At the very least it's going to require some hard-to-verify restriction about how we can't start doing changeset reconstruction in the middle of a transaction that's doing DDL. Currently changeset extraction needs to wait (and does so) till it found a point where it has seen the start of all in-progress transactions. All transaction that *commit* after the last partiall observed in-progress transaction finished can be decoded. To make that point visible for external tools to synchronize - e.g. pg_dump - it exports the snapshot of exactly the moment when that last in-progress transaction committed. So, from what I gather there's a slight leaning towards *not* storing the relation's oid in the WAL. Which means the concerns about the uniqueness issues with the syscaches need to be addressed. So far I know of three solutions: 1) develop a custom caching/mapping module 2) Make sure InvalidOid's (the only possible duplicate) can't end up the syscache by adding a hook that prevents that on the catcache level 3) Make sure that there can't be any duplicates by storing the oid of the relation in a mapped relations relfilenode Opinions? 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] changeset generation v5-01 - Patches git tree
I'm looking at the combined patches 0003-0005, which are essentially all about adding a function to obtain relation OID from (tablespace, filenode). It takes care to look through the relation mapper, and uses a new syscache underneath for performance. One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. It is said that this doesn't matter because the only non-unique values that can exist would reference entries that have relfilenode = 0; and in turn this doesn't matter because those values would be queried through the relation mapper anyway, not from the syscache. (This is implemented in the higher-level function.) This means that there would be one syscache that is damn easy to misuse .. and we've setup things so that syscaches are very easy to use in the first place. From that perspective, this doesn't look good. However, it's an easy mistake to notice and fix, so perhaps this is not a serious problem. (I would much prefer for there to be a way to define partial indexes in BKI.) I'm not sure about the placing of the new SQL-callable function in dbsize.c either. It is certainly not a function that has anything to do with object sizes. The insides of it would belong more in lsyscache.c, I think, except then that file does not otherwise concern itself with the relation mapper so its scope would have to expand a bit. But this is no place for the SQL-callable portion, so that would have to find a different home as well. The other option, of course, it to provide a separate caching layer for these objects altogether, but given how concise this implementation is, it doesn't sound too palatable. Thoughts? -- Á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] changeset generation v5-01 - Patches git tree
Hi, On 2013-06-27 17:33:04 -0400, Alvaro Herrera wrote: One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. It is said that this doesn't matter because the only non-unique values that can exist would reference entries that have relfilenode = 0; and in turn this doesn't matter because those values would be queried through the relation mapper anyway, not from the syscache. (This is implemented in the higher-level function.) Well, you can even query the syscache without hurt for mapped relations, you just won't get an answer. The only thing you may not do because it would yield multiple results is to query the syscache with (tablespace, InvalidOid/0). Which is still not nice although it doesn't make much sense to query with InvalidOid. I'm not sure about the placing of the new SQL-callable function in dbsize.c either. It is certainly not a function that has anything to do with object sizes. Not happy with that myself. I only placed the function there because pg_relation_filenode() already was in it. Happy to change if somebody has a good idea. (I would much prefer for there to be a way to define partial indexes in BKI.) I don't think that's the hard part, it's that we don't use the full machinery for updating indexes but rather the relatively simplistic CatalogUpdateIndexes(). I am not sure we can guarantee that the required infrastructure is available in all the cases to support doing generic predicate evaluation. Should bki really be the problem we probably could create the index after bki based bootstrapping finished. Thanks, 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] changeset generation v5-01 - Patches git tree
Andres Freund wrote: On 2013-06-27 17:33:04 -0400, Alvaro Herrera wrote: One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. It is said that this doesn't matter because the only non-unique values that can exist would reference entries that have relfilenode = 0; and in turn this doesn't matter because those values would be queried through the relation mapper anyway, not from the syscache. (This is implemented in the higher-level function.) Well, you can even query the syscache without hurt for mapped relations, you just won't get an answer. The only thing you may not do because it would yield multiple results is to query the syscache with (tablespace, InvalidOid/0). Which is still not nice although it doesn't make much sense to query with InvalidOid. Yeah, I agree that it doesn't make sense to query for that. The problem is that something could reasonably be developed that uses the syscache directly without checking whether the relfilenode is 0. (I would much prefer for there to be a way to define partial indexes in BKI.) I don't think that's the hard part, it's that we don't use the full machinery for updating indexes but rather the relatively simplistic CatalogUpdateIndexes(). I am not sure we can guarantee that the required infrastructure is available in all the cases to support doing generic predicate evaluation. You're right, CatalogIndexInsert() doesn't allow for predicates, so fixing BKI would not help. I still wonder about having a separate cache. Right now pg_class has two indexes; adding this new one would mean a rather large decrease in insert performance (50% more indexes to update than previously), which is not good considering that it's inserted into for each and every temp table creation -- that would become slower. This would be a net loss for every user, even those that don't want logical replication. On the other hand, table creation also has to add tuples to pg_attribute, pg_depend, pg_shdepend and maybe other catalogs, so perhaps the difference is negligible. -- Á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] changeset generation v5-01 - Patches git tree
Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm looking at the combined patches 0003-0005, which are essentially all about adding a function to obtain relation OID from (tablespace, filenode). It takes care to look through the relation mapper, and uses a new syscache underneath for performance. One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. ... which, I assume, is on top of a pg_class index that doesn't exist today. Exactly what is the argument that says performance of this function is sufficiently critical to justify adding both the maintenance overhead of a new pg_class index, *and* a broken-by-design syscache? Lose the cache and this probably gets a lot easier to justify. As is, I think I'd vote to reject altogether. 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] changeset generation v5-01 - Patches git tree
On Thu, Jun 27, 2013 at 6:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm looking at the combined patches 0003-0005, which are essentially all about adding a function to obtain relation OID from (tablespace, filenode). It takes care to look through the relation mapper, and uses a new syscache underneath for performance. One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. ... which, I assume, is on top of a pg_class index that doesn't exist today. Exactly what is the argument that says performance of this function is sufficiently critical to justify adding both the maintenance overhead of a new pg_class index, *and* a broken-by-design syscache? Lose the cache and this probably gets a lot easier to justify. As is, I think I'd vote to reject altogether. I already voted that way, and nothing's happened since to change my mind. -- 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: Hm. There were some issues with the test_logical_decoding Makefile not cleaning up the regression installation properly. Which might have caused the issue. Could you try after applying the patches and executing a clean and then rebuild? Tried, and problem persists. Otherwise, could you try applying my git tree so we are sure we test the same thing? $ git remote add af git://git.postgresql.org/git/users/andresfreund/postgres.git $ git fetch af $ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4 $ ./configure ... $ make Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. Because you mention possible problems with the regression test cleanup for test_logical_decoding I also tried: rm -fr contrib/test_logical_decoding/ git reset --hard HEAD make world make check-world I get the same failure, with primary key or unique index column showing as 0 in results. I am off on vacation tomorrow and next week. Will dig into this with gdb if not solved when I get back -- unless you have a better suggestion for how to figure it out. Once this is solved, I will be working with testing the final result of all these layers, including creating a second output plugin. I want to confirm that multiple plugins play well together. I'm glad to see other eyes also on this patch set. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world [ build failure referencing pg_receivellog.o ] I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. It works with this patch-on-patch: clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. Hm. Why not? Well, I could easily be wrong on just about anything to do with make files, but on a second look that appeared to be dealing with eliminating an installed pg_receivellog binary, which is not created. I don't think either hunk has anything to do with that buildfailure though - can you reproduce the error without? I tried that scenario three times and it failed three times. Then I made the above changes and it worked. Then I eliminated the one on the uninstall target and tried a couple more times and it worked on both attempts. The scenario is to have a `make world` build in the source tree, and run the above line starting with `make maintainer-clean` and going to `make -j4 world`. I did notice that without that change to the maintainer-clean target I did not get a pg_receivellog.Po file in src/bin/pg_basebackup/.deps/ -- and with it I do. I admit to being at about a 1.5 on a 10 point scale of make file competence -- I just look for patterns used for things similar to what I want to do and copy without much understanding of what it all means. :-( So when I got an error on pg_receivellog which didn't happen on pg_receivexlog, I looked for differences -- my suggestion has no more basis than that and the fact that empirical testing seemed to show that it worked. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
On 2013-06-24 06:44:53 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world [ build failure referencing pg_receivellog.o ] I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. It works with this patch-on-patch: clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. Hm. Why not? Well, I could easily be wrong on just about anything to do with make files, but on a second look that appeared to be dealing with eliminating an installed pg_receivellog binary, which is not created. I think it actually is? install: all installdirs $(INSTALL_PROGRAM) pg_basebackup$(X) '$(DESTDIR)$(bindir)/pg_basebackup$(X)' $(INSTALL_PROGRAM) pg_receivexlog$(X) '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' $(INSTALL_PROGRAM) pg_receivellog$(X) '$(DESTDIR)$(bindir)/pg_receivellog$(X)' I don't think either hunk has anything to do with that buildfailure though - can you reproduce the error without? I tried that scenario three times and it failed three times. Then I made the above changes and it worked. Then I eliminated the one on the uninstall target and tried a couple more times and it worked on both attempts. The scenario is to have a `make world` build in the source tree, and run the above line starting with `make maintainer-clean` and going to `make -j4 world`. Hm. I think it might be something in makes intermediate target logic biting us. Anyway, if the patch fixes that: Great ;). Merged it logally since it's obviously missing. I did notice that without that change to the maintainer-clean target I did not get a pg_receivellog.Po file in src/bin/pg_basebackup/.deps/ -- and with it I do. Yea, according to your log it's not even built before pg_receivellog is linked. Thanks, 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: On 2013-06-23 10:32:05 -0700, Kevin Grittner wrote: The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. Hm. Any chance this was an incomplete rebuild? With my hack on the pg_basebackup Makefile, `make -j4 world` is finishing with no errors and: PostgreSQL, contrib, and documentation successfully made. Ready to install. I seem to remember having seen that once because some header dependency wasn't recognized correctly after applying some patch. I wonder whether this is related to the build problems we've been discussing on the other fork of this thread. I was surprised to see this error when I got past the maintainer-clean full build problems, because I thought I had seen clean `make check-world` regression tests after applying each incremental patch file. Until I read this I had been assuming that somehow I missed the error on the 16th and 17th iterations; but now I'm suspecting that I didn't miss anything after all -- it may just be another symptom of the build problems. Otherwise, could you give me: * the version you aplied the patch on 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 * os/compiler Linux Kevin-Desktop 3.5.0-34-generic #55-Ubuntu SMP Thu Jun 6 20:18:19 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux gcc (Ubuntu/Linaro 4.7.2-2ubuntu1) 4.7.2 Because I can't reproduce it, despite some playing around... Maybe if you can reproduce the build problems I'm seeing -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: On 2013-06-24 06:44:53 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. Hm. Why not? Well, I could easily be wrong on just about anything to do with make files, but on a second look that appeared to be dealing with eliminating an installed pg_receivellog binary, which is not created. I think it actually is? Oh, yeah I see it now. I warned you I could be wrong. :-/ I just had a thought thought -- perhaps the dependency information is being calculated incorrectly. Attached is the dependency file from the successful build (with the adjusted Makefile), which still fails the test_logical_decoding regression test, with the same diff. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Companypg_receivellog.o: pg_receivellog.c ../../../src/include/postgres.h \ ../../../src/include/c.h ../../../src/include/postgres_ext.h \ ../../../src/include/pg_config_ext.h ../../../src/include/pg_config.h \ ../../../src/include/pg_config_manual.h \ ../../../src/include/pg_config_os.h ../../../src/include/port.h \ ../../../src/include/utils/elog.h ../../../src/include/utils/errcodes.h \ ../../../src/include/utils/palloc.h \ ../../../src/include/common/fe_memutils.h \ ../../../src/include/utils/palloc.h \ ../../../src/interfaces/libpq/libpq-fe.h \ ../../../src/include/postgres_ext.h \ ../../../src/include/libpq/pqsignal.h \ ../../../src/include/access/xlog_internal.h \ ../../../src/include/access/xlogdefs.h \ ../../../src/include/datatype/timestamp.h \ ../../../src/include/lib/stringinfo.h ../../../src/include/pgtime.h \ ../../../src/include/storage/block.h \ ../../../src/include/storage/relfilenode.h \ ../../../src/include/storage/backendid.h \ ../../../src/include/utils/datetime.h ../../../src/include/nodes/nodes.h \ ../../../src/include/utils/timestamp.h ../../../src/include/fmgr.h \ receivelog.h streamutil.h ../../../src/include/getopt_long.h ../../../src/include/postgres.h: ../../../src/include/c.h: ../../../src/include/postgres_ext.h: ../../../src/include/pg_config_ext.h: ../../../src/include/pg_config.h: ../../../src/include/pg_config_manual.h: ../../../src/include/pg_config_os.h: ../../../src/include/port.h: ../../../src/include/utils/elog.h: ../../../src/include/utils/errcodes.h: ../../../src/include/utils/palloc.h: ../../../src/include/common/fe_memutils.h: ../../../src/include/utils/palloc.h: ../../../src/interfaces/libpq/libpq-fe.h: ../../../src/include/postgres_ext.h: ../../../src/include/libpq/pqsignal.h: ../../../src/include/access/xlog_internal.h: ../../../src/include/access/xlogdefs.h: ../../../src/include/datatype/timestamp.h: ../../../src/include/lib/stringinfo.h: ../../../src/include/pgtime.h: ../../../src/include/storage/block.h: ../../../src/include/storage/relfilenode.h: ../../../src/include/storage/backendid.h: ../../../src/include/utils/datetime.h: ../../../src/include/nodes/nodes.h: ../../../src/include/utils/timestamp.h: ../../../src/include/fmgr.h: receivelog.h: streamutil.h: ../../../src/include/getopt_long.h: -- 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] changeset generation v5-01 - Patches git tree
On 2013-06-24 07:29:43 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: On 2013-06-23 10:32:05 -0700, Kevin Grittner wrote: The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. Hm. Any chance this was an incomplete rebuild? With my hack on the pg_basebackup Makefile, `make -j4 world` is finishing with no errors and: Hm. There were some issues with the test_logical_decoding Makefile not cleaning up the regression installation properly. Which might have caused the issue. Could you try after applying the patches and executing a clean and then rebuild? Otherwise, could you try applying my git tree so we are sure we test the same thing? $ git remote add af git://git.postgresql.org/git/users/andresfreund/postgres.git $ git fetch af $ git checkout -b xlog-decoding af/xlog-decoding-rebasing-cf4 $ ./configure ... $ make Because I can't reproduce it, despite some playing around... Maybe if you can reproduce the build problems I'm seeing Tried your recipe but still couldn't... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From cdd0ed46ab75768f8a2e82394b04e6392d8ed32a Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Mon, 24 Jun 2013 11:52:23 +0200 Subject: [PATCH 1/2] wal_decoding: mergme: Fix pg_basebackup makefile --- src/bin/pg_basebackup/Makefile | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..c251249 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,9 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) \ + pg_basebackup.o pg_receivexlog.o pg_receivellog.o \ + $(OBJS) -- 1.8.2.rc2.4.g7799588.dirty From 022c2da1873de2fbc93ae524819932719ca41bdb Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Mon, 24 Jun 2013 16:47:48 +0200 Subject: [PATCH 2/2] wal_decoding: mergme: Fix test_logical_decoding Makefile --- contrib/test_logical_decoding/Makefile | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/contrib/test_logical_decoding/Makefile b/contrib/test_logical_decoding/Makefile index 0e7d5d3..3850d44 100644 --- a/contrib/test_logical_decoding/Makefile +++ b/contrib/test_logical_decoding/Makefile @@ -4,18 +4,14 @@ OBJS = test_logical_decoding.o EXTENSION = test_logical_decoding DATA = test_logical_decoding--1.0.sql -ifdef USE_PGXS -PG_CONFIG = pg_config -PGXS := $(shell $(PG_CONFIG) --pgxs) -include $(PGXS) -else +# Note: because we don't tell the Makefile there are any regression tests, +# we have to clean those result files explicitly +EXTRA_CLEAN = -r $(pg_regress_clean_files) + subdir = contrib/test_logical_decoding top_builddir = ../.. include $(top_builddir)/src/Makefile.global include $(top_srcdir)/contrib/contrib-global.mk -endif - -test_logical_decoding.o: test_logical_decoding.c # Disabled because these tests require wal_level=logical, which # typical installcheck users do not have (e.g. buildfarm clients). -- 1.8.2.rc2.4.g7799588.dirty -- 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 On 2013-06-15 00:48:17 +0200, Andres Freund wrote: Overview of the attached patches: 0001: indirect toast tuples; required but submitted independently 0002: functions for testing; not required, 0003: (tablespace, filenode) syscache; required 0004: RelationMapFilenodeToOid: required, simple 0005: pg_relation_by_filenode() function; not required but useful 0006: Introduce InvalidCommandId: required, simple 0007: Adjust Satisfies* interface: required, mechanical, 0008: Allow walsender to attach to a database: required, needs review 0009: New GetOldestXmin() parameter; required, pretty boring 0010: Log xl_running_xact regularly in the bgwriter: required 0011: make fsync_fname() public; required, needs to be in a different file 0012: Relcache support for an Relation's primary key: required 0013: Actual changeset extraction; required 0014: Output plugin demo; not required (except for testing) but useful 0015: Add pg_receivellog program: not required but useful 0016: Add test_logical_decoding extension; not required, but contains the tests for the feature. Uses 0014 0017: Snapshot building docs; not required Version v5-01 attached Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Reviewing and testing the final result now. If that all looks good, will submit a separate review of each patch. Simon, do you want to do the final review and commit after I do each piece? -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Kevin Grittner kgri...@ymail.com wrote: Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Just to be paranoid, I did one last build with all 17 patch files applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this line: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world and it died with this: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF .deps/pg_receivexlog.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o pg_receivellog gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' make[2]: *** [all-pg_basebackup-recurse] Error 2 make[2]: *** Waiting for unfinished jobs It works with this patch-on-patch: diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..18d02f3 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,7 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o It appears to be an omission from file 0015. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Kevin Grittner kgri...@ymail.com wrote: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o Just that part. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: Pushed and attached. The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company regression.diffs Description: Binary data -- 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] changeset generation v5-01 - Patches git tree
On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: Kevin Grittner kgri...@ymail.com wrote: Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Just to be paranoid, I did one last build with all 17 patch files applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this line: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world and it died with this: gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF .deps/pg_receivexlog.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o pg_receivellog gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' make[2]: *** [all-pg_basebackup-recurse] Error 2 make[2]: *** Waiting for unfinished jobs I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. It works with this patch-on-patch: diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..18d02f3 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,7 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o It appears to be an omission from file 0015. Yes, both are missing. + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. Hm. Why not? I don't think either hunk has anything to do with that buildfailure though - can you reproduce the error without? Thanks, 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] changeset generation v5-01 - Patches git tree
On 2013-06-23 10:32:05 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: Pushed and attached. The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. Hm. Any chance this was an incomplete rebuild? I seem to remember having seen that once because some header dependency wasn't recognized correctly after applying some patch. Otherwise, could you give me: * the version you aplied the patch on * os/compiler Because I can't reproduce it, despite some playing around... 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com writes: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. What versions of gmake are you guys using? It wouldn't be the first time we've tripped over bugs in parallel make. See for instance http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 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] changeset generation v5-01 - Patches git tree
On 2013-06-23 16:48:41 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. What versions of gmake are you guys using? It wouldn't be the first time we've tripped over bugs in parallel make. See for instance http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 3.81 here. That was supposed to be the safe one, right? At least to the bugs seen/fixed recently. Kevin, any chance you still have more log than in the upthread mail available? 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: 0007: Adjust Satisfies* interface: required, mechanical, Version v5-01 attached I'm still working on a review and hope to post something more substantive by this weekend, but when applying patches in numeric order, this one did not compile cleanly. gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o allpaths.o allpaths.c -MMD -MP -MF .deps/allpaths.Po vacuumlazy.c: In function ‘heap_page_is_all_visible’: vacuumlazy.c:1725:3: warning: passing argument 1 of ‘HeapTupleSatisfiesVacuum’ from incompatible pointer type [enabled by default] In file included from vacuumlazy.c:61:0: ../../../src/include/utils/tqual.h:84:20: note: expected ‘HeapTuple’ but argument is of type ‘HeapTupleHeader’ Could you post a new version of that? -- Kevin Grittner 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
