Re: Add regression tests for autocommit-off mode for psql and fix some omissions

Hi all,

Lately I have come across two inconveniences/bugs related to running
the autocommit-off mode in psql.

These are:
- BUG #11524: Unable to add value to ENUM when having AUTOCOMMIT
disabled in psql
- BUG #10822: "ALTER SYSTEM cannot run inside a transaction block"
when having autocommit disabled.

The documentation states about autocommit-off in
http://www.postgresql.org/docs/devel/static/app-psql.html :

The autocommit-off mode works by issuing an implicit BEGIN for you,
just before any command that is not already in a transaction block and
is not itself a BEGIN or other transaction-control command, nor a
command that cannot be executed inside a transaction block (such as
VACUUM).

In src/bin/psql/common.c the statements which should not start a
transaction implicitly are filtered in command_no_begin.

I would like to propose to add a regression test for all statements
that call PreventTransactionChain in autocommit-off mode. I propose to
add these tests to src/test/regress/sql/psql.sql as this is a
psql-specific mode.
Alternatively an isolated test called autocommit.sql could be created.

During the writing of the regression test I found another statement
not covered in the current function: DROP INDEX CONCURRENTLY.

After applying this patch, the only command that I can find currently
which cannot be executed inside a transaction block and is currently
not able to be run in autocommit-off is "ALTER TYPE name ADD VALUE";

I have created a patch consisting of a regression test and adding DROP
INDEX CONCURRENTLY to command_no_begin.

Kind regards,

Feike Steenbergen

On Mon, Oct 6, 2014 at 7:36 PM, Feike Steenbergen <
feikesteenbergen(at)gmail(dot)com> wrote:

> I would like to propose to add a regression test for all statements
> that call PreventTransactionChain in autocommit-off mode. I propose to
> add these tests to src/test/regress/sql/psql.sql as this is a
> psql-specific mode. Alternatively an isolated test called autocommit.sql
> could be created.
>
Putting all this stuff in psql.sql is good enough IMO.

> During the writing of the regression test I found another statement
> not covered in the current function: DROP INDEX CONCURRENTLY.
>
That's a good catch and it should be a separate patch. This could even be
considered for a back-patch down to 9.2. Thoughts?

>
> I have created a patch consisting of a regression test and adding DROP
> INDEX CONCURRENTLY to command_no_begin.
>

CREATE DATABASE and DROP DATABASE are not commands present (not allowed?)
in the regression suite. ALTER SYSTEM has no tests as well, and REINDEX
DATABASE may take time so they may be better ripped off... Also tests for
CLUSTER without arguments, transaction commands, DISCARD and VACUUM would
be good things.
Regards,
--
Michael

On 6 October 2014 14:09, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> That's a good catch and it should be a separate patch. This could even be
> considered for a back-patch down to 9.2. Thoughts?

If I isolate "DROP INDEX concurrently", this patch would do the trick.

Feike Steenbergen <feikesteenbergen(at)gmail(dot)com> writes:
> I would like to propose to add a regression test for all statements
> that call PreventTransactionChain in autocommit-off mode.

What class of bug would that prevent exactly? It seems to me like
something that would normally get forgotten when we add any new
such statement.

regards, tom lane

It would test that when setting AUTOCOMMIT to off that you will not run into:

ERROR: [...] cannot run inside a transaction block

when issuing one of these PreventTransactionChain commands. In
src/bin/psql/common.c

On 10/6/14, 9:59 AM, Feike Steenbergen wrote:
> It would test that when setting AUTOCOMMIT to off that you will not run into:
>
> ERROR: [...] cannot run inside a transaction block
>
> when issuing one of these PreventTransactionChain commands. In
> src/bin/psql/common.c

Yes, but what happens when a new non-transaction command is added? If we forget to exclude it in psql, we'll certainly also forget to add it to the unit test.

The options I see...

1) If there's a definitive way to tell from backend source code what commands disallow transactions then we can just use that information to generate the list of commands psql shouldn't do that with.

2) Always run the regression test with auto-commit turned off.

3) Run the regression in both modes (presumably only on the build farm due to how long it would take).
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Apologies for the previous message, I didn't send the full version.

On 6 October 2014 16:01, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> What class of bug would that prevent exactly?

ERROR: [...] cannot run inside a transaction block

when:
- running psql in AUTOCOMMIT off
- not having started a transaction yet

Currently some statements (ALTER TYPE name ADD VALUE, DROP INDEX CONCURRENTLY)
can only be run in psql when enabling autocommit
(which I consider a bug - either in the code, or in the documentation),
whilst many others (VACUUM, CREATE DATABASE) can be run in AUTOCOMMIT
off because
they will not implicitly create a transaction in psql.

> It seems to me like
> something that would normally get forgotten when we add any new
> such statement.

I think that is probably true; it has already been forgotten to be added
to psql for a few commands.
Perhaps I am the only one using autocommit-off mode and we shouldn't put effort
into fixing this?

For me the reason to add some tests was to make sure that the current behaviour
will not change in future versions; the function command_no_begin might be added
to, modified, or rewritten.

