On Mon, Jul 22, 2019 at 3:51 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jul 22, 2019 at 2:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> I have reviewed 0012-Infrastructure-to-execute-pending-undo-actions,
> Please find my comment so far.
>
> 1.
> + /* It shouldn't be discarded. */
> + Assert(!UndoRecPtrIsDiscarded(xact_urp));
>
> I think comments can be added to explain why it shouldn't be discarded.
>
> 2.
> + /* Compute the offset of the uur_next in the undo record. */
> + offset = SizeOfUndoRecordHeader +
> + offsetof(UndoRecordTransaction, urec_progress);
> +
> in comment /uur_next/uur_progress
>
> 3.
> +/*
> + * undo_record_comparator
> + *
> + * qsort comparator to handle undo record for applying undo actions of the
> + * transaction.
> + */
> Function header formating is not in sync with other functions.
>
Fixed all the above comments in the attached patch.
> 4.
> +void
> +undoaction_redo(XLogReaderState *record)
> +{
> + uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
> +
> + switch (info)
> + {
> + case XLOG_UNDO_APPLY_PROGRESS:
> + undo_xlog_apply_progress(record);
> + break;
>
> For HotStandby it doesn't make sense to apply this wal as this
> progress is only required when we try to apply the undo action after
> restart
> but in HotStandby we never apply undo actions.
>
I have already responded in my earlier email on why this is required [1].
> 5.
> + Assert(from_urecptr != InvalidUndoRecPtr);
> + Assert(to_urecptr != InvalidUndoRecPtr);
>
> we can use macros UndoRecPtrIsValid instead of checking like this.
>
Fixed.
> 6.
> + if ((slot == NULL) || (UndoRecPtrGetLogNo(urecptr) != slot->logno))
> + slot = UndoLogGetSlot(UndoRecPtrGetLogNo(urecptr), false);
> +
> + Assert(slot != NULL);
> We are passing missing_ok as false in UndoLogGetSlot. But, not sure
> why we are expecting that undo lot can not be dropped. In multi-log
> transaction it's possible
> that the tablespace in which next undolog is there is already dropped?
>
Already responded on this in my earlier reply [1].
> 7.
> + */
> + do
> + {
> + BlockNumber progress_block_num = InvalidBlockNumber;
> + int i;
> + int nrecords;
> .....
> + */
> + if (!UndoRecPtrIsValid(urec_ptr))
> + break;
> + } while (true);
>
> I think we can convert above loop to while(true) instead of do..while,
> because there is no need for do while loop.
>
> 8.
> + if (last_urecinfo->uur->uur_info & UREC_INFO_LOGSWITCH)
> + {
> + UndoRecordLogSwitch *logswitch = last_urecinfo->uur->uur_logswitch;
>
> IMHO, the caller of UndoFetchRecord should directly check
> uur->uur_logswitch instead of uur_info & UREC_INFO_LOGSWITCH.
> Actually, uur_info is internally set
> for inserting the tuple and check there to know what to insert and
> fetch but I think caller of UndoFetchRecord should directly rely on
> the field because ideally all
> the fields in UnpackUndoRecord must be set and uur_txt or
> uur_logswitch will be allocated when those headers present. I think
> this needs to be improved in undo interface patch
> as well (in UndoBulkFetchRecord).
>
Okay, fixed both of the above. I have exposed a new macro
IsUndoLogSwitched from undorecord.h which you might also want to use
in your patch.
Apart from this, in the attached patches, I have fixed various
comments raised in this thread from Amit Khandekar. I'll respond to
them separately. I have yet to address various comments raised by
Andres and Robert which also includes integration with the latest
patch on queues posted by Robert.
Note - The patches for undo-log and undo-interface has not been
rebased as others are working actively on their branches. The branch
where this code resides can be accessed at
https://github.com/EnterpriseDB/zheap/tree/undoprocessing
[1] - https://www.postgresql.org/message-id/CAA4eK1KoA0L%3DPNBc_uu2v8H0%3DLA_Cm%3Do9GyFm6i6DSD6mUMppg%40mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com