Re: Undo logs

From: Dilip Kumar <dilipbalaut(at)gmail(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Dilip Kumar <dilip(dot)kumar(at)enterprisedb(dot)com>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Undo logs
Date: 2019-01-05 05:59:05
Message-ID: CAFiTN-sa4T_C+uvZstsa=_LYkt0owg7ctsKuUWLPH29shA=1_A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0002-Provide-access-to-undo-log-data-via-the-buffer-manag.patch application/octet-stream 40.3 KB
0001-Add-undo-log-manager.patch application/octet-stream 116.5 KB
0003-undo-interface-v13.patch application/octet-stream 69.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Steele 2019-01-05 06:32:55 Re: Add timeline to partial WAL segments
Previous Message Michael Paquier 2019-01-05 01:31:07 Re: Add timeline to partial WAL segments