Re: POC: Cleaning up orphaned files using undo logs

On Fri, Jul 26, 2019 at 9:57 PM Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>
> On Fri, 26 Jul 2019 at 12:25, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I agree with all your other comments.
>
> Thanks for addressing the comments. Below is the continuation of my
> comments from 0014-Allow-execution-and-discard-of-undo-by-background-wo.patch
> :
>
>
> + * Perform rollback request.  We need to connect to the database for first
> + * request and that is required because we access system tables while
> for first request and that is required => for the first request. This
> is required
>

Changed as per suggestion.

> ---------------
>
> +UndoLauncherShmemSize(void)
> +{
> +    Size        size;
> +
> +    /*
> +     * Need the fixed struct and the array of LogicalRepWorker.
> +     */
> +    size = sizeof(UndoApplyCtxStruct);
>
> The fixed structure size should be  offsetof(UndoApplyCtxStruct,
> workers) rather than sizeof(UndoApplyCtxStruct)
>
> ---------------
>

Why? I see the similar code in ApplyLauncherShmemSize.  If there is
any problem with this, then we might have similar problem in existing
code as well.

> In UndoWorkerCleanup(), we set individual fields of the
> UndoApplyWorker structure, whereas in UndoLauncherShmemInit(), for all
> the UndoApplyWorker array elements, we just memset all the
> UndoApplyWorker structure elements to 0. I think we should be
> consistent for the two cases. I guess we can just memset to 0 as you
> do in UndoLauncherShmemInit(), but this will cause the
> worker->undo_worker_queue to be 0 i.e. XID_QUEUE , whereas in
> UndoWorkerCleanup(), it is set to -1. Is the -1 value essential, or we
> can just set it to XID_QUEUE initially ?

Either is fine, because before launching the worker we set the valid
value.  It is better to set it as InvalidUndoWorkerQueue though.

> Also, if we just use memset in UndoWorkerCleanup(), we need to first
> save generation into a temp variable, and then after memset(), restore
> it back.
>

This sounds like an unnecessary trickery.

> That brought me to another point :
> We already have a macro ResetUndoRequestInfo(), so UndoWorkerCleanup()
> can just call ResetUndoRequestInfo().
> ------------
>

Hmm, both (UndoRequestInfo and UndoApplyWorker) are separate
structures, so how can we reuse them?

> +        bool        allow_peek;
> +
> +        CHECK_FOR_INTERRUPTS();
> +
> +        allow_peek = !TimestampDifferenceExceeds(started_at,
> Some comments would be good about what is allow_peek  used for. Something like :
> "Arrange to prevent the worker from restarting quickly to switch databases"
>

Added a slightly different comment.

> -----------------
> +++ b/src/backend/access/undo/README.UndoProcessing
> -----------------
>
> +worker then start reading from one of the queues the requests for that
> start=>starts
> ---------------
>
> +work, it lingers for UNDO_WORKER_LINGER_MS (10s as default).  This avoids
> As per the latest definition, it is 20s. IMHO, there's no need to
> mention the default value in the readme.
> ---------------
>
> +++ b/src/backend/access/undo/discardworker.c
> ---------------
>
> + * portion of transaction that is overflowed into a separate log can
> be processed
> 80-col crossed.
>
> +#include "access/undodiscard.h"
> +#include "access/discardworker.h"
> Not in alphabetical order
>

Fixed all of the above four points.

>
> +++ b/src/backend/access/undo/undodiscard.c
> ---------------
>
> +        next_insert = UndoLogGetNextInsertPtr(logno);
> I checked UndoLogGetNextInsertPtr() definition. It calls
> find_undo_log_slot() to get back the slot from logno. Why not make it
> accept slot as against logno ? At all other places, the slot->logno is
> passed, so it is convenient to just pass the slot there. And in
> UndoDiscardOneLog(), first call find_undo_log_slot() just before the
> above line (or call it at the end of the do-while loop).
>

I am not sure if this is a good idea because find_undo_log_slot is
purely a undolog module stuff, exposing it to outside doesn't seem
like a good idea to me.

> This way,
> during each of the UndoLogGetNextInsertPtr() calls in undorequest.c,
> we will have one less find_undo_log_slot() call.
>

I am not sure there is any performance benefit either because there is
a cache for the slots and it should get from there very quickly.  I
think we can avoid this repeated call and I have done so in the
attached patch.

> -------------
>
> In UndoDiscardOneLog(), there are at least 2 variable declarations
> that can be moved inside the do-while loop : uur and next_insert. I am
> not sure about the other variables viz : undofxid and
> latest_discardxid. Values of these variables in one iteration continue
> across to the second iteration. For latest_discardxid, it looks like
> we do want its value to be carried forward, but is it also true for
> undofxid ?
>

undofxid can be moved inside the loop, fixed that and other variables
pointed out by you.

> + /* If we reach here, this means there is something to discard. */
> +     need_discard = true;
> + } while (true);
>
> Also, about need_discard; there is no place where need_discard is set
> to false. That means, from 2nd iteration onwards, it will never be
> false. So even if the code that explicitly sets need_discard to true
> does not get run, still the undolog will be discarded. Is this
> expected ?
> -------------

Yes.  We will discard once we have even one transaction data to
discard.  For ex., say we decided that we can discard data for
transaction id 501, and then next transaction 502 is aborted and the
actions for same are not yet applied, in that case, we will discard
the data of transaction 501.  I hope this answers your question.

>
> +            if (request_rollback && dbid_exists(uur->uur_txn->urec_dbid))
> +            {
> +                (void) RegisterRollbackReq(InvalidUndoRecPtr,
> +                                           undo_recptr,
> +                                           uur->uur_txn->urec_dbid,
> +                                           uur->uur_fxid);
> +
> +                pending_abort = true;
> +            }
> We can get rid of request_rollback variable. Whatever the "if" block
> above is doing, do it in this upper condition :
> if (!IsXactApplyProgressCompleted(uur->uur_txn->urec_progress))
>
> Something like this :
>
> if (!IsXactApplyProgressCompleted(uur->uur_txn->urec_progress))
> {
>     if (dbid_exists(uur->uur_txn->urec_dbid))
>     {
>         (void) RegisterRollbackReq(InvalidUndoRecPtr,
>                                    undo_recptr,
>                                    uur->uur_txn->urec_dbid,
>                                    uur->uur_fxid);
>
>        pending_abort = true;
>     }
> }

Hmm, you also need to check that transaction is not in-progress along
with it.  I think there will be more movements of checks and that will
make code look less readable than it is now.

> -------------
>
> +            UndoRecordRelease(uur);
> +            uur = NULL;
> +        }
> .....
> .....
> +        Assert(uur == NULL);
> +
> +        /* If we reach here, this means there is something to discard. */
> +        need_discard = true;
> +    } while (true);
>
> Looks like it is neither necessary to set uur to NULL, nor is it
> necessary to have the Assert(uur == NULL). At the start of each
> iteration uur is anyway assigned a fresh value, which may or may not
> be NULL.
> -------------
>

