On Mon, Nov 26, 2018 at 2:13 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Nov 20, 2018 at 7:37 PM Dilip Kumar <dilipbal...@gmail.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
0003-undo-interface-v9.patch
Description: Binary data
