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