On Mon, 31 Oct 2022 at 12:22, Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Mon, Oct 31, 2022 at 5:03 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Mon, Oct 31, 2022 at 4:23 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs > > > <simon.ri...@enterprisedb.com> wrote: > > > > > > > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs > > > > <simon.ri...@enterprisedb.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/
001_psql_parse_only.v1.patch
Description: Binary data
002_nested_xacts.v9.patch
Description: Binary data
003_rollback_on_commit.v1.patch
Description: Binary data
