Re: POC: Cleaning up orphaned files using undo logs

On Thu, Jul 25, 2019 at 11:25 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> Hi Thomas,
>
> I have started reviewing 0003-Add-undo-log-manager, I haven't yet
> reviewed but some places I noticed that instead of UndoRecPtr you are
> directly
> using UndoLogOffset. Which seems like bugs to me
>
> 1.
> +UndoRecPtr
> +UndoLogAllocateInRecovery(UndoLogAllocContext *context,
> + TransactionId xid,
> + uint16 size,
> + bool *need_xact_header,
> + UndoRecPtr *last_xact_start,
> ....
> + *need_xact_header =
> + context->try_location == InvalidUndoRecPtr &&
> + slot->meta.unlogged.insert == slot->meta.unlogged.this_xact_start;
> + *last_xact_start = slot->meta.unlogged.last_xact_start;
>
> the output parameter last_xact_start is of type UndoRecPtr whereas
> slot->meta.unlogged.last_xact_start is of type UndoLogOffset
> shouldn't we use MakeUndoRecPtr(logno, offset) here?
>
> 2.
> + slot = find_undo_log_slot(logno, false);
> + if (UndoLogOffsetPlusUsableBytes(try_offset, size) <= slot->meta.end)
> + {
> + *need_xact_header = false;
> + return try_offset;
> + }
>
> Here also you are returning directly try_offset instead of UndoRecPtr
>

+UndoLogRegister(UndoLogAllocContext *context, uint8 block_id,
UndoLogNumber logno)
+{
+ int i;
+
+ for (i = 0; i < context->num_meta_data_images; ++i)
+ {
+ if (context->meta_data_images[i].logno == logno)
+ {
+ XLogRegisterBufData(block_id,
+ (char *) &context->meta_data_images[i].data,
+ sizeof(context->meta_data_images[i].data));
+ return;
+ }
+ }
+}

I have observed one more thing that you are registering
"meta_data_images" with each buffer of that log. Suppose, if one undo
record is spread across 2 undo blocks then both the blocks will
include a duplicate copy of this metadata image if this first changes
after the checkpoint? It will not cause any issue but IMHO we can
avoid including 2 copies of the same meta_data_image.

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