Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
On Mon, Oct 6, 2014 at 03:49:37PM +0200, Feike Steenbergen wrote: On 6 October 2014 14:09, Michael Paquier michael.paqu...@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 br...@momjian.ushttp://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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
Apologies for the previous message, I didn't send the full version. On 6 October 2014 16:01, Tom Lane t...@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.na...@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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
On 7 October 2014 09:55, Marko Tiikkaja ma...@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 ma...@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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
On 10/7/14, 2:11 AM, Feike Steenbergen wrote: On 7 October 2014 01:41, Jim Nasbyjim.na...@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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
On Mon, Oct 6, 2014 at 7:36 PM, Feike Steenbergen feikesteenber...@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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
On 6 October 2014 14:09, Michael Paquier michael.paqu...@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. 20141006_drop_index_concurrently.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
Feike Steenbergen feikesteenber...@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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
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
Re: [HACKERS] Add regression tests for autocommit-off mode for psql and fix some omissions
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
