Re: POC: Cleaning up orphaned files using undo logs

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(dot)kapila(at)enterprisedb(dot)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. 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

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.

> +/* 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? This should all go through FullTransactionId.

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

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

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

Greetings,

Andres Freund