On Wed, Jul 24, 2019 at 10:00 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar <dilipbalaut@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