Re: eXtensible Transaction Manager API (v2)

On 03/11/2016 11:35 PM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Mar 11, 2016 at 1:11 PM, David Steele <david(at)pgmasters(dot)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(at)postgrespro(dot)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.

Lack of subtractions support is not a limitation of XTM API.
It is limitation of current pg_dtm implementation. And another DTM implementation - pg_tsdtm supports subtransactions.

>
> 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?

I agree with you that pg_dtm is very far from production.
But I wan to notice two things:

1. pg_dtm and pg_tsdtm are not complete cluster solutions, them are just one (relatively small) part of them.
pg_tsdtm seems to be even more "mature", may be because it is simpler and do not have many limitations which pg_dtm has (like subtrasaction support).

2. Them can be quite easily integrated with other (existed) cluster solutions. We have integrated bother of them with postgres_fwd and pg_shard.
Postgres_fdw is also not a ready solution, but just a mechanism which can be used also for sharding.
But pg_shard & CitusDB are quite popular solutions for distributed execution of queries which provide good performance for analytic and single node OLTP queries.
Integration with DTMs adds ACID semantic for distributed transactions and makes it possible to support more complex OLTP and OLAP queries involving multiple nodes.

Such integration is already done, performance was evaluated, so it is not quite correct to say that we need a year or more to make pg_dtm/pg_tsdtm ready to deploy.

>
> 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.
>
Sorry, may be I am completely wrong, but I do not thing that it is possible to develop stable API if nobody is using it.
It is like "fill pool with a water only after you learn how to swim".

> 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*.

Sorry, it was my fault. I have already written documentation and it will be included in next version of the patch.
But please notice, that starting work on DTM we do not have good understanding with PostgreSQL TM features have to be changed.
Only during work on pg_dtm, pg_tsdtm, multimaster current view of XTM has been formed.

And yet another moment: we have not introduce new abstractions in XTM.
We just override existed PostgreSQL functions.
Certainly when some internal functions become part of API, it should be much better documented.

> 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.

I wonder how such guarantees can be provided at API level?
Atomicity means that all other transaction either see this transaction as committed, either uncommitted.
So transaction commit should be coordinated with visibility check.
In case of pg_dtm atomicity is simply enforced by the fact that decision whether to commit transaction is taken by central coordinator.
When it decides that transaction is committed, it marks it as committed in all subsequently obtained snapshots.

In case of pg_tsdtm there is no central arbiter, so we have to introduce "in-doubt" state of transaction, when it is not known whether transaction is
committed or aborted and any other transaction accessing tuples updated but this transaction has to wait while its status is "in-doubt".
The main challenge of pg_tsdtm is to make this period as short as possible...

But it is details of particular implementation which IMHO have no relation to API itself.

>
> 3. Uh, how can you hook GetNewTransactionId but not ReadNewTransactionId?

Uh-uh-uh:)
ReadNewTransactionId is just reading value of ShmemVariableCache->nextXid,
but unfortunately it is not the only point where nextXid is used - there are about hundred occurrences of nextXid in Postgres core.
This is why we made a decision that GetNewTransactionId should actually update ShmemVariableCache->nextXid, so that
there is no need to rewrite all this code.
Sorry, but IMHO it is problem of Postgres design and not of XTM;)
We just want to find some compromise which allows XTM to be flexible enough but minimize changes in core code.

> 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?)

XTM encapsulation of snapshots allows us to implement pg_dtm.
It does almost the same as Postgres-XL GTM, but without huge amount of #ifdefs.

Representation of XID is yet another compromise point: we do not want to change tuple header format.
So XID is still 32 bit and has the same meanining as in PostgreSQL. If custom implementation of TM wants to use some other identifiers of transactions,
like CSN in pg_tsdtm, it has to provide mapping between them and XIDs.

>
> 5. BTW, why would you hook at XidInMVCCSnapshot rather than making use of
> the existing capability to have a custom SnapshotSatisfiesFunc snapshot
> checker function?

HeapTupleSatisfies routines in times/tqual.c have implemented a lot of logic of handling different kind of snapshots, checking/setting hint bits in tuples,
caching,... We do not want to replace or just cut&copy all this code in DTM implementation.
And XidInMVCCSnapshot is common function finally used by most HeapTupleSatisfies* functions when all other checks are passed.
So it is really the most convenient place to plug-in custom visibility checking rules. And as far as I remember similar approach was used in Postgres-XL.

>
>
> 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

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company