Re: Code checks for App Devs, using new options for transaction behavior - Mailing list pgsql-hackers

On Mon, Oct 31, 2022 at 6:54 PM Simon Riggs
<simon.riggs@enterprisedb.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