On Wed, Jul 24, 2019 at 10:00 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > Please find my review comments for > 0013-Allow-foreground-transactions-to-perform-undo-action > > > + * We can't postpone applying undo actions for subtransactions as the > + * modifications made by aborted subtransaction must not be visible even if > + * the main transaction commits. > + */ > + if (IsSubTransaction()) > + return; > > I am not completely sure but is it possible that the outer function > CommitTransactionCommand/AbortCurrentTransaction can avoid > calling this function in the switch case based on the current state, > so that under subtransaction this will never be called? >
We can do that and also can have an additional check similar to "if (!s->performUndoActions)", but such has to be all places from where this function is called. I feel that will make code less readable at many places. > > + bool undo_req_pushed[UndoLogCategories]; /* undo request pushed > + * to worker? */ > + bool performUndoActions; > + > struct TransactionStateData *parent; /* back link to parent */ > > We must have some comments to explain how performUndoActions is used, > where it's set. If it's explained somewhere else then we can > give reference to that code. > I am planning to remove this variable in the next version and have an explicit check as we have in UndoActionsRequired. I agree with your other comments and will address them in the next version of the patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com