Robert Haas <robertmh...@gmail.com> writes: > On Fri, Mar 11, 2016 at 1:11 PM, David Steele <da...@pgmasters.net> wrote: >> Is anyone willing to volunteer a review or make an argument for the >> importance of this patch?
> There's been a lot of discussion on another thread about this patch. > The subject is "The plan for FDW-based sharding", but the thread kind > of got partially hijacked by this issue. The net-net of that is that > I don't think we have a clear enough idea about where we're going with > global transaction management to make it a good idea to adopt an API > like this. For example, if we later decide we want to put the > functionality in core, will we keep the hooks around for the sake of > alternative non-core implementations? I just don't believe this > technology is nearly mature enough to commit to at this point. > Konstantin does not agree with my assessment, perhaps unsurprisingly. I re-read the original thread, http://www.postgresql.org/message-id/flat/f2766b97-555d-424f-b29f-e0ca0f6d1...@postgrespro.ru I think there is no question that this is an entirely immature patch. Not coping with subtransactions is alone sufficient to make it not credible for production. Even if the extension API were complete and clearly stable, I have doubts that there's any great value in integrating it into 9.6, rather than some later release series. The above thread makes it clear that pg_dtm is very much WIP and has easily a year's worth of work before anybody would think of wanting to deploy it. So end users don't need this patch in 9.6, and developers working on pg_dtm shouldn't really have much of a problem applying the patch locally --- how likely is it that they'd be using a perfectly stock build of the database apart from this patch? But my real takeaway from that thread is that there's no great reason to believe that this API definition *is* stable. The single existing use-case is very far from completion, and it's hardly unlikely that what it needs will change. I also took a very quick look at the patch itself: 1. No documentation. For something that purports to be an API specification, really the documentation should have been written *first*. 2. As noted in the cited thread, it's not clear that Get/SetTransactionStatus are a useful cutpoint; they don't provide any real atomicity guarantees. 3. Uh, how can you hook GetNewTransactionId but not ReadNewTransactionId? 4. There seems to be an intention to encapsulate snapshots, but surely wrapping hooks around GetSnapshotData and XidInMVCCSnapshot is not nearly enough for that. Look at all the knowledge snapmgr.c has about snapshot representation, for example. And is a function like GetOldestXmin even meaningful with a different notion of what snapshots are? (For that matter, is TransactionId == uint32 still tenable for any other notion of snapshots?) 5. BTW, why would you hook at XidInMVCCSnapshot rather than making use of the existing capability to have a custom SnapshotSatisfiesFunc snapshot checker function? IMO this is not committable as-is, and I don't think that it's something that will become committable during this 'fest. I think we'd be well advised to boot it to the 2016-09 CF and focus our efforts on other stuff that has a better chance of getting finished this month. regards, tom lane -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers