Re: Code checks for App Devs, using new options for transaction behavior

On Wed, 2 Nov 2022 at 03:52, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Oct 31, 2022 at 6:54 PM Simon Riggs
> <simon(dot)riggs(at)enterprisedb(dot)com> wrote:
> >
> > > > What is the behavior if "nested_transactions" value is changed within
> > > > a transaction execution, suppose the value was on and we have created
> > > > a few levels of nested subtransactions and within the same transaction
> > > > I switched it to off or to outer?
>
> I think you missed the above comment?

[copy of earlier reply]

Patch does the same dance as with other xact variables.

XactNesting is the value within the transaction and in the patch this
is not exported, so cannot be set externally.

XactNesting is set at transaction start to the variable
DefaultXactNesting, which is set by the GUC.

So its not a problem, but thanks for checking.

> > > I am not sure whether it is good to not allow PREPARE or we can just
> > > prepare it and come out of the complete nested transaction. Suppose
> > > we have multiple savepoints and we say prepare then it will just
> > > succeed so why does it have to be different here?
> >
> > I'm happy to discuss what the behavior should be in this case. It is
> > not a common case,
> > and people don't put PREPARE in their scripts except maybe in a test.
> >
> > My reasoning for this code is that we don't want to accept a COMMIT
> > until we reach top-level of nesting,
> > so the behavior should be similar for PREPARE, which is just
> > first-half of final commit.
>
> Yeah this is not a very common case. And we can see opinions from
> others as well. But I think your reasoning for doing it this way also
> makes sense to me.
>
> I have some more comments for 0002
> 1.
> + if (XactNesting == XACT_NEST_OUTER && XactNestingLevel > 0)
> + {
> + /* Throw ERROR */
> + ereport(ERROR,
> + (errmsg("nested ROLLBACK, level %u aborts
> outer transaction", XactNestingLevel--)));
> + }
>
> I did not understand in case of 'outer' if we are giving rollback from
> inner nesting level why it is throwing error? Documentation just says
> this[1] but it did not
> mention the error. I agree that we might need to give the rollback as
> many times as the nesting level but giving errors seems confusing to
> me.

Docs mention ROLLBACK at any level will abort the transaction, which
is what the ERROR does.

> [1]
> + <para>
> + A setting of <quote>outer</quote> will cause a nested
> + <command>BEGIN</command> to be remembered, so that an equal number
> + of <command>COMMIT</command> or <command>ROLLBACK</command> commands
> + are required to end the nesting. In that case a
> <command>ROLLBACK</command>
> + at any level will abort the entire outer transaction.
> + Once we reach the top-level transaction,
> + the final <command>COMMIT</command> will end the transaction.
> + This ensures that all commands within the outer transaction are atomic.
> + </para>
>
>
> 2.
>
> + if (XactNesting == XACT_NEST_OUTER)
> + {
> + if (XactNestingLevel <= 0)
> + s->blockState = TBLOCK_END;
> + else
> + ereport(NOTICE,
> + (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> + errmsg("nested COMMIT, level %u",
> XactNestingLevel)));
> + XactNestingLevel--;
> + return true;
> + }

This is decrementing the nesting level for XACT_NEST_OUTER,
until we reach the top level, when the commit is allowed.

> + while (s->parent != NULL && !found_subxact)
> {
> + if (XactNesting == XACT_NEST_ALL &&
> + XactNestingLevel > 0 &&
> + PointerIsValid(s->name) &&
> + strcmp(s->name, NESTED_XACT_NAME) == 0)
> + found_subxact = true;
> +
> if (s->blockState == TBLOCK_SUBINPROGRESS)
> s->blockState = TBLOCK_SUBCOMMIT
>
> I think these changes should be explained in the comments.

This locates the correct subxact by name, as you mention in (4)

> 3.
>
> + if (XactNesting == XACT_NEST_OUTER)
> + {
> + if (XactNestingLevel > 0)
> + {
> + ereport(NOTICE,
> + (errmsg("nested COMMIT, level %u in
> aborted transaction", XactNestingLevel)));
> + XactNestingLevel--;
> + return false;
> + }
> + }
>
> Better to write this as if (XactNesting == XACT_NEST_OUTER &&
> XactNestingLevel > 0) instead of two levels nested if conditions.

Sure. I had been aiming for clarity.

> 4.
> + if (XactNesting == XACT_NEST_ALL &&
> + XactNestingLevel > 0 &&
> + PointerIsValid(s->name) &&
> + strcmp(s->name, NESTED_XACT_NAME) == 0)
> + found_subxact = true;
>
> I think this strcmp(s->name, NESTED_XACT_NAME) is done because there
> could be other types of internal subtransaction also like savepoints?

In XACT_NEST_ALL mode, each nested subxact that is created needs a name.
The name is used to ensure we roll back to the correct subxact, which
might exist.

> What will be the behavior if someone declares a savepoint with this
> name ("_internal_nested_xact"). Will this interfere with this new
> functionality?

Clearly! I guess you are saying we should disallow them.

> Have we tested scenarios like that?

No, but that can be done.

--
Simon Riggs http://www.EnterpriseDB.com/