On 7 October 2014 01:41, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> The options I see...
>
> 1) If there's a definitive way to tell from backend source code what
> commands disallow transactions then we can just use that information to
> generate the list of commands psql shouldn't do that with.
>
> 2) Always run the regression test with auto-commit turned off.
>
> 3) Run the regression in both modes (presumably only on the build farm due
> to how long it would take).

1) I don't know about a definitive way. I used grep to find all
statements calling PreventTransactionChain.

2) - I expect most people use autocommit-on; so only running it in
autocommit-off would not test the majority of users.
- autocommit-off also obliges you to explicitly rollback transactions after
errors occur; this would probably mean a rewrite of some tests?

kind regards,

Feike Steenbergen

On 10/7/14, 9:11 AM, Feike Steenbergen wrote:
> Perhaps I am the only one using autocommit-off mode

You most definitely aren't.

> and we shouldn't put effort
> into fixing this?

It's not clear to me that this is fixing a problem, to be honest. If
you're running autocommit=off, you have an expectation that you can roll
back commands at will. It's fine if I can't roll back a VACUUM, for
example, since I would practically never want to do that. But ALTER
TYPE .. ADD VALUE ..; is an entirely different beast. That one's
permanent; there's no DROP equivalent. If the command is just executed,
and I can't roll it back, wouldn't that be a serious violation of the
principle of least astonishment? DROP INDEX CONCURRENTLY has a bit of
the same problem. You can CREATE INDEX CONCURRENTLY, but it might take
days in some cases.

I think that just running the command is a bad idea, and if we want to
fix something here we should focus on just providing a better error message.

.marko

On 7 October 2014 09:55, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> It's not clear to me that this is fixing a problem, to be honest. If you're
> running autocommit=off, you have an expectation that you can roll back
> commands at will. It's fine if I can't roll back a VACUUM, for example,
> since I would practically never want to do that. But ALTER TYPE .. ADD
> VALUE ..; is an entirely different beast. That one's permanent; there's no
> DROP equivalent. If the command is just executed, and I can't roll it back,
> wouldn't that be a serious violation of the principle of least astonishment?

I think you have a valid and good point; however the autocommit-off mode can
currently already execute statements which cannnot be rolled back.
Perhaps it is a good idea to not allow any of these statements in autocommit-off
mode to prevent astonishement from users, but that would be a discussion of
itself.

My reason for proposing this is to have all these commands treated
consistently.
The expectation of being able to roll back commands at will cannot be fulfilled
currently, many statemens that are allowed with autocommit-off fall into the
category "different beast".

Currently the following statemens call PreventTransactionChain and do not
generate errors in autocommit-off mode:
- REINDEX DATABASE
- CREATE INDEX CONCURRENTLY
- ALTER SYSTEM
- CREATE DATABASE
- DROP DATABASE
- CREATE TABLESPACE
- DROP TABLESPACE
- CLUSTER
- VACUUM

The following statements call PreventTransactionChain and do generate errors
in autocommit-off mode:
- DROP INDEX CONCURRENTLY
- ALTER DATABASE ... SET TABLESPACE
- ALTER TYPE ... ADD

I don't see why these last three should be treated seperately from the
first list; we should
either allow all, or none of these statements IMHO.

kind regards,

Feike Steenbergen

On 7 October 2014 09:55, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> On 10/7/14, 9:11 AM, Feike Steenbergen wrote:
>>
>> Perhaps I am the only one using autocommit-off mode
>
>
> You most definitely aren't.
>
>> and we shouldn't put effort
>> into fixing this?
>
>
> It's not clear to me that this is fixing a problem, to be honest. If you're
> running autocommit=off, you have an expectation that you can roll back
> commands at will. It's fine if I can't roll back a VACUUM, for example,
> since I would practically never want to do that. But ALTER TYPE .. ADD
> VALUE ..; is an entirely different beast. That one's permanent; there's no
> DROP equivalent. If the command is just executed, and I can't roll it back,
> wouldn't that be a serious violation of the principle of least astonishment?
> DROP INDEX CONCURRENTLY has a bit of the same problem. You can CREATE INDEX
> CONCURRENTLY, but it might take days in some cases.
>
> I think that just running the command is a bad idea, and if we want to fix
> something here we should focus on just providing a better error message.
>
>
> .marko

On 10/7/14, 2:11 AM, Feike Steenbergen wrote:
> On 7 October 2014 01:41, Jim Nasby<Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> >The options I see...
>> >
>> >1) If there's a definitive way to tell from backend source code what
>> >commands disallow transactions then we can just use that information to
>> >generate the list of commands psql shouldn't do that with.
>> >
>> >2) Always run the regression test with auto-commit turned off.
>> >
>> >3) Run the regression in both modes (presumably only on the build farm due
>> >to how long it would take).
>
> 1) I don't know about a definitive way. I used grep to find all
> statements calling PreventTransactionChain.

Perhaps it wouldn't be too horrific to create some perl code that would figure out what all of those commands are, and we could then use that to generate the appropriate list for psql.

