On Mon, Jun 17, 2019 at 7:30 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Jun 17, 2019 at 6:03 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I'm not precisely sure what the best thing to do here is, but I'm
> skeptical that the code in question belongs in this function. There
> are two separate things going on here: one is this revalidation that
> the undo hasn't been discarded, and the other is executing the undo
> actions. Those are clearly separate tasks, and they are not tasks that
> always get done together: sometimes we do only one, and sometimes we
> do both. Any function that looks like this is inherently suspicious:
>
> whatever(....., bool flag)
> {
> if (flag)
> {
> // lengthy block of code
> }
>
> // another lengthy block of code
> }
>
> There has to be a reason not to just split this into two functions and
> let the caller decide whether to call one or both.
>
Yeah, because some of the information required to perform the
necessary steps (in the code under the flag) is quite central to this
function (see undo apply progress update part) and it is used at more
than one place in this function. I have refactored the code in this
function, see if it makes sense now. You need to check patch
0012-Infrastructure-to-execute-pending-undo-actions.patch for these
changes.
> > For Error case, it is fine to report failure, but there can be cases
> > where we don't need to apply undo actions like when the relation is
> > dropped/truncated, undo actions are already applied. The original
> > idea was to cover such cases by the return value. I agree that
> > currently, caller ignores this value, but there is some value in
> > keeping it. So, I am in favor of a signature with bool as the return
> > value.
>
> OK. So then the callers can't keep ignoring it...
>
I again thought about this but couldn't come up with anything
meaningful. The idea is to ignore some undo records if they belong to
the same relation which is already gone. I think we can do something
about it in zheap specific code and make the generic code return void.
I have fixed the other comments raised by you. See
0012-Infrastructure-to-execute-pending-undo-actions.patch
Apart from the changes related to the undo apply, this patch series
contains changes for making the transaction header at a location
immediately after
UndoRecordHeader which makes it easy to update the same. The changes
are in patches 0007-Provide-interfaces-to-store-and-fetch-undo-records.patch
and 0012-Infrastructure-to-execute-pending-undo-actions.patch.
There are no changes in undo log module patches.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com