Re: POC: Cleaning up orphaned files using undo logs

On Wed, May 22, 2019 at 4:47 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, May 21, 2019 at 10:47 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> > 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.
>

Changed.

> >
> > > +/* 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.
>

Removed both the above defines.

> >
> > > /* 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?
>

I think we need some more discussion on this before we change as
Robert seems to feel that we should have 'undo' someplace in the name.
Please let me know your
preference.

> >
> > > @@ -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.
>

Done. Now, there is a common function that is used in twophase.c and
ApplyUndoActions.

> >
> > > + /*
> > > + * 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()?
>

Changed to UndoActionsRequired and added comments atop of the function
to make it clear why and when this function needs to use.

Apart from fixing the above comments, the patch is rebased on latest
undo patchset. As of now, I have split the binaryheap.c changes into
a separate patch. We are stilll enhancing the patch to compute
oldestXidHavingUnappliedUndo which touches various parts of patch, so
splitting further without completing that can make it a bit difficult
to work on that.

Pending work
-------------------
1. Enhance uur_progress so that it updates undo action apply progress
at regular intervals.
2. Enhance to support oldestXidHavingUnappliedUndo, more on that later.
3. Split the patch.

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