Re: POC: Cleaning up orphaned files using undo logs

On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
Please find my review comments for
0013-Allow-foreground-transactions-to-perform-undo-action

+ /* initialize undo record locations for the transaction */
+ for (i = 0; i < UndoLogCategories; i++)
+ {
+ s->start_urec_ptr[i] = InvalidUndoRecPtr;
+ s->latest_urec_ptr[i] = InvalidUndoRecPtr;
+ s->undo_req_pushed[i] = false;
+ }

Can't we just memset this memory?

+ * 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?

+ /*
+ * Prepare required undo request info so that it can be used in
+ * exception.
+ */
+ ResetUndoRequestInfo(&urinfo);
+ urinfo.dbid = dbid;
+ urinfo.full_xid = fxid;
+ urinfo.start_urec_ptr = start_urec_ptr[per_level];
+

I see that we are preparing urinfo before execute_undo_actions so that
in case of an error in CATCH we can use that to
insert into the queue, but can we just initialize urinfo right there
before inserting into the queue, we have all the information
Am I missing something?

+
+ /*
+ * We need the locations of the start and end undo record pointers when
+ * rollbacks are to be performed for prepared transactions using undo-based
+ * relations. We need to store this information in the file as the user
+ * might rollback the prepared transaction after recovery and for that we
+ * need it's start and end undo locations.
+ */
+ UndoRecPtr start_urec_ptr[UndoLogCategories];
+ UndoRecPtr end_urec_ptr[UndoLogCategories];

it's -> its

+ 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.

+ for (i = 0; i < UndoLogCategories; i++)
+ {
+ if (s->latest_urec_ptr[i])
+ {
+ s->performUndoActions = true;
+ break;
+ }
+ }

I think we should chek UndoRecPtrIsValid(s->latest_urec_ptr[i])

+ PG_TRY();
+ {
+ /*
+ * Prepare required undo request info so that it can be used in
+ * exception.
+ */
+ ResetUndoRequestInfo(&urinfo);
+ urinfo.dbid = dbid;
+ urinfo.full_xid = fxid;
+ urinfo.start_urec_ptr = start_urec_ptr[per_level];
+
+ /* for subtransactions, we do partial rollback. */
+ execute_undo_actions(urinfo.full_xid,
+ end_urec_ptr[per_level],
+ start_urec_ptr[per_level],
+ !isSubTrans);
+ }
+ PG_CATCH();

Wouldn't it be good to explain in comments that we are not rethrowing
the error in PG_CATCH but because we don't want the main
transaction to get an error if there is an error while applying to
undo action for the main transaction and we will abort the transaction
in the caller of this function?

+tables are only accessible in the backend that has created them. 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.

I think we need to give detail reasoning why subtransaction changes
will be visible if we don't apply it's undo and the main
the transaction commits by mentioning that we don't use separate
transaction id for the subtransaction and that will make all the
changes of the transaction id visible when it commits.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com