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
Attachments:
20141006_autocommit-off_regression.patchapplication/octet-stream; name=20141006_autocommit-off_regression.patchDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 0f83799..bc39e55 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1637,6 +1637,8 @@ command_no_begin(const char *query)
if (wordlen == 6 && pg_strncasecmp(query, "system", 6) == 0)
return true;
+ /* ALTER TYPE name ADD VALUE isn't allowed in xacts */
+
return false;
}
@@ -1662,6 +1664,24 @@ command_no_begin(const char *query)
return true;
if (wordlen == 10 && pg_strncasecmp(query, "tablespace", 10) == 0)
return true;
+
+ /* DROP INDEX CONCURRENTLY isn't allowed in xacts */
+ if (wordlen == 5 && pg_strncasecmp(query, "index", 5) == 0)
+ {
+ query += wordlen;
+
+ query = skip_white_space(query);
+
+ wordlen = 0;
+ while (isalpha((unsigned char) query[wordlen]))
+ wordlen += PQmblen(&query[wordlen], pset.encoding);
+
+ if (wordlen == 12 && pg_strncasecmp(query, "concurrently", 12) == 0)
+ return true;
+
+ return false;
+ }
+
return false;
}
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 3764127..8745a84 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -2123,3 +2123,47 @@ execute q;
+------------------+-------------------+
deallocate q;
+--
+-- AUTOCOMMIT
+--
+-- Enable autocommit-off mode, make sure we are not inside a transaction
+\set AUTOCOMMIT OFF
+SELECT 1;
++-[ RECORD 1 ]---+
+| ?column? | 1 |
++----------+-----+
+
+ROLLBACK;
+--
+-- Commands which cannot be executed within a transaction
+-- should not return an error, as we have not begun a transaction yet.
+--
+-- Statements that call PreventTransactionChain() should not implicitly create a transaction.
+--
+-- ALTER SYSTEM
+ALTER SYSTEM SET authentication_timeout = 400;
+ALTER SYSTEM RESET authentication_timeout;
+-- CREATE database
+CREATE DATABASE autocommitregress;
+\c autocommitregress
+-- ALTER TYPE name ADD VALUE
+CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy');
+COMMIT;
+-- ALTER TYPE mood ADD VALUE ('buggy'); #This doesn't work correctly yet
+-- CREATE INDEX CONCURRENTLY
+CREATE TABLE mytest(i integer);
+COMMIT;
+CREATE INDEX CONCURRENTLY concurindex on mytest(i);
+DROP INDEX CONCURRENTLY concurindex;
+-- REINDEX DATABASE
+SET client_min_messages TO WARNING;
+COMMIT;
+REINDEX DATABASE autocommitregress;
+SET client_min_messages TO DEFAULT;
+-- DROP database
+\c postgres
+DROP DATABASE autocommitregress;
+--
+--
+-- Cleanup
+--
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 5ccc68f..2d8ab13 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -276,3 +276,54 @@ execute q;
execute q;
deallocate q;
+
+
+--
+-- AUTOCOMMIT
+--
+-- Enable autocommit-off mode, make sure we are not inside a transaction
+\set AUTOCOMMIT OFF
+SELECT 1;
+ROLLBACK;
+--
+-- Commands which cannot be executed within a transaction
+-- should not return an error, as we have not begun a transaction yet.
+--
+-- Statements that call PreventTransactionChain() should not implicitly create a transaction.
+--
+
+-- ALTER SYSTEM
+ALTER SYSTEM SET authentication_timeout = 400;
+ALTER SYSTEM RESET authentication_timeout;
+
+-- CREATE database
+CREATE DATABASE autocommitregress;
+
+\c autocommitregress
+-- ALTER TYPE name ADD VALUE
+CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy');
+COMMIT;
+-- ALTER TYPE mood ADD VALUE ('buggy'); #This doesn't work correctly yet
+
+-- CREATE INDEX CONCURRENTLY
+CREATE TABLE mytest(i integer);
+COMMIT;
+CREATE INDEX CONCURRENTLY concurindex on mytest(i);
+DROP INDEX CONCURRENTLY concurindex;
+
+-- REINDEX DATABASE
+SET client_min_messages TO WARNING;
+COMMIT;
+REINDEX DATABASE autocommitregress;
+SET client_min_messages TO DEFAULT;
+
+-- DROP database
+\c postgres
+DROP DATABASE autocommitregress;
+
+--
+
+
+--
+-- Cleanup
+--
On Mon, Oct 6, 2014 at 7:36 PM, Feike Steenbergen <
feikesteenbergen@gmail.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.paquier@gmail.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.
Attachments:
20141006_drop_index_concurrently.patchapplication/octet-stream; name=20141006_drop_index_concurrently.patchDownload
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 0f83799..7ed915a 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1662,6 +1662,24 @@ command_no_begin(const char *query)
return true;
if (wordlen == 10 && pg_strncasecmp(query, "tablespace", 10) == 0)
return true;
+
+ /* DROP INDEX CONCURRENTLY isn't allowed in xacts */
+ if (wordlen == 5 && pg_strncasecmp(query, "index", 5) == 0)
+ {
+ query += wordlen;
+
+ query = skip_white_space(query);
+
+ wordlen = 0;
+ while (isalpha((unsigned char) query[wordlen]))
+ wordlen += PQmblen(&query[wordlen], pset.encoding);
+
+ if (wordlen == 12 && pg_strncasecmp(query, "concurrently", 12) == 0)
+ return true;
+
+ return false;
+ }
+
return false;
}
Feike Steenbergen <feikesteenbergen@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Apologies for the previous message, I didn't send the full version.
On 6 October 2014 16:01, Tom Lane <tgl@sss.pgh.pa.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.Nasby@bluetreble.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 7 October 2014 09:55, Marko Tiikkaja <marko@joh.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@joh.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/7/14, 2:11 AM, Feike Steenbergen wrote:
On 7 October 2014 01:41, Jim Nasby<Jim.Nasby@bluetreble.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Jim Nasby-5 wrote
On 10/7/14, 2:11 AM, Feike Steenbergen wrote:
On 7 October 2014 01:41, Jim Nasby<
Jim.Nasby@
> 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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Oct 6, 2014 at 03:49:37PM +0200, Feike Steenbergen wrote:
On 6 October 2014 14:09, Michael Paquier <michael.paquier@gmail.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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers