Re: POC: Cleaning up orphaned files using undo logs - Mailing list pgsql-hackers

On Thu, Nov 12, 2020 at 2:45 PM Antonin Houska <ah@cybertec.at> wrote:
>
>
> No background undo
> ------------------
>
> Reduced complexity of the patch seems to be the priority at the moment. Amit
> suggested that cleanup of an orphaned relation file is simple enough to be
> done on foreground and I agree.
>

Yeah, I think we should try and see if we can make it work but I
noticed that there are few places like AbortOutOfAnyTransaction where
we have the assumption that undo will be executed in the background.
We need to deal with it.

> "undo worker" is still there, but it only processes undo requests after server
> restart because relation data can only be changed in a transaction - it seems
> cleaner to launch a background worker for this than to hack the startup
> process.
>

But, I see there are still multiple undoworkers that are getting
launched and I am not sure if that works correctly because a
particular undoworker is connected to a database and then it starts
processing all the pending undo.

> Since the concept of undo requests is closely related to the undo worker, I
> removed undorequest.c too. The new (much simpler) undo worker gets the
> information on incomplete / aborted transactions from the undo log as
> mentioned above.
>
> SMGR enhancement
> ----------------
>
> I used the 0001 patch from [3] rather than [4], although it's more invasive
> because I noticed somewhere in the discussion that there should be no reserved
> database OID for the undo log. (InvalidOid cannot be used because it's already
> in use for shared catalogs.)
>
> Components added
> ----------------
>
> pg_undo_dump utility and test framework for undoread.c. BTW, undoread.c seems
> to need some refactoring.
>
>
> Following are a few areas which are not implemented yet because more
> discussion is needed there:
>
> Discarding
> ----------
>
> There's no discard worker for the URS infrastructure yet. I thought about
> discarding the undo log during checkpoint, but checkpoint should probably do
> more straightforward tasks than the calculation of a new discard pointer for
> each undo log, so a background worker is needed. A few notes on that:
>
>   * until the zheap AM gets added, only the transaction that creates the undo
>     records needs to access them. This assumption should make the discarding
>     algorithm a bit simpler. Note that with zheap, the other transactions need
>     to look for old versions of tuples, so the concept of oldestXidHavingUndo
>     variable is needed there.
>
>   * it's rather simple to pass pointer the URS pointer to the discard worker
>     when transaction either committed or the undo has been executed.
>

Why can't we have a separate discard worker which keeps on scanning
the undorecords and discard accordingly? Giving the onus of foreground
process might be tricky because say discard worker is not up to speed
and we ran out of space to pass such information for each commit/abort
request.

>
> Do not execute the same undo record multiple times
> --------------------------------------------------
>
> Although I've noticed in the zheap code that it checks whether particular undo
> action was already undone, I think this functionality fits better in the URS
> layer.
>

If you want to track at undo record level, then won't it lead to
performance overhead and probably additional WAL overhead considering
this action needs to be WAL-logged. I think recording at page-level
might be a better idea.

>
> I can spend more time on this project, but need a hint which part I should
> focus on.
>

I can easily imagine that this needs a lot of work and I can try to
help with this as much as possible from my side. I feel at this stage
we should try to focus on undo-related work (to start with you can
look at finishing the undoprocessing work for which I have shared some
thoughts) and then probably at some point in time we need to rebase
zheap over this.

-- 
With Regards,
Amit Kapila.