Re: Undo logs

On Sun, Dec 23, 2018 at 3:49 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Wed, Dec 12, 2018 at 3:03 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> For addressing these issues related to multilog I have changed the
> design as we discussed offlist.
> 1) Now, at Do time we identify the log switch as you mentioned above
> (identify which log we are attached to before and after allocate).
> And, if the log is switched we write a WAL for the same and during
> recovery whenever this WAL is replayed we stored the undo record
> pointer of the transaction header (which is in the previous undo log)
> in UndoLogStateData and read it while allocating space the undo record
> and immediately reset it.
>
> 2) For handling the discard issue, along with updating the current
> transaction's start header in the previous undo log we also update the
> previous transaction's start header in the current log if we get
> assigned with the non-empty undo log.
>
> 3) For, identifying the previous undo record of the transaction during
> rollback (when undo log is switched), we store the transaction's last
> record's (in previous undo log) undo record pointer in the transaction
> header of the first undo record in the new undo log.
>

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?

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?

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

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.

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