This change is 

This change still requires unittests; I've only tested it works manually so far.

I would like early feedback on SQL changes as this is the first time I've messed with the grammar or added a plan node. I mostly just cargo culted.

Spencer I'm sure this is probably awesome, but unfortunately I won't be able to review in the next two weeks. I'm a bit swamped trying to finish other stuff. If you want it soon, might want to try someone else. Sorry.

fly-by: might wanna mention #22329

Btw, are you sure we want to add this feature now? Was there anything other than that one forum post that made you do this? Seems like a pretty large change...

Review status: 0 of 48 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

It’s not the first time the question has been asked around XA. If we want to integrate with any externally managed transactions, this is a requirement. In practice this usually means message queues. If you really look at this change, it’s tiny. The bulk is just adding the syntax and a new system table. The pull request here is 10x the actual meaningful changed lines, zero exaggeration. I will add someone else for reviewer.

If you do modify conn_fsm.go, please do it on top of #22367. That adds something that'll probably affect the changes you'd be making.

Another random thought - I guess we need to fail the prepare if the txn has queued any schema changes.

@andreimatei it's not clear to me how to add the changes to conn_fsm.go. Why would I add anything there when all of the code I'm cargo-culting from is in executor.go?

Also, yes, this provides protection against the intents being pushed. There's a 1h default timeout for prepared transaction records. After that they're fair game.

I'm going to have to check in with you when I get a free moment this week to see how to accommodate your suggestion.

Added unittests and a mechanism to update the prepared_xacts table upon what's called a "heuristic commit/abort". According to my reading of the X/Open specification, this applies any time the resource manager futzes with the prepared transaction before it can be committed or rolled back by the transaction manager.

This doesn't just need unittests - we need to test that it can work in an actual distributed transaction with another system (Ideally in an automated way, but manually might be OK as a stopgap as long as we document how to repeat it).

This opens up a lot of compatibility surface area, which our experience with ORM compatibility suggests will take a lot of tweaking. The x/open spec has a lot of rules around when the RM is allowed to forget about a transaction (even after it's been aborted), and about the error codes that are used (the "heuristic commit/abort" cases are supposed to be distinguishable from other outcomes).

The new system table containing prepared transactions could grow quite large; I think this may have the potential to be the largest thing we load up in a virtual table, which could lead to memory problems.

I'm -1 on implementing this in the near future. Demand for this feature is low (AFAICT), and the testing/support burden is high.

I spent some time looking through the postgres code base today to understand what kind of errors they're returning on various error conditions, like heuristic abort/commits. There are no specific errors being returned. Basically the conclusion I've reached is that the X/Open specification is mostly ignored by Postgres' implementation and that's not as big a deal as it might sound like because in practice, transaction managers merely rely on success or failure responses to the first and second commit phases. A failure on the first phase means the TM won't move to the second phase. A failure on the second phase is assumed to mean the resource manager independently finalized its local txn state and the TM will return a significant failure.

Postgres, for example, does not handle requests by the TM to forget prepared transactions. Even if we were to follow the X/Open specification exactly, it wouldn't do any good because the postgres XA drivers that I've looked at don't use any of the extra stuff.

My thought was that the prepared transactions table would in fact not get large – at least no larger than an order of magnitude more than the peak number of concurrent distributed transactions in the system. Having these things hang around is bad news and limiting their lifetimes is the main concern of transaction managers. If the TM is in recovery, it also isn't preparing new transactions, so they don't grow without bound just because a TM is offline. Maybe that's hopelessly naive.

My larger point for doing this is that it seemed to fit nicely with our existing transaction model, the whole thing is fresh in my head, and it's been requested now three times that I've seen.

I agree it needs testing with an actual transaction manager.

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Any updates on further testing?