Re: eXtensible Transaction Manager API (v2)

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Mar 11, 2016 at 1:11 PM, David Steele <david@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-E0CA0F6D1D74@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