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

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?

> > 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.

[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;
+ }

+ 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.

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.

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?
What will be the behavior if someone declares a savepoint with this
name ("_internal_nested_xact"). Will this interfere with this new
functionality? Have we tested scenarios like that?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com