Re: POC: Cleaning up orphaned files using undo logs - Mailing list pgsql-hackers

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