Re: Undo logs

On Sat, Jan 5, 2019 at 11:29 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> 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?.
>

No, you are correct.

Few more comments:
--------------------------------
Few comments:
----------------
1.
+ * undorecord.c
+ * encode and decode undo records
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group

Change the year in Copyright notice for all new files?

2.
+ * This function sets uur->uur_info as a side effect.
+ */
+bool
+InsertUndoRecord(UnpackedUndoRecord *uur, Page page,
+ int starting_byte, int *already_written, bool header_only)

Is the above part of comment still correct? I don't see uur_info being set here.

3.
+ work_txn.urec_next = uur->uur_next;
+ work_txn.urec_xidepoch = uur->uur_xidepoch;
+ work_txn.urec_progress = uur->uur_progress;
+ work_txn.urec_prevurp = uur->uur_prevurp;
+ work_txn.urec_dbid = uur->uur_dbid;

It would be better if we initialize these members in the order in
which they appear in the actual structure. All other undo header
structures are initialized that way, so this looks out-of-place.

4.
+ * 'my_bytes_written' is a pointer to the count of previous-written bytes
+ * from this and following structures in this undo record; that is, any
+ * bytes that are part of previous structures in the record have already
+ * been subtracted out. We must update it for the bytes we write.
+ *
..
+static bool
+InsertUndoBytes(char *sourceptr, int sourcelen,
+ char **writeptr, char *endptr,
+ int *my_bytes_written, int *total_bytes_written)
+{
..
+
+ /* Update bookkeeeping infrormation. */
+ *writeptr += can_write;
+ *total_bytes_written += can_write;
+ *my_bytes_written = 0;

I don't understand the above comment where it is written: "We must
update it for the bytes we write.". We always set 'my_bytes_written'
as 0 if we write. Can you clarify? I guess this part of the comment
is about total_bytes_written or here does it mean to say that caller
should update it. I think some wording change might be required based
on what we intend to say here.

Similar to above, there is a confusion in the description of
my_bytes_read atop ReadUndoBytes.

5.
+uint32
+GetEpochForXid(TransactionId xid)
{
..
+ /*
+ * Xid can be on either side when near wrap-around. Xid is certainly
+ * logically later than ckptXid.
..

From the usage of this function in the patch, can we say that Xid is
always later than ckptxid, if so, how? Also, I think you previously
told in this thread that usage of uur_xidepoch is mainly for zheap, so
we might want to postpone the inclusion of this undo records. On
thinking again, I think we should follow your advice as I think the
correct usage here would require the patch by Thomas to fix our epoch
stuff [1]? Am, I correct, if so, I think we should postpone it for
now.

6.
/*
+ * SetCurrentUndoLocation
+ */
+void
+SetCurrentUndoLocation(UndoRecPtr urec_ptr)
{
..
}

I think you can use some comments atop this function to explain the
usage of this function or how will callers use it.

I am done with the first level of code-review for this patch. I am
sure we might need few interface changes here and there while
integrating and testing this with other patches, but the basic idea
and code look reasonable to me. I have modified the proposed commit
message in the attached patch, see if that looks fine to you.

To be clear, this patch can't be independently committed/tested, we
need undo logs and undo worker machinery patches to be ready as well.
I will review those next.

[1] - https://www.postgresql.org/message-id/CAEepm%3D2YYAtkSnens%3DTR2S%3DoRcAF9%3D2P7GPMK0wMJtxKF1QRig%40mail.gmail.com

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