Re: POC: Cleaning up orphaned files using undo logs

On Mon, Jul 22, 2019 at 2:21 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>

I have reviewed 0012-Infrastructure-to-execute-pending-undo-actions,
Please find my comment so far.

1.
+ /* It shouldn't be discarded. */
+ Assert(!UndoRecPtrIsDiscarded(xact_urp));

I think comments can be added to explain why it shouldn't be discarded.

2.
+ /* Compute the offset of the uur_next in the undo record. */
+ offset = SizeOfUndoRecordHeader +
+ offsetof(UndoRecordTransaction, urec_progress);
+
in comment /uur_next/uur_progress

3.
+/*
+ * undo_record_comparator
+ *
+ * qsort comparator to handle undo record for applying undo actions of the
+ * transaction.
+ */
Function header formating is not in sync with other functions.

4.
+void
+undoaction_redo(XLogReaderState *record)
+{
+ uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
+
+ switch (info)
+ {
+ case XLOG_UNDO_APPLY_PROGRESS:
+ undo_xlog_apply_progress(record);
+ break;

For HotStandby it doesn't make sense to apply this wal as this
progress is only required when we try to apply the undo action after
restart
but in HotStandby we never apply undo actions.

5.
+ Assert(from_urecptr != InvalidUndoRecPtr);
+ Assert(to_urecptr != InvalidUndoRecPtr);

we can use macros UndoRecPtrIsValid instead of checking like this.

6.
+ if ((slot == NULL) || (UndoRecPtrGetLogNo(urecptr) != slot->logno))
+ slot = UndoLogGetSlot(UndoRecPtrGetLogNo(urecptr), false);
+
+ Assert(slot != NULL);
We are passing missing_ok as false in UndoLogGetSlot. But, not sure
why we are expecting that undo lot can not be dropped. In multi-log
transaction it's possible
that the tablespace in which next undolog is there is already dropped?

7.
+ */
+ do
+ {
+ BlockNumber progress_block_num = InvalidBlockNumber;
+ int i;
+ int nrecords;
.....
+ */
+ if (!UndoRecPtrIsValid(urec_ptr))
+ break;
+ } while (true);

I think we can convert above loop to while(true) instead of do..while,
because there is no need for do while loop.

8.
+ if (last_urecinfo->uur->uur_info & UREC_INFO_LOGSWITCH)
+ {
+ UndoRecordLogSwitch *logswitch = last_urecinfo->uur->uur_logswitch;

IMHO, the caller of UndoFetchRecord should directly check
uur->uur_logswitch instead of uur_info & UREC_INFO_LOGSWITCH.
Actually, uur_info is internally set
for inserting the tuple and check there to know what to insert and
fetch but I think caller of UndoFetchRecord should directly rely on
the field because ideally all
the fields in UnpackUndoRecord must be set and uur_txt or
uur_logswitch will be allocated when those headers present. I think
this needs to be improved in undo interface patch
as well (in UndoBulkFetchRecord).

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