Re: POC: Cleaning up orphaned files using undo logs

On Thu, Jun 20, 2019 at 9:56 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 20, 2019 at 11:35 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > IIRC, then you only seem to have suggested that we need a kind of
> > back-off algorithm that gradually increases the retry time up to some
> > maximum [1]. I think that is a good way to de-prioritize requests
> > that are repeatedly failing. Say, there is a request that has already
> > failed for 5 times and the worker queues it to get executed after 10s.
> > Immediately after that, another new request has failed for the first
> > time for the same database and it also got queued to get executed
> > after 10s. In this scheme the request that has already failed for 5
> > times will get a chance before the request that has failed for the
> > first time.
>
> Sure, that's an advantage of increasing back-off times -- you can keep
> the stuff that looks hopeless from interfering too much with the stuff
> that is more likely to work out. However, I don't think we've actually
> done enough testing to know for sure what algorithm will work out
> best. Do we want linear back-off (10s, 20s, 30s, ...)? Exponential
> back-off (1s, 2s, 4s, 8s, ...)? No back-off (10s, 10s, 10s, 10s)?
> Some algorithm that depends on the size of the failed transaction, so
> that big things get retried less often? I think it's important to
> design the code in such a way that the algorithm can be changed easily
> later, because I don't think we can be confident that whatever we pick
> for the first attempt will prove to be best. I'm pretty sure that
> storing the failure count INSTEAD OF the next retry time is going to
> make it harder to experiment with different algorithms later.
>

Fair enough. I have implemented it based on next_retry_at and use
constant time 10s for the next retry. I have used define instead of a
GUC as all the other constants for similar things are defined as of
now. One thing to note is that we want the linger time (defined as
UNDO_WORKER_LINGER_MS) for a undo worker to be more than failure retry
time (defined as UNDO_FAILURE_RETRY_DELAY_MS) as, otherwise, the undo
worker can exit before retrying the failed requests. The changes for
this are in patches
0011-Infrastructure-to-register-and-fetch-undo-action-req.patch and
0014-Allow-execution-and-discard-of-undo-by-background-wo.patch.

Apart from these, there are few other changes in the patch series:

0014-Allow-execution-and-discard-of-undo-by-background-wo.patch:
1. Allow the undo workers to respond to cancel command by the user.
CHECK_FOR_INTERRUPTS was missing while the worker was checking for the
next undo request in a loop.
2. Change the value of UNDO_WORKER_LINGER_MS to 20s, so that it is
more than UNDO_FAILURE_RETRY_DELAY_MS.
3. Handled Sigterm signal for undo launcher and workers
4. Fixed the code bug to avoid having CommitTransaction when one of
the workers fails to register. There is no StartTransaction to match
the same. This was leftover from the previous approach.

0012-Infrastructure-to-execute-pending-undo-actions.patch
1 Fix compiler warning

0007-Provide-interfaces-to-store-and-fetch-undo-records.patch
1. Fixed a bug to unlock the buffer while resetting the undo unpacked record
2. Fixed the spurious release of the lock in UndoFetchRecord.
3. Remove the pointer to previous undo in a different log from
UndoRecordTransaction structure. Now, a separate low_switch header
contains the same.

0007-Provide-interfaces-to-store-and-fetch-undo-records.patch is
Dilip's patch and he has modified it, but changes were small so there
was not much sense in posting it separately.

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