On Mon, Jun 17, 2019 at 6:03 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> The idea was that it could be use for multiple purposes like 'rolling
> back complete xact', 'rolling back subxact', 'rollback at page-level'
> or any similar future need even though not all code paths use that
> function. I am not wedded to any particular name here, but among your
> suggestions complete_transaction sounds better to me. Are you okay
> going with that?
Sure, let's try that for now and see how it looks. We can always
change it again if it seems to be a good idea later.
> It won't change in between because we have ensured at top-level that
> no two processes can start executing pending undo at the same time.
> Basically, anyone wants to execute the undo actions will have an entry
> in rollback hash table and that will be marked as in-progress. As
> mentioned in comments, the race is only "after discard worker
> fetches the record and found that this transaction need to be rolled
> back, backend might concurrently execute the actions and remove the
> request from rollback hash table."
>
> [ discussion of alternatives ]
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.
> 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... and there should be
some test framework that verifies the behavior when the return value
is false.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company