Thread: Code checks for App Devs, using new options for transaction behavior
In the past, developers have wondered how we can provide "--dry-run" functionality https://www.postgresql.org/message-id/15791.1450383201%40sss.pgh.pa.us This is important for application developers, especially when migrating programs to Postgres. Presented here are 3 features aimed at developers, each of which is being actively used by me in a large and complex migration project. * psql --parse-only Checks the syntax of all SQL in a script, but without actually executing it. This is very important in the early stages of complex migrations because we need to see if the code would generate syntax errors before we attempt to execute it. When there are many dependencies between objects, actual execution fails very quickly if we run in a single transaction, yet running outside of a transaction can leave a difficult cleanup task. Fixing errors iteratively is difficult when there are long chains of dependencies between objects, since there is no easy way to predict how long it will take to make everything work unless you understand how many syntax errors exist in the script. 001_psql_parse_only.v1.patch * nested transactions = off (default) | all | on Handle nested BEGIN/COMMIT, which can cause chaos on failure. This is an important part of guaranteeing that everything that gets executed is part of a single atomic transaction, which can then be rolled back - this is a pre-requisite for the last feature. 002_nested_xacts.v7.patch The default behavior is unchanged (off) Setting "all" treats nested BEGIN/COMMIT as subtransactions, allowing some parts to fail without rolling back the outer transaction. Setting "outer" flattens nested BEGIN/COMMIT into one single outer transaction, so that any failure rolls back the entire transaction. * rollback_on_commit = off (default) | on Force transactions to fail their final commit, ensuring that no lasting change is made when a script is tested. i.e. accept COMMIT, but do rollback instead. 003_rollback_on_commit.v1.patch We will probably want to review these on separate threads, but the common purpose of these features is hopefully clear from these notes. 001 and 003 are fairly small patches, 002 is longer. Comments please -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Thu, 27 Oct 2022 at 12:09, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > Comments please Update from patch tester results. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
Op 27-10-2022 om 18:35 schreef Simon Riggs: > On Thu, 27 Oct 2022 at 12:09, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > >> Comments please > > Update from patch tester results. > > [001_psql_parse_only.v1.patch ] > [002_nested_xacts.v7.patch ] > [003_rollback_on_commit.v1.patch ] > [004_add_params_to_sample.v1.patch] patch 002 has (2x) : 'transction' should be 'transaction' also in patch 002: 'at any level will be abort' should be 'at any level will abort' I also dislike the 'we' in 'Once we reach the top-level transaction,' That seems a bit too much like the 'we developers working together to make a database server system' which is of course used often and usefully on this mailinglist and in code itself. But I think user-facing docs should be careful with that team-building 'we'. I remember well how it confused me, many years ago. Better, IMHO: 'Once the top-level transaction is reached,' Thanks, Erik Rijkers
On Fri, 28 Oct 2022 at 07:54, Erik Rijkers <er@xs4all.nl> wrote: > > Op 27-10-2022 om 18:35 schreef Simon Riggs: > > On Thu, 27 Oct 2022 at 12:09, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > > >> Comments please > > > > Update from patch tester results. > > > > > [001_psql_parse_only.v1.patch ] > > [002_nested_xacts.v7.patch ] > > [003_rollback_on_commit.v1.patch ] > > [004_add_params_to_sample.v1.patch] > > > patch 002 has (2x) : > 'transction' should be > 'transaction' > > also in patch 002: > 'at any level will be abort' should be > 'at any level will abort' > > I also dislike the 'we' in > > 'Once we reach the top-level transaction,' > > That seems a bit too much like the 'we developers working together to > make a database server system' which is of course used often and > usefully on this mailinglist and in code itself. But I think > user-facing docs should be careful with that team-building 'we'. I > remember well how it confused me, many years ago. Better, IMHO: > > 'Once the top-level transaction is reached,' Thanks for the feedback, I will make all of those corrections in the next version. I'm guessing you like the features?? -- Simon Riggs http://www.EnterpriseDB.com/
On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon.riggs@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. -- Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon.riggs@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. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Oct 31, 2022 at 4:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs > <simon.riggs@enterprisedb.com> wrote: > > > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon.riggs@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? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Mon, Oct 31, 2022 at 5:03 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 4:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs
> > <simon.riggs@enterprisedb.com> wrote:
> > >
> > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon.riggs@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?
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?
Remaining part w.r.t "nested xact" patch looks fine, I haven't tested
it yet though.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Mon, 31 Oct 2022 at 11:33, Dilip Kumar <dilipbalaut@gmail.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? 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. -- Simon Riggs http://www.EnterpriseDB.com/
On Mon, 31 Oct 2022 at 12:22, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 5:03 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 4:23 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Sun, Oct 30, 2022 at 11:32 PM Simon Riggs
> > > <simon.riggs@enterprisedb.com> wrote:
> > > >
> > > > On Fri, 28 Oct 2022 at 10:33, Simon Riggs <simon.riggs@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/
Attachment
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
On Wed, 2 Nov 2022 at 03:52, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> 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?
[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/
On Wed, 2 Nov 2022 at 07:40, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
> > 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.
More tests as requested, plus minor code rework, plus comment updates.
--
Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Mon, 7 Nov 2022 at 14:25, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>
> On Wed, 2 Nov 2022 at 07:40, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>
> > > 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.
>
> More tests as requested, plus minor code rework, plus comment updates.
New versions
--
Simon Riggs http://www.EnterpriseDB.com/
Attachment
On Thu, 27 Oct 2022 at 07:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote: > > In the past, developers have wondered how we can provide "--dry-run" > functionality That would be an awesome functionality, indeed. I have concerns of how feasible it is in general but I think providing features to allow developers to build it for their use cases is a good approach. The corner cases that might not be possible in general might be tractable developers willing to constrain their development environment or use information available outside Postgres. But... I have concerns about some of the design here. > * psql --parse-only > Checks the syntax of all SQL in a script, but without actually > executing it. This is very important in the early stages of complex > migrations because we need to see if the code would generate syntax > errors before we attempt to execute it. When there are many > dependencies between objects, actual execution fails very quickly if > we run in a single transaction, yet running outside of a transaction > can leave a difficult cleanup task. Fixing errors iteratively is > difficult when there are long chains of dependencies between objects, > since there is no easy way to predict how long it will take to make > everything work unless you understand how many syntax errors exist in > the script. > 001_psql_parse_only.v1.patch This effectively enables \gdesc mode for every query. It needs docs explaining what's actually going to happen and how to use it because that wasn't super obvious to me even after reading the patch. I'm not sure reusing DescribeQuery() and then returning early is the best idea. But more importantly it's only going to handle the simplest scripts that don't do DDL that further statements will depend on. That at least needs to be documented. > * nested transactions = off (default) | all | on > Handle nested BEGIN/COMMIT, which can cause chaos on failure. This is > an important part of guaranteeing that everything that gets executed > is part of a single atomic transaction, which can then be rolled back > - this is a pre-requisite for the last feature. > 002_nested_xacts.v7.patch > The default behavior is unchanged (off) > Setting "all" treats nested BEGIN/COMMIT as subtransactions, allowing > some parts to fail without rolling back the outer transaction. > Setting "outer" flattens nested BEGIN/COMMIT into one single outer > transaction, so that any failure rolls back the entire transaction. I think we've been burned pretty badly by GUCs that control SQL semantics before. I think there was discussion at the time nested transactions went in and there must have been a reason we did SAVEPOINT rather than make nested BEGINs do things like this. But regardless if we do want to change what nested BEGINs do I think we have to decide what behaviour we want, think about the backwards compatibility impacts, and make the change. We can't make it just for some people some of the time based on a GUC. Doing that makes it impossible to write scripts that work consistently. I'm not clear what happens if you have this feature enabled and *also* use SAVEPOINTs... You say this is a prerequisite for 003 and I see how they're related though I don't immediately see why it should be necessary to change nested BEGIN behaviour to make that work. > * rollback_on_commit = off (default) | on > Force transactions to fail their final commit, ensuring that no > lasting change is made when a script is tested. i.e. accept COMMIT, > but do rollback instead. > 003_rollback_on_commit.v1.patch I suppose technically this is also a "semantics controlled by a GUC" but I guess it's safe since you would only set this when you want this debugging environment and then you really do want it to be global. I'm not sure it's super safe though. Like, dblink connections can break it, what happens if it gets turned off midway through a transaction? I wonder if this should be handled by the client itself the way autocommit is. Like have an "autocommit" mode of "autorollback" instead. That would mean having to add it to every client library of course but then perhaps it would be able to change client behaviour in ways that make sense at the same time. -- greg
Greg Stark <stark@mit.edu> writes:
> On Thu, 27 Oct 2022 at 07:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote:
>> * nested transactions = off (default) | all | on
>> Handle nested BEGIN/COMMIT, which can cause chaos on failure.
> I think we've been burned pretty badly by GUCs that control SQL
> semantics before.
Yeah, this idea is an absolute nonstarter. rollback_on_commit seems
excessively dangerous as well compared to the value.
> I think there was discussion at the time nested
> transactions went in and there must have been a reason we did
> SAVEPOINT rather than make nested BEGINs do things like this.
I believe the reason was "because the SQL standard says so".
I'm not sure if any of these proposals are still live now that
Simon's retired. Presumably somebody else would have to push
them forward for there to be a chance of anything happening.
regards, tom lane