Re: POC: Cleaning up orphaned files using undo logs

On Tue, Apr 30, 2019 at 8:44 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Apr 30, 2019 at 2:16 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > Like previous version these patch set also applies on:
> > https://github.com/EnterpriseDB/zheap/tree/undo
> > (b397d96176879ed5b09cf7322b8d6f2edd8043a5)
>
> Some more review of 0003:
>
> There is a whitespace-only hunk in xact.c.
Fixed
>
> It would be nice if AtAbort_ResetUndoBuffers didn't need to exist at
> all. Then, this patch would make no changes whatsoever to xact.c.
> We'd still need such changes in other patches, because the whole idea
> of undo is tightly bound up with the concept of transactions. Yet,
> this particular patch wouldn't touch that file, and that would be
> nice. In general, the reason why we need AtCommit/AtAbort/AtEOXact
> callbacks is to adjust the values of global variables (or the data
> structures to which they point) at commit or abort time. And that is
> also the case here. The thing is, I'm not sure why we need these
> particular global variables. Is there some way that we can get by
> without them? The obvious thing to do seems to be to invent yet
> another context object, allocated via a new function, which can then
> be passed to PrepareUndoInsert, InsertPreparedUndo,
> UndoLogBuffersSetLSN, UnlockReleaseUndoBuffers, etc. This would
> obsolete UndoSetPrepareSize, since you'd instead pass the size to the
> context allocator function.

I have moved all the global variables to a context. Now, I think we
don't need AtAbort_ResetUndoBuffers as memory will be freed with the
transaction context.

>
> UndoRecordUpdateTransInfo should declare a local variable
> XactUndoRecordInfo *something = &xact_urec_info[idx] and use that
> variable wherever possible.
>
Done.
> It should also probably use while (1) { ... } rather than do { ... }
> while (true).
Ok

>
> In UndoBufferGetSlot you could replace 'break;' with 'return i;' and
> then more than half the function would need one less level of
> indentation. This function should also declare PreparedUndoBuffer
> *something and set that variable equal to &prepared_undo_buffers[i] at
> the top of the loop and again after the loop, and that variable should
> then be used whenever possible.

Done
>
> UndoRecordRelationDetails seems to need renaming now that it's down to
> a single member.
I have directly moved that context to UndoPackContext
>
> The comment for UndoRecordBlock needs updating, as it still refers to blkprev.
Done
>
> The comment for UndoRecordBlockPrev refers to "Identifying
> information" but it's not identifying anything. I think you could
> just delete "Identifying information for" from this sentence and not
> lose anything. And I think several of the nearby comments that refer
> to "Identifying information" could more simply and more correctly just
> refer to "Information".
Done
>
> I don't think that SizeOfUrecNext needs to exist.
Removed
>
> xact.h should not add an include for undolog.h. There are no other
> changes to this header, so presumably the header does not depend on
> anything in undolog.h. If .c files that include xact.h need
> undolog.h, then the header should be included in those files, not in
> the header itself. That way, we avoid making partial recompiles more
> painful than necessary.

Right, fixed.
>
> UndoGetPrevUndoRecptr looks like a strange interface. Why not just
> arrange not to call the function in the first place if prevurp is
> valid?
Done
>
> Every use of palloc0 in this code should be audited to check whether
> it is really necessary to zero the memory before use. If it will be
> initialized before it's used for anything anyway, it doesn't need to
> be pre-zeroed.

Yeah, I found at few places it was not required so fixed.

>
> FinishUnpackUndo looks nice! But it is missing a blank line in one
> place, and 'if it presents' should be 'if it is present' in a whole
> bunch of places.
>
> BeginInsertUndo also looks to be missing a few blank lines.
Fixed
>
> Still need to do some cleanup of the UnpackedUndoRecord, e.g. unifying
> uur_xid and uur_xidepoch into uur_fxid.
>
I will work on this.

> InsertUndoData ends in an unnecessary return, as does SkipInsertingUndoData.
Done

>
> I think the argument to SkipInsertingUndoData should be the number of
> bytes to skip, rather than the starting byte within the block.
Done
>
> Function header comment formatting is not consistent. Compare
> FinishUnpackUndo (function name recapitulated on first line of
> comment) to ReadUndoBytes (not recapitulated) to UnpackUndoData
> (entire header comment jammed into one paragraph with function name at
> start). I prefer the style where the function name is not restated,
> but YMMV. Anyway, it has to be consistent.
>
Fixed
> UndoGetPrevRecordLen should not declare char *page instead of Page
> page, I think.
>
> UndoRecInfo looks a bit silly, I think. Isn't index just the index of
> this entry in the array? You can always figure that out by ptr -
> array_base. Instead of having UndoRecPtr urp in this structure, how
> about adding that to UnpackedUndoRecord? When inserting, caller
> leaves it unset and InsertPreparedUndo sets it; when retrieving,
> UndoFetchRecord or UndoRecordBulkFetch sets it. With those two
> changes, I think you can get rid of UndoRecInfo entirely and just
> return an array of UnpackedUndoRecords. This might also eliminate the
> need for an 'urp' member in PreparedUndoSpace.

As discussed upthread, I will work on fixing this.

>
> I'd probably use UREC_INFO_BLKPREV rather than UREC_INFO_BLOCKPREV for
> consistency.
>
> Similarly UndoFetchRecord and UndoRecordBulkFetch seems a bit
> inconsistent. Perhaps UndoBulkFetchRecord.
Done
>
> In general, I find the code for updating transaction headers to be
> really hard to understand. I'm not sure exactly what can be done
> about that. Like, why is UndoRecordPrepareTransInfo unpacking undo?

It's only unpacking header. But, yeah we can do better, instead of
unpacking we can just read the main header and from uur_info we can
calculate exact offset of the uur_next and in
UndoRecordUpdateTransInfo we can directly update only uur_next by
writing at that offset, instead of overwriting the complete header?

> Why does it take two undo record pointers as arguments and how are
> they different?
One is previous transaction's start header which we wants to update
and other is current transaction's urec pointer what we want to set as
uur_next in the previous transaction's start header.

Just for tracking, open comments which still needs to be worked on.

1. Avoid special case in UndoRecordIsValid.
> Can we instead eliminate the special case? It seems like the if
> (log->oldest_data == InvalidUndoRecPtr) case will be taken very
> rarely, so if it's buggy, we might not notice.

2. While updating the previous transaction header instead of unpacking
complete header and writing it back, we can just unpack main header
and calculate the offset of uur_next and then update it directly.

3. unifying uur_xid and uur_xidepoch into uur_fxid.

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