Re: Undo logs - Mailing list pgsql-hackers

On Wed, Dec 12, 2018 at 3:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 12, 2018 at 11:18 AM Dilip Kumar
> <dilip.kumar@enterprisedb.com> wrote:
> >
> > On Sat, Dec 8, 2018 at 7:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >>
> >>
> >> I think I see the problem in the discard mechanism when the log is
> >> spread across multiple logs.  Basically, if the second log contains
> >> undo of some transaction prior to the transaction which has just
> >> decided to spread it's undo in the chosen undo log, then we might
> >> discard the undo log of some transaction(s) inadvertently.  Am, I
> >> missing something?
> >
> > Actually, I don't see exactly this problem here because we only process one undo log at a time, so we will not got
tothe next undo log and discard some transaction for which we supposed to retain the undo. 
> >
>
> How will rollbacks work for such a case?  I have more to say about
> this, see below.
Yeah, I agree rollback will have the problem.

>
> >>
> >> If not, then I guess we need to ensure that we
> >> don't immediately discard the undo in the second log when a single
> >> transactions undo is spreaded across two logs
> >>
> >> Before choosing a new undo log to span the undo for a transaction, do
> >> we ensure that it is not already linked with some other undo log for a
> >> similar reason?
> >>
>
> You seem to forget answering this.
In current patch I have removed the concept of prevlogno in undo log meta.
>
> >> One more thing in this regard:
> >> +UndoRecordAllocate(UnpackedUndoRecord *undorecords, int nrecords,
> >> +    TransactionId txid, UndoPersistence upersistence)
> ..
> >>
> >> Isn't there a hidden assumption in the above code that you will always
> >> get a fresh undo log if the undo doesn't fit in the currently attached
> >> log?  What is the guarantee of same?
> >
> >
> > Yeah it's a problem, we might get the undo which may not be empty.  One way to avoid this could be that instead of
relyingon  the check "UndoRecPtrGetOffset(urecptr) == UndoLogBlockHeaderSize". We can add some flag to the undo log
metadata that whether it's the first record after attach or not and based on that we can decide.  But, I want to think
somebetter solution where we can identify without adding anything extra to undo meta. 
> >
>
> I think what we need to determine here is whether we have switched the
> log for some non-first record of the transaction.  If so, can't we
> detect by something like:
>
> log = GetLatestUndoLogAmAttachedTo();
> UndoLogAllocate();
> if (!need_xact_hdr)
> {
> currnet_log = GetLatestUndoLogAmAttachedTo();
> if (currnet_log is not same as log)
> {
> Assert(currnet_log->meta.prevlogno == log->logno);
> log_switched = true;
> }
> }
>
>
> Won't the similar problem happens when we read undo records during
> rollback?  In code below:
>
> +UndoRecPtr
> +UndoGetPrevUndoRecptr(UndoRecPtr urp, uint16 prevlen)
> +{
> + UndoLogNumber logno = UndoRecPtrGetLogNo(urp);
> + UndoLogOffset offset = UndoRecPtrGetOffset(urp);
> +
> + /*
> + * We have reached to the first undo record of this undo log, so fetch the
> + * previous undo record of the transaction from the previous log.
> + */
> + if (offset == UndoLogBlockHeaderSize)
> + {
> + UndoLogControl *prevlog,
>
> We seem to be assuming here that the new log starts from the
> beginning.  IIUC, we can read the record of some other transaction if
> the transaction's log spans across two logs and the second log
> contains some other transaction's log in the beginning.

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.

>
> >>
> >> 8.
> >> +typedef struct UndoRecordTransaction
> >> +{
> >> + uint32 urec_progress; /* undo applying progress. */
> >> + uint32 urec_xidepoch; /* epoch of the current transaction */
> >>
> >> Can you expand comments about how the progress is defined and used?
> >
> > Moved your comment from UnpackedUndoRecord to this structure and in UnpackedUndoRecord I have mentioned that we can
referdetailed comment in this structure. 
> >
> >>
> >> Also, write a few sentences about why epoch is captured and or used?
> >
> > urec_xidepoch is captured mainly for the zheap visibility purpose so isn't it good that we mention it there?
> >
>
> Okay, you can leave it here as it is.  One small point about this structure:
>
> + 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.

Handling multilog, needed some changes in undo-log-manager patch so
attaching the updated version of the undo-log patches as well.

Commit on which patches created (bf491a9073e12ce1fc3e6facd0ae1308534df570)

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