Re: POC: Cleaning up orphaned files using undo logs

On Tue, May 21, 2019 at 10:47 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-05 10:28:21 +0530, Amit Kapila wrote:
> > From 5d9e179bd481b5ed574b6e7117bf3eb62b5dc003 Mon Sep 17 00:00:00 2001
> > From: Amit Kapila <amit.kapila@enterprisedb.com>
> > Date: Sat, 4 May 2019 16:52:01 +0530
> > Subject: [PATCH] Allow undo actions to be applied on rollbacks and discard
> >  unwanted undo.
>
> I think this needs to be split into some constituent parts, to be
> reviewable.

Okay.

> Discussing 270kb of patch at once is just too much. My first
> guess for a viable split would be:
>
> 1) undoaction related infrastructure
> 2) xact.c integration et al
> 3) binaryheap changes etc
> 4) undo worker infrastructure
>
> It probably should be split even further, by moving things like:
> - oldestXidHavingUndo infrastructure
> - discard infrastructure
>

Okay, I will think about this and split the patch.

> Some small remarks:
>
> >
> > +     {
> > +             {"disable_undo_launcher", PGC_POSTMASTER, DEVELOPER_OPTIONS,
> > +                     gettext_noop("Decides whether to launch an undo worker."),
> > +                     NULL,
> > +                     GUC_NOT_IN_SAMPLE
> > +             },
> > +             &disable_undo_launcher,
> > +             false,
> > +             NULL, NULL, NULL
> > +     },
> > +
>
> We don't normally formulate GUCs in the negative like that. C.F.
> autovacuum etc.
>

Okay, will change.  Actually, this is just for development purpose.
It can help us in testing cases where we have pushed the undo, but it
won't apply, so whenever the foreground process encounter such a
transaction, it will perform the page-wise undo.  I am not 100% sure
if we need this for the final version.  Similarly, for testing
purpose, we might need enable_discard_worker to test the cases where
discard doesn't happen for a long time.

>
> > +/* Extract xid from a value comprised of epoch and xid  */
> > +#define GetXidFromEpochXid(epochxid)                 \
> > +     ((uint32) (epochxid) & 0XFFFFFFFF)
> > +
> > +/* Extract epoch from a value comprised of epoch and xid  */
> > +#define GetEpochFromEpochXid(epochxid)                       \
> > +     ((uint32) ((epochxid) >> 32))
> > +
>
> Why do these exist?
>

We don't need the second one (GetEpochFromEpochXid), but the first one
is required.  Basically, the oldestXidHavingUndo computation does
consider oldestXmin (which is still a TransactionId) as we can't
retain undo which is 2^31 transactions old due to other limitations
like clog/snapshots still has a limit of 4-byte transaction ids.
Slightly unrelated, but we do want to improve the undo retention in a
subsequent version such that we won't allow pending undo for
transaction whose age is more than 2^31.

>
> >       /* End-of-list marker */
> >       {
> >               {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
> > @@ -2923,6 +2935,16 @@ static struct config_int ConfigureNamesInt[] =
> >               5000, 1, INT_MAX,
> >               NULL, NULL, NULL
> >       },
> > +     {
> > +             {"rollback_overflow_size", PGC_USERSET, RESOURCES_MEM,
> > +                     gettext_noop("Rollbacks greater than this size are done lazily"),
> > +                     NULL,
> > +                     GUC_UNIT_MB
> > +             },
> > +             &rollback_overflow_size,
> > +             64, 0, MAX_KILOBYTES,
> > +             NULL, NULL, NULL
> > +     },
>
> rollback_foreground_size? rollback_background_size? I don't think
> overflow is particularly clear.
>

How about rollback_undo_size or abort_undo_size or
undo_foreground_size or pending_undo_size?

>
> > @@ -1612,6 +1635,85 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
> >
> >       MyLockedGxact = NULL;
> >
> > +     /*
> > +      * Perform undo actions, if there are undologs for this transaction. We
> > +      * need to perform undo actions while we are still in transaction. Never
> > +      * push rollbacks of temp tables to undo worker.
> > +      */
> > +     for (i = 0; i < UndoPersistenceLevels; i++)
> > +     {
>
> This should be in a separate function. And it'd be good if more code
> between this and ApplyUndoActions() would be shared.
>

makes sense, will try.

>
> > +     /*
> > +      * Here, we just detect whether there are any pending undo actions so that
> > +      * we can skip releasing the locks during abort transaction.  We don't
> > +      * release the locks till we execute undo actions otherwise, there is a
> > +      * risk of deadlock.
> > +      */
> > +     SetUndoActionsInfo();
>
> This function name is so generic that it gives the reader very little
> information about why it's called here (and in other similar places).
>

NeedToPerformUndoActions()? UndoActionsRequired()?

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