Re: Undo logs

On Mon, Nov 26, 2018 at 2:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 20, 2018 at 7:37 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > Along with that I have merged latest changes in zheap branch committed
> > by Rafia Sabih for cleaning up the undo buffer information in abort
> > path.
> >
>
> Thanks, few more comments:
>
> 1.
> @@ -2627,6 +2653,7 @@ AbortTransaction(void)
> AtEOXact_HashTables(false);
> AtEOXact_PgStat(false);
> AtEOXact_ApplyLauncher(false);
> + AtAbort_ResetUndoBuffers();
>
> Don't we need similar handling in AbortSubTransaction?

Yeah we do. I have fixed.
>
> 2.
> Read undo record header in by calling UnpackUndoRecord, if the undo
> + * record header is splited across buffers then we need to read the complete
> + * header by invoking UnpackUndoRecord multiple times.
>
> /splited/splitted. You can just use split here.
Fixed
>
> 3.
> +/*
> + * Identifying information for a transaction to which this undo belongs.
> + * it will also store the total size of the undo for this transaction.
> + */
> +typedef struct UndoRecordTransaction
> +{
> + uint32 urec_progress; /* undo applying progress. */
> + uint32 urec_xidepoch; /* epoch of the current transaction */
> + Oid urec_dbid; /* database id */
> + uint64 urec_next; /* urec pointer of the next transaction */
> +} UndoRecordTransaction;
>
> /it will/It will.
> BTW, which field(s) in the above structure stores the size of the undo?
Fixed

>
> 4.
> + /*
> + * Set uur_info for an UnpackedUndoRecord appropriately based on which
> + * fields are set and calculate the size of the undo record based on the
> + * uur_info.
> + */
>
> Can we rephrase it as "calculate the size of the undo record based on
> the info required"?

Fixed
>
> 5.
> +/*
> + * Unlock and release undo buffers. This step performed after exiting any
> + * critical section.
> + */
> +void
> +UnlockReleaseUndoBuffers(void)
>
> Can we change the later sentence as "This step is performed after
> exiting any critical section where we have performed undo action."?

Done, I mentioned This step is performed after exiting any critical
section where we have prepared undo record.
>
> 6.
> +InsertUndoRecord()
> {
> ..
> + Assert (uur->uur_info != 0);
>
> Add a comment above Assert "The undo record must contain a valid information."
Done
>
> 6.
> +UndoRecordAllocateMulti(UnpackedUndoRecord *undorecords, int nrecords,
> + UndoPersistence upersistence, TransactionId txid)
> {
> ..
> + first_rec_in_recovery = InRecovery && IsTransactionFirstRec(txid);
> +
> + if ((!InRecovery && prev_txid[upersistence] != txid) ||
> + first_rec_in_recovery)
> + {
> + need_start_undo = true;
> + }
>
> Here, I think we can avoid using two boolean variables
> (first_rec_in_recovery and need_start_undo). Also, this same check is
> used in this function twice. I have tried to simplify it in the
> attached. Can you check and let me know if that sounds okay to you?

I have taken your changes
>
> 7.
> UndoRecordAllocateMulti
> {
> ..
> /*
> + * If this is the first undo record of the transaction then initialize
> + * the transaction header fields of the undorecord. Also, set the flag
> + * in the uur_info to indicate that this record contains the transaction
> + * header so allocate the space for the same.
> + */
> + if (need_start_undo && i == 0)
> + {
> + urec->uur_next = InvalidUndoRecPtr;
> + urec->uur_xidepoch = GetEpochForXid(txid);
> + urec->uur_progress = 0;
> +
> + /* During recovery, Fetch database id from the undo log state. */
> + if (InRecovery)
> + urec->uur_dbid = UndoLogStateGetDatabaseId();
> + else
> + urec->uur_dbid = MyDatabaseId;
> +
> + /* Set uur_info to include the transaction header. */
> + urec->uur_info |= UREC_INFO_TRANSACTION;
> + }
> ..
> }
>
> It seems here you have written the code in your comments. I have
> changed it in the attached delta patch.

Taken you changes
>
> 8.
> UndoSetPrepareSize(int max_prepare, UnpackedUndoRecord *undorecords,
> + TransactionId xid, UndoPersistence upersistence)
> +{
> ..
> ..
> + multi_prep_urp = UndoRecordAllocateMulti(undorecords, max_prepare,
>
> Can we rename this variable as prepared_urec_ptr or prepared_urp?
Done
>
> 9.
> +void
> +UndoSetPrepareSize(int max_prepare,
>
> I think it will be better to use nrecords instead of 'max_prepare'
> similar to how you have it in UndoRecordAllocateMulti()
Done
>
> 10.
> + if (!UndoRecPtrIsValid(multi_prep_urp))
> + urecptr = UndoRecordAllocateMulti(urec, 1, upersistence, txid);
> + else
> + urecptr = multi_prep_urp;
> +
> + size = UndoRecordExpectedSize(urec);
> ..
> ..
> + if (UndoRecPtrIsValid(multi_prep_urp))
> + {
> + UndoLogOffset insert = UndoRecPtrGetOffset(multi_prep_urp);
> + insert = UndoLogOffsetPlusUsableBytes(insert, size);
> + multi_prep_urp = MakeUndoRecPtr(UndoRecPtrGetLogNo(urecptr), insert);
> + }
>
> Can't we use urecptr instead of multi_prep_urp in above code after
> urecptr is initialized?

I think we can't because urecptr is just the current pointer we are
going to return but multi_prep_urp is the static variable we need to
update so that
the next prepare can calculate urecptr from this location.
>
> 11.
> +static int max_prepare_undo = MAX_PREPARED_UNDO;
>
> Let's change the name of this variable as max_prepared_undo. Already
> changed in attached delta patch

ok
>
> 12.
> PrepareUndoInsert()
> {
> ..
> + /* Already reached maximum prepared limit. */
> + if (prepare_idx == max_prepare_undo)
> + return InvalidUndoRecPtr;
> ..
> }
>
> I think in the above condition, we should have elog, otherwise,
> callers need to be prepared to handle it.
Done
>
> 13.
> UndoRecordAllocateMulti()
>
> How about naming it as UndoRecordAllocate as this is used to allocate
> even a single undo record?
Done
>
> 14.
> If not already done, can you pgindent the new code added by this patch?

Done
>
> Attached is a delta patch on top of your previous patch containing
> some fixes as memtioned above and few other minor changes and cleanup.
> If you find changes okay, kindly include them in your next version.
I have taken your changes.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com