On Fri, Dec 29, 2023 at 12:56 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Here's a draft patch along this line. Basically the idea is that > subtransactions used for error control are now legal in parallel > mode (including in parallel workers) so long as they don't try to > acquire their own XIDs. I had to clean up some error handling > in xact.c, but really this is a pretty simple patch.
I agree with the general direction. A few comments: - Isn't it redundant to test if IsInParallelMode() || IsParallelWorker()? We can't be in a parallel worker without also being in parallel mode, except during the worker startup sequence. - I don't think the documentation changes are entirely accurate. The whole point of the patch is to allow parallel workers to make changes to the transaction state, but the documentation says you can't. Maybe we should just delete "change the transaction state" entirely from the list of things that you're not allowed to do, since "write to the database" is already listed separately; or maybe we should replace it with something like "assign new transaction IDs or command IDs," although that's kind of low-level. I don't think we should just delete the "even temporarily" bit, as you've done. - While I like the new comments in BeginInternalSubTransaction(), I think the changes in ReleaseCurrentSubTransaction() and RollbackAndReleaseCurrentSubTransaction() need more thought. For one thing, it's got to be wildly optimistic to claim that we would have caught *anything* that's forbidden in parallel mode; that would require solving the halting problem. I'd rather have no comment at all here than one making such an ambitious claim, and I think that might be a fine way to go. But if we do have a comment, I think it should be more narrowly focused e.g. "We do not check for parallel mode here. It's permissible to start and end subtransactions while in parallel mode, as long as no new XIDs or command IDs are assigned." One additional thing that might (or might not) be worth mentioning or checking for here is that the leader shouldn't try to reduce the height of the transaction state stack to anything less than what it was when the parallel operation started; if it wants to do that, it needs to clean up the parallel workers and exit parallel mode first. Similarly a worker shouldn't ever end the toplevel transaction except during backend cleanup. -- Robert Haas EDB: http://www.enterprisedb.com