> 2) - I expect most people use autocommit-on; so only running it in
> autocommit-off would not test the majority of users.
> - autocommit-off also obliges you to explicitly rollback transactions after
> errors occur; this would probably mean a rewrite of some tests?

Well, that is at least doable, but probably rather ugly. It would probably be less ugly if our test framework had a way to test for errors (ala pgTap).

Where I was going with this is a full-on brute-force test: execute every possible command with autocommit turned off. We don't need to check that each command does what it's supposed to do, only that it can execute.

Of course, the huge problem with that is knowing how to actually successfully run each command. :( Theoretically the tests could be structured in such a way that there's a subset of tests that just see if the command even executes, but creating that is obviously a lot of work and with our current test framework probably a real pain to maintain.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby-5 wrote
> On 10/7/14, 2:11 AM, Feike Steenbergen wrote:
>> On 7 October 2014 01:41, Jim Nasby&lt;

> Jim.Nasby@

> &gt; wrote:
>>> >The options I see...
>>> >
>>> >1) If there's a definitive way to tell from backend source code what
>>> >commands disallow transactions then we can just use that information to
>>> >generate the list of commands psql shouldn't do that with.
>>> >
>>> >2) Always run the regression test with auto-commit turned off.
>>> >
>>> >3) Run the regression in both modes (presumably only on the build farm
>>> due
>>> >to how long it would take).
>>
>> 1) I don't know about a definitive way. I used grep to find all
>> statements calling PreventTransactionChain.
>
> Perhaps it wouldn't be too horrific to create some perl code that would
> figure out what all of those commands are, and we could then use that to
> generate the appropriate list for psql.
>
>> 2) - I expect most people use autocommit-on; so only running it in
>> autocommit-off would not test the majority of users.
>> - autocommit-off also obliges you to explicitly rollback transactions
>> after
>> errors occur; this would probably mean a rewrite of some tests?
>
> Well, that is at least doable, but probably rather ugly. It would probably
> be less ugly if our test framework had a way to test for errors (ala
> pgTap).
>
> Where I was going with this is a full-on brute-force test: execute every
> possible command with autocommit turned off. We don't need to check that
> each command does what it's supposed to do, only that it can execute.
>
> Of course, the huge problem with that is knowing how to actually
> successfully run each command. :( Theoretically the tests could be
> structured in such a way that there's a subset of tests that just see if
> the command even executes, but creating that is obviously a lot of work
> and with our current test framework probably a real pain to maintain.

From the comments here the effort needed to prevent this particular
oversight seems excessive compared to the error it is trying to prevent - an
error that is fairly easily remedied in a minor release and which has an
easy work around.

That said can we just do:

"1) I don't know about a definitive way. I used grep to find all
statements calling PreventTransactionChain."

and save the results to an .out file with a comment somewhere that if there
is any change to the content of this file the corresponding command should
be manually tested in psql with autocommit=on. This seems to be what you
are saying but the psql check does not have to be automated...

David J.

--
View this message in context: http://postgresql.1045698.n5.nabble.com/Add-regression-tests-for-autocommit-off-mode-for-psql-and-fix-some-omissions-tp5821889p5823728.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

On 10/20/14, 3:49 PM, David G Johnston wrote:
>> Well, that is at least doable, but probably rather ugly. It would probably
>> >be less ugly if our test framework had a way to test for errors (ala
>> >pgTap).
>> >
>> >Where I was going with this is a full-on brute-force test: execute every
>> >possible command with autocommit turned off. We don't need to check that
>> >each command does what it's supposed to do, only that it can execute.
>> >
>> >Of course, the huge problem with that is knowing how to actually
>> >successfully run each command.:( Theoretically the tests could be
>> >structured in such a way that there's a subset of tests that just see if
>> >the command even executes, but creating that is obviously a lot of work
>> >and with our current test framework probably a real pain to maintain.
> From the comments here the effort needed to prevent this particular
> oversight seems excessive compared to the error it is trying to prevent - an
> error that is fairly easily remedied in a minor release and which has an
> easy work around.
>
> That said can we just do:
>
> "1) I don't know about a definitive way. I used grep to find all
> statements calling PreventTransactionChain."
>
> and save the results to an .out file with a comment somewhere that if there
> is any change to the content of this file the corresponding command should
> be manually tested in psql with autocommit=on. This seems to be what you
> are saying but the psql check does not have to be automated...

Are you thinking we'd commit the expected output of the perl script and have the regression suite call that script to verify it? That seems like a good way to fix this. The only better option I can come up with is if the perl script generated an actual test that we know would fail if a new command showed up.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Mon, Oct 6, 2014 at 03:49:37PM +0200, Feike Steenbergen wrote:
> On 6 October 2014 14:09, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> > That's a good catch and it should be a separate patch. This could even be
> > considered for a back-patch down to 9.2. Thoughts?
>
> If I isolate "DROP INDEX concurrently", this patch would do the trick.

Patch applied for 9.5. Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +