Re: Undo logs

On Tue, Jan 1, 2019 at 4:37 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> Thanks, the new changes look mostly okay to me, but I have few comments:
> 1.
> + /*
> + * WAL log, for log switch. This is required to identify the log switch
> + * during recovery.
> + */
> + if (!InRecovery && log_switched && upersistence == UNDO_PERMANENT)
> + {
> + XLogBeginInsert();
> + XLogRegisterData((char *) &prevlogurp, sizeof(UndoRecPtr));
> + XLogInsert(RM_UNDOLOG_ID, XLOG_UNDOLOG_SWITCH);
> + }
> +
>
> Don't we want to do this under critical section?
I think we are not making any buffer changes here and just inserting a
WAL, IMHO we don't need any critical section. Am I missing
something?.

>
> 2.
> +UndoRecordAllocate(UnpackedUndoRecord *undorecords, int nrecords,
> + TransactionId txid, UndoPersistence upersistence)
> {
> ..
> + if (log_switched)
> + {
> + /*
> + * If undo log is switched then during rollback we can not go
> + * to the previous undo record of the transaction by prevlen
> + * so we store the previous undo record pointer in the
> + * transaction header.
> + */
> + log = UndoLogGet(prevlogno, false);
> + urec->uur_prevurp = MakeUndoRecPtr(prevlogno,
> + log->meta.insert -
> log->meta.prevlen);
> + }
> ..
> }
>
> Can we have an Assert for a valid prevlogno in the above condition?

Done
>
> > > + uint64 urec_next; /* urec pointer of the next transaction */
> > > +} UndoRecordTransaction;
> > > +
> > > +#define SizeOfUrecNext (sizeof(UndoRecPtr))
> > > +#define SizeOfUndoRecordTransaction \
> > > + (offsetof(UndoRecordTransaction, urec_next) + SizeOfUrecNext)
> > >
> > > Isn't it better to define urec_next as UndoRecPtr, even though it is
> > > technically the same as per the current code?
> >
> > While replying I noticed that I haven't address this comment, I will
> > handle this in next patch. I have to change this at couple of place.
> >
>
> Okay, I think the new variable (uur_prevurp) introduced by this
> version of the patch also needs to be changed in a similar way.

DOne
>
> Apart from the above, I have made quite a few cosmetic changes and
> modified few comments, most notably, I have updated the comments
> related to the handling of multiple logs at the beginning of
> undoinsert.c file. Kindly, include these changes in your next
> patchset, if they look okay to you.
>
I have taken all changes except this one

if (xact_urec_info_idx > 0)
{
- int i = 0;
+ int i = 0; --> pgindent changed it back to the above one.

for (i = 0; i < xact_urec_info_idx; i++)
- UndoRecordUpdateTransInfo(i);
+ UndoRecordUpdateTransInfo(i); -- This induce extra space so I ignored this
}

Undo-log patches need rebased so I have done that as well along with
the changes mentioned above.

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