On Mon, Jul 1, 2019 at 3:54 AM Thomas Munro <thomas.mu...@gmail.com> wrote: > 2. Introduced a new RMGR callback rm_undo_status. It is used to > decide when record sets in the UNDO_SHARED category should be > discarded (instead of the usual single xid-based rules). The possible > answers are "discard me now!", "ask me again when a given XID is all > visible", and "ask me again when a given XID is no longer running".
>From the minor nitpicking department, the patches from this stack that are updating rmgrlist.h are consistently failing to update the comment line preceding the list of PG_RMGR() lines. This looks to be patches 0014 and 0015 in this stack; 0015 seems to need to be squashed into 0014. Reviewing Amit's 0016: performUndoActions appears to be badly-designed. For starters, it's sometimes wrong: the only place it gets set to true is in UndoActionsRequired (which is badly named, because from the name you expect it to return a Boolean and to not have side effects, but instead it doesn't return anything and does have side effects). UndoActionsRequired() only gets called from selected places, like AbortCurrentTransaction(), so the rest of the time it just returns a wrong answer. Now maybe it's never called at those times, but there's no guard to prevent a function like CanPerformUndoActions() (which is also badly named, because performUndoActions tells you whether you need to perform undo actions, not whether it's possible to perform undo actions) from being called before the flag is set. I think that this flag should be either (1) maintained eagerly - so that wherever we set start_urec_ptr we also set the flag right away or (2) removed - so when we need to know, we just loop over all of the undo categories on the spot, which is not that expensive because there aren't that many of them. It seems pointless to make PrepareTransaction() take undo pointers as arguments, because those pointers are just extracted from the transaction state, to which PrepareTransaction() has a pointer. Thomas has already objected to another proposal to add functions that turn 32-bit XIDs into 64-bit XIDs. Therefore, I feel confident in predicting that he will likewise object to GetEpochForXid. I think this needs to be changed somehow, maybe by doing what the XXX comment you added suggests. This patch has some problems with naming consistency. There's a function called PushUndoRequest() which calls a function called RegisterRollbackReq() to do the heart of the work. So, is it undo or rollback? Are we pushing or registering? Is it a request or a req? For bonus points, the flag that the function sets is called undo_req_pushed, which is halfway in between the two competing terminologies. Other gripes about PushUndoRequest: push is vague and doesn't really explain what's happening, "apllying" is a typo, per_level is a poor variable name and shouldn't be declared volatile. This function has problems with naming in other places, too; please go through all of the names carefully and make them consistent and adequately descriptive. I am not a fan of applying_subxact_undo. I think we should look for a better design there. A couple of things occur to me. One is that we don't necessarily need to go to FATAL; we could just force the current transaction and all of its subtransactions fail all the way out to the top level, but then perhaps allow new transactions to be started afterwards. I'm not sure that's worth it, but it would work, and I think it has precedent in SxactIsDoomed. Assuming we're going to stick with the current FATAL plan, I think we should do something like invent a new kind of critical section that forces ERROR to be promoted to FATAL and then use it here. We could call it a semi-critical or locally-critical section, and the undo machinery could use it, but then also so could other things. I've wanted that sort of concept before, so I think it's a good idea to try to have something general and independent of undo. The same concept could be used in PerformUndoActions() instead of having to invent pg_rethrow_as_fatal(), so we'd have two uses for this mechanism right away. FinishPreparedTransactions() tries to apply undo actions while interrupts are still held. Is that necessary? Can we avoid it? It seems highly likely that the logic added to the TBLOCK_SUBCOMMIT case inside CommitTransactionCommand and also into ReleaseCurrentSubTransaction should have been added to CommitSubTransaction instead. If that's not true, then we have to believe that the TBLOCK_SUBRELEASE call to CommitSubTransaction needs different treatment from the other two cases, which sounds unlikely; we also have to explain why undo is somehow different from all of these other releases that are already handled in that function, not in its callers. I also strongly suspect it is altogether wrong to do this before CommitSubTransaction sets s->state to TRANS_COMMIT; what if a subxact callback throws an error? For related reasons, I don't think that the change ReleaseSavepoint() are right either. Notice the header comment: "As above, we don't actually do anything here except change blockState." The "as above" part of the comment probably didn't originally refer to DefineSavepoint(), which definitely does do other stuff, but to something like EndImplicitTransactionBlock() or EndTransactionBlock(), and DefineSavepoint() got stuck in the middle later. Anyway, your patch makes the comment false by doing actual state changes in this function, rather than just marking the subtransactions for commit. But why should that be right? If none of the many other bits of state are manipulated here rather than in CommitSubTransaction(), why is undo the one thing that is different? I guess this is basically just compensation for the lack of any of this code in the TBLOCK_SUBRELEASE path which I noted in the previous paragraph, but I still think the right answer is to put it all in CommitSubTransaction() *after* we set TRANS_COMMIT. There are a number of things I either don't like or don't understand about PerformUndoActions. One is that undo_req_pushed gets passed to this function. That just looks really odd from an abstraction point of view. Basically, we have a function whose job is to "perform undo actions," and it gets a flag as an argument that tells it to not actually perform some of the undo actions: that's odd. I think the reason it's like that is because of the issue we've been discussing elsewhere that there's a separate undo request for each category. If you didn't have that, you wouldn't need to do this here. I'm not saying that proves that the one-request-per-persistence-level design is definitely wrong, but this is certainly not a point in its favor, at least IMHO. PerformUndoActions() also thinks that there is a possibility of failing to insert a failed request into the error queue, and makes reference to such requests being rediscovered by the discard worker, but I thought (as I said in my previous email) that we had abandoned that approach in favor of always having enough space in shared memory to record everything. Among other problems, if you want oldestXidHavingUndo to be calculated based on the information in shared memory, then you have to have all the records in shared memory, not lose some of them temporarily and have them get re-inserted into the error queue. It also feels to me like there may be a conflict between the everything-must-fit approach and the one-request-per-persistence level thing you've got here. I believe Andres's idea was one-request-per-transaction, so the idea is something like: - When your transaction first tries to attach to an undo log, you make a hash table entry. - If that fails, you error out, but you have no undo, so it's OK. - If it works, then you know that there's no chance of aborting without making a hash table entry, because you already did it. - If you commit, you remove the entry, because your transaction does not need to be undone. - If you abort, you process the entry in the foreground if it's small or if the number of hash table slots remaining is < max_connections. Otherwise you leave it for the background worker to handle. If you have one request per persistence level, you could make an entry for the first persistence level, and then find that you are out of room when trying to make an entry for the second persistence level. I guess that doesn't break anything: the changes from the first persistence level would get undone, and the second persistence level wouldn't get any undo. Maybe that's OK, but again it doesn't seem all that nice, so maybe we need to think about it some more. I think there's more, but I am out of time for the moment. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company