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

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