Re: POC: Cleaning up orphaned files using undo logs

On Tue, Aug 6, 2019 at 1:26 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
> > Need to do something else for a bit. More later.
>
> > + * false, otherwise.
> > + */
> > +static bool
> > +UndoAlreadyApplied(FullTransactionId full_xid, UndoRecPtr to_urecptr)
> > +{
> > + UnpackedUndoRecord *uur = NULL;
> > + UndoRecordFetchContext context;
> > +
> > + /* Fetch the undo record. */
> > + BeginUndoFetch(&context);
> > + uur = UndoFetchRecord(&context, to_urecptr);
> > + FinishUndoFetch(&context);
>
> Literally all the places that fetch a record, fetch them with exactly
> this combination of calls. If that's the pattern, what do we gain by
> this split? Note that UndoBulkFetchRecord does *NOT* use an
> UndoRecordFetchContext, for reasons that are beyond me.

Actually, for the zheap or any other AM, where it needs to traverse
the transactions undo the chain. For example, in zheap we will get the
latest undo record pointer from the slot but we need to traverse the
undo record chain backward using the prevundo pointer store in the
undo record and find the undo record for a particular tuple. Earlier,
there was a loop in UndoFetchRecord which were traversing the chain of
the undo until it finds the matching record and record was matched
using a callback. There was also an optimization that if the current
record doesn't satisfy the callback then we keep the pin hold on the
buffer and go to the previous record in the chain. Later based on the
review comments by Robert we have decided that finding the matching
undo record should be caller's responsibility so we have moved the
loop out of the UndoFetchRecord and kept it in the zheap code. The
reason for keeping the context is that we can keep the buffer pin held
and remember that buffer in the context so that the caller can call
UndoFetchRecord in a loop and the pin will be held on the buffer from
which we have read the last undo record.

I agree that in undoprocessing patch set we always need to fetch one
record so instead of repeating this pattern everywhere we can write
one function and move this sequence of calls in that function.

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