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

On Mon, 31 Oct 2022 at 12:22, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Oct 31, 2022 at 5:03 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Oct 31, 2022 at 4:23 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs
> > > <simon(dot)riggs(at)enterprisedb(dot)com> wrote:
> > > >
> > > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com> wrote:
> > > >
> > > > > Thanks for the feedback, I will make all of those corrections in the
> > > > > next version.
> > > >
> > > > New version attached. I've rolled 002-004 into one patch, but can
> > > > split again as needed.
> > >
> > > I like the idea of "parse only" and "nested xact", thanks for working
> > > on this. I will look into patches in more detail, especially nested
> > > xact. IMHO there is no point in merging "nested xact" and "rollback on
> > > commit". They might be changing the same code location but these two
> > > are completely different ideas, in fact all these three should be
> > > reviewed as three separate threads as you mentioned in the first email
> > > in the thread.
> >
> > 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?
>
> 1.
> @@ -3815,6 +3861,10 @@ PrepareTransactionBlock(const char *gid)
> /* Set up to commit the current transaction */
> result = EndTransactionBlock(false);
>
> + /* Don't allow prepare until we are back to an unnested state at level 0 */
> + if (XactNestingLevel > 0)
> + return false;
>
>
> 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.

Note that the nesting of begin/commit is completely separate to the
existence/non-existence of subtransactions, especially with
nested_transactions = 'outer'

> 2. case TBLOCK_SUBABORT:
> ereport(WARNING,
> (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> errmsg("there is already a transaction in progress")));
> + if (XactNesting == XACT_NEST_OUTER)
> + XactNestingLevel++;
> break;
>
> I did not understand what this change is for, can you tell me the
> scenario or a test case to hit this?

Well spotted, thanks. That seems to be some kind of artefact.

There is no test that exercises that since it is an unintended change,
so I have removed it.

> Remaining part w.r.t "nested xact" patch looks fine, I haven't tested
> it yet though.

New versions attached, separated again as you suggested.

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