Re: Undo logs

On Tue, Jan 8, 2019 at 2:11 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

>
> 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?
>

Done

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

Changed

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

Fixed

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

Fixed

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

Removed

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

Done

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

Make sense

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

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