On 03/11/2016 11:35 PM, Tom Lane wrote:
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.
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© 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
