Re: POC: Cleaning up orphaned files using undo logs

On Thu, Jun 20, 2019 at 2:24 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Jun 19, 2019 at 11:35 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 2:07 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Tue, Jun 18, 2019 at 7:31 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > [ new patches ]
> > >
> > > I tried writing some code [ to use these patches ].
> >
> > I spent some more time experimenting with this patch set today and I
> > think that the UndoFetchRecord interface is far too zheap-centric.  I
> > expected that I would be able to do this:
> >
> > UnpackedUndoRecord *uur = UndoFetchRecord(urp);
> > // do stuff with uur
> > UndoRecordRelease(uur);
> >
> > But I can't, because the UndoFetchRecord API requires me to pass not
> > only an undo record but also a block number, an offset number, an XID,
> > and a callback.  I think I could get the effect that I want by
> > defining a callback that always returns true.  Then I could do:
> >
> > UndoRecPtr junk;
> > UnpackedUndoRecord *uur = UndoFetchRecord(urp, InvalidBlockNumber,
> > InvalidOffsetNumber, &junk, always_returns_true);
> > // do stuff with uur
> > UndoRecordRelease(uur);
> >
> > That seems ridiculously baroque.  I think the most common thing that
> > an AM will want to do with an UndoRecPtr is look up that exact record;
> > that is, for example, what zedstore will want to do. However, even if
> > some AMs, like zheap, want to search backward through a chain of
> > records, there's no real reason to suppose that all of them will want
> > to search by block number + offset.  They might want to search by some
> > bit of data buried in the payload, for example.
> >
> > I think the basic question here is whether we really need anything
> > more complicated than:
> >
> > extern UnpackedUndoRecord *UndoFetchRecord(UndoRecPtr urp);
> >
> > I mean, if you had that, the caller can implement looping easily
> > enough, and insert any test they want:
> >
> > for (;;)
> > {
> >     UnpackedUndoRecord *uur = UndoFetchRecord(urp);
> >     if (i like this one)
> >         break;
> >     urp = uur->uur_blkprev; // should be renamed, since zedstore +
> > probably others will have tuple chains not block chains
> >     UndoRecordRelease(uur);
> > }
>
> The idea behind having the loop inside the undo machinery was that
> while traversing the blkprev chain, we can read all the undo records
> on the same undo page under one buffer lock.
>

I think if we want we can hold this buffer and allow it to be released
in UndoRecordRelease.  However, this buffer needs to be stored in some
common structure which can be then handed over to UndoRecordRelease.
Another thing is that as of now the API allocates the memory just once
for UnpackedUndoRecord whereas in the new scheme it needs to be
allocated again and again.  I think this is a relatively minor thing,
but it might be better if we can avoid palloc again and again.

BTW, while looking at the code of UndoFetchRecord, I see some problem.
There is a coding pattern like
if()
{
}
else
{
   LWLockAcquire()
  ..
  ..
}

LWLockRelease().

I think this is not correct.

> >
> > BTW, an actually generic iterator interface would probably look more like this:
> >
> > typedef bool (*SatisfyUndoRecordCallback)(void *callback_data,
> > UnpackedUndoRecord *uur);
> Right, it should be this way.
>
> > extern UnpackedUndoRecord *UndoFetchRecord(UndoRecPtr urp, UndoRecPtr
> > *found, SatisfyUndoRecordCallback callback, void *callback_data);
> >
> > Now we're not assuming anything about what parts of the record the
> > callback wants to examine.  It can do whatever it likes.  Typically
> > with this sort of interface a caller will define a file-private struct
> > that is known to both the callback and the caller of UndoFetchRecord,
> > but not elsewhere.
> >
> > If we decide we need an iterator within the undo machinery itself,
> > then I think it should look like the above, and I think it should
> > accept NULL for found, callback, and callback_data, so that somebody
> > who wants to just look up a record, full stop, can do just:
> >
> > UnpackedUndoRecord *uur = UndoFetchRecord(urp, NULL, NULL, NULL);
> >
> > which seems tolerable.
> >
> I agree with this.
>

+1.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com