I think there is no harm in doing it what you are saying, but the idea
here is to prevent the release of undo record.  Basically, if we have
fetched a valid undo record, then it must be released.  I understand
this is not a bullet-proof Assert, because one might set it to NULL
without actually releasing the memory.  For now, I have added a
comment before Assert, see if that makes sense?

> + * over undo logs is complete, new undo can is allowed to be written in the
> new undo can is allowed => new undo is allowed
>
> + * hash table size.  So before start allowing any new transaction to write the
> before start allowing => before allowing any new transactions to start
> writing the
> -------------
>

Changed as per suggestion.

> +    /* Get the smallest of 'xid having pending undo' and 'oldestXmin' */
> +    oldestXidHavingUndo = RollbackHTGetOldestFullXid(oldestXidHavingUndo);
> +   ....
> +   ....
> +    if (FullTransactionIdIsValid(oldestXidHavingUndo))
> +        pg_atomic_write_u64(&ProcGlobal->oldestFullXidHavingUnappliedUndo,
> +                            U64FromFullTransactionId(oldestXidHavingUndo));
>
> Is it possible that the FullTransactionId returned by
> RollbackHTGetOldestFullXid() could be invalid ? If not, then the if
> condition above can be changed to an Assert().
> -------------
>

Yeah, it could be changed to Assert.

>
> +         * If the log is already discarded, then we are done.  It is important
> +         * to first check this to ensure that tablespace containing this log
> +         * doesn't get dropped concurrently.
> +         */
> +        LWLockAcquire(&slot->mutex, LW_SHARED);
> +        /*
> +         * We don't have to worry about slot recycling and check the logno
> +         * here, since we don't care about the identity of this slot, we're
> +         * visiting all of them.
> I guess, it's accidental that the LWLockAcquire() call is *between*
> the two comments ?
> -----------
>

I think it is better to have them as a single comment before acquiring
the lock, so changed that way.

> +            if (UndoRecPtrGetCategory(undo_recptr) == UNDO_SHARED)
> +            {
> +                /*
> +                 * For the "shared" category, we only discard when the
> +                 * rm_undo_status callback tells us we can.
> +                 */
> +                status = RmgrTable[uur->uur_rmid].rm_undo_status(uur,
> &wait_xid);
> status variable could be declared in this block itself.
> -------------
>

Thomas mentioned that he is planning to change the implementation of
shared undo logs, so let's keep this as it is for now.

>
> Some variable declaration alignments and comments spacing need changes
> as per pgindent.
>

I have left this for now, will take care of this in the next version.

Thanks, Amit Khandekar for all your review comments.  As per my
knowledge, I have addressed all of your review comments raised so far
related to undo-processing patches.  Do let me know if I have missed
anything?

Please find the latest patches in my email up thread [1].

[1] - https://www.postgresql.org/message-id/CAA4eK1%2BMcY0qGaak0AHyzdgAn%2BF6dyxcpDwp9ifGg%3D1WVDadeQ%40mail.gmail.com

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