|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|
|Views:||Raw Message | Whole Thread | Download mbox|
On Tue, Jan 8, 2019 at 2:11 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Few more comments:
> Few comments:
> + * 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?
> + * This function sets uur->uur_info as a side effect.
> + */
> +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
> + 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.
> + * '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.
> +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 ? Am, I correct, if so, I think we should postpone it for
> + * SetCurrentUndoLocation
> + */
> +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.
|Next Message||Pavel Stehule||2019-01-09 06:11:03||Re: proposal: variadic argument support for least, greatest function|
|Previous Message||Surafel Temesgen||2019-01-09 06:09:33||Re: FETCH FIRST clause PERCENT option|