Re: POC: Cleaning up orphaned files using undo logs

On Tue, Aug 20, 2019 at 8:10 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Aug 20, 2019 at 2:42 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > Well, my main point, which so far has largely been ignored, was that we
> > > may not acquire page locks when we still need to search for victim
> > > buffers later. If we don't need to lock the pages up-front, but only do
> > > so once we're actually copying the records into the undo pages, then we
> > > don't a separate phase to acquire the locks. We can still hold all of
> > > the page locks at the same time, as long as we just acquire them at the
> > > later stage.
> >
> > Okay, IIUC, this means that we should have a separate phase where we
> > call LockUndoBuffers (or something like that) before
> > InsertPreparedUndo and after PrepareUndoInsert. The LockUndoBuffers
> > will lock all the buffers pinned during PrepareUndoInsert. We can
> > probably call LockUndoBuffers before entering the critical section to
> > avoid any kind of failure in critical section. If so, that sounds
> > reasonable to me.
>
> I'm kind of scratching my head here, because this is clearly different
> than what Andres said in the quoted text to which you were replying.
> He clearly implied that we should acquire the buffer locks within the
> critical section during InsertPreparedUndo, and you responded by
> proposing to do it outside the critical section in a separate step.
> Regardless of which way is actually better, when somebody says "hey,
> let's do A!" and you respond by saying "sounds good, I'll go implement
> B!" that's not really helping us to get toward a solution.
>

I got confused by the statement "We can still hold all of the page
locks at the same time, as long as we just acquire them at the later
stage."

> FWIW, although I also thought of doing what you are describing here, I
> think Andres's proposal is probably preferable, because it's simpler.
> There's not really any reason why we can't take the buffer locks from
> within the critical section, and that way callers don't have to deal
> with the extra step.
>

IIRC, the reason this was done before starting critical section was
because of coding convention mentioned in src/access/transam/README
(Section: Write-Ahead Log Coding). It says first pin and exclusive
lock the shared buffers and then start critical section. It might be
that we can bypass that convention here, but I guess it is mainly to
avoid any error in the critical section. I have checked the
LWLockAcquire path and there doesn't seem to be any reason that it
will throw error except when the caller has acquired many locks at the
same time which is not the case here.

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