Re: Conflict between regression tests namespace & transactions due to recent changes
On 2023-05-19 17:59, Tom Lane wrote: Marina Polyakova writes: On 2023-05-19 09:03, Michael Paquier wrote: FYI, the buildfarm is seeing some spurious failures as well: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-1904%3A29%3A42 Yes, it is the same error. Here's another one in version 13: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-18%2022:37:49 Right. I went ahead and pushed the fix in hopes of stabilizing things. (I went with "trans_abc" as the new table name, for consistency with some other nearby names.) regards, tom lane Thank you! I missed the same changes in version 11 :( -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Conflict between regression tests namespace & transactions due to recent changes
Marina Polyakova writes: > On 2023-05-19 09:03, Michael Paquier wrote: >> FYI, the buildfarm is seeing some spurious failures as well: >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-1904%3A29%3A42 > Yes, it is the same error. Here's another one in version 13: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-18%2022:37:49 Right. I went ahead and pushed the fix in hopes of stabilizing things. (I went with "trans_abc" as the new table name, for consistency with some other nearby names.) regards, tom lane
Re: Conflict between regression tests namespace & transactions due to recent changes
On 2023-05-19 09:03, Michael Paquier wrote: On Wed, May 17, 2023 at 02:39:10PM +0900, Michael Paquier wrote: On Tue, May 16, 2023 at 11:02:45AM +0300, Marina Polyakova wrote: It confuses me a little that different methods are used for the same purpose. But the namespace test checks schemas. So see diff_abc_to_txn_table.patch which replaces abc with txn_table in the transaction test. Looks OK seen from here. Thanks! FYI, the buildfarm is seeing some spurious failures as well: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-19 04%3A29%3A42 -- Michael Yes, it is the same error. Here's another one in version 13: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-18%2022:37:49 -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Conflict between regression tests namespace & transactions due to recent changes
On Wed, May 17, 2023 at 02:39:10PM +0900, Michael Paquier wrote: > On Tue, May 16, 2023 at 11:02:45AM +0300, Marina Polyakova wrote: >> It confuses me a little that different methods are used for the same >> purpose. But the namespace test checks schemas. So see >> diff_abc_to_txn_table.patch which replaces abc with txn_table in the >> transaction test. > > Looks OK seen from here. Thanks! FYI, the buildfarm is seeing some spurious failures as well: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer=2023-05-19 04%3A29%3A42 -- Michael signature.asc Description: PGP signature
Re: Conflict between regression tests namespace & transactions due to recent changes
On Tue, May 16, 2023 at 11:02:45AM +0300, Marina Polyakova wrote: > It confuses me a little that different methods are used for the same > purpose. But the namespace test checks schemas. So see > diff_abc_to_txn_table.patch which replaces abc with txn_table in the > transaction test. Looks OK seen from here. Thanks! -- Michael signature.asc Description: PGP signature
Re: Conflict between regression tests namespace & transactions due to recent changes
On 2023-05-16 02:19, Michael Paquier wrote:
On Mon, May 15, 2023 at 11:23:18PM +0300, Marina Polyakova wrote:
Maybe use a separate schema for all new objects in the transaction
test?..
See diff_set_tx_schema.patch.
Sure, you could do that to bypass the failure (without the "public"
actually?), leaving non-generic names around. Still I'd agree with
Tom here and just rename the objects to something more in line with
the context of the test to make things a bit more greppable. These
could be renamed as transaction_tab or transaction_view, for example.
--
Michael
It confuses me a little that different methods are used for the same
purpose. But the namespace test checks schemas. So see
diff_abc_to_txn_table.patch which replaces abc with txn_table in the
transaction test.
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 2b2cff7d9120a139532d1a6a2f2bc79e463bc4fa..21efb900ef1bc30338f1102f6a40f1d25be0db9c 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -609,10 +609,10 @@ drop function inverse(int);
-- performed in the aborted subtransaction
begin;
savepoint x;
-create table abc (a int);
-insert into abc values (5);
-insert into abc values (10);
-declare foo cursor for select * from abc;
+create table txn_table (a int);
+insert into txn_table values (5);
+insert into txn_table values (10);
+declare foo cursor for select * from txn_table;
fetch from foo;
a
---
@@ -625,11 +625,11 @@ fetch from foo;
ERROR: cursor "foo" does not exist
commit;
begin;
-create table abc (a int);
-insert into abc values (5);
-insert into abc values (10);
-insert into abc values (15);
-declare foo cursor for select * from abc;
+create table txn_table (a int);
+insert into txn_table values (5);
+insert into txn_table values (10);
+insert into txn_table values (15);
+declare foo cursor for select * from txn_table;
fetch from foo;
a
---
@@ -698,7 +698,7 @@ COMMIT;
DROP FUNCTION create_temp_tab();
DROP FUNCTION invert(x float8);
-- Tests for AND CHAIN
-CREATE TABLE abc (a int);
+CREATE TABLE txn_table (a int);
-- set nondefault value so we have something to override below
SET default_transaction_read_only = on;
START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
@@ -720,8 +720,8 @@ SHOW transaction_deferrable;
on
(1 row)
-INSERT INTO abc VALUES (1);
-INSERT INTO abc VALUES (2);
+INSERT INTO txn_table VALUES (1);
+INSERT INTO txn_table VALUES (2);
COMMIT AND CHAIN; -- TBLOCK_END
SHOW transaction_isolation;
transaction_isolation
@@ -741,11 +741,11 @@ SHOW transaction_deferrable;
on
(1 row)
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
ERROR: invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
-INSERT INTO abc VALUES (3); -- check it's really aborted
+LINE 1: INSERT INTO txn_table VALUES ('error');
+ ^
+INSERT INTO txn_table VALUES (3); -- check it's really aborted
ERROR: current transaction is aborted, commands ignored until end of transaction block
COMMIT AND CHAIN; -- TBLOCK_ABORT_END
SHOW transaction_isolation;
@@ -766,7 +766,7 @@ SHOW transaction_deferrable;
on
(1 row)
-INSERT INTO abc VALUES (4);
+INSERT INTO txn_table VALUES (4);
COMMIT;
START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
SHOW transaction_isolation;
@@ -788,10 +788,10 @@ SHOW transaction_deferrable;
(1 row)
SAVEPOINT x;
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
ERROR: invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
+LINE 1: INSERT INTO txn_table VALUES ('error');
+ ^
COMMIT AND CHAIN; -- TBLOCK_ABORT_PENDING
SHOW transaction_isolation;
transaction_isolation
@@ -811,7 +811,7 @@ SHOW transaction_deferrable;
on
(1 row)
-INSERT INTO abc VALUES (5);
+INSERT INTO txn_table VALUES (5);
COMMIT;
START TRANSACTION ISOLATION LEVEL REPEATABLE READ, READ WRITE, DEFERRABLE;
SHOW transaction_isolation;
@@ -873,7 +873,7 @@ SHOW transaction_deferrable;
off
(1 row)
-INSERT INTO abc VALUES (6);
+INSERT INTO txn_table VALUES (6);
ROLLBACK AND CHAIN; -- TBLOCK_ABORT_PENDING
SHOW transaction_isolation;
transaction_isolation
@@ -893,10 +893,10 @@ SHOW transaction_deferrable;
off
(1 row)
-INSERT INTO abc VALUES ('error');
+INSERT INTO txn_table VALUES ('error');
ERROR: invalid input syntax for type integer: "error"
-LINE 1: INSERT INTO abc VALUES ('error');
-^
+LINE 1: INSERT INTO txn_table VALUES ('error');
+ ^
ROLLBACK AND CHAIN; -- TBLOCK_ABORT_END
SHOW transaction_isolation;
Re: Conflict between regression tests namespace & transactions due to recent changes
Noah Misch writes: > For the record, I'm fairly sure s/public, test_ns_schema_1/test_ns_schema_1/ > on the new namespace tests would also solve things. Those tests don't need > "public" in the picture. Nonetheless, +1 for your proposal. Hmm, I'd not read the test case all that closely, but I did think that including "public" in the search path was an important part of it. If it is not, maybe the comments could use adjustment. regards, tom lane
Re: Conflict between regression tests namespace & transactions due to recent changes
On Mon, May 15, 2023 at 12:16:08PM -0400, Tom Lane wrote: > Marina Polyakova writes: > > IIUC the conflict was caused by > > > +SET search_path to public, test_ns_schema_1; > > +CREATE SCHEMA test_ns_schema_2 > > + CREATE VIEW abc_view AS SELECT a FROM abc; > > > because the parallel regression test transactions had already created > > the table abc and was trying to drop it. > > Hmm. I'd actually fix the blame on transactions.sql here. Creating > a table named as generically as "abc" is horribly bad practice in > a set of concurrent tests. namespace.sql is arguably okay, since > it's creating that table name in a private schema. > > I'd be inclined to fix this by doing s/abc/something-else/g in > transactions.sql. For the record, I'm fairly sure s/public, test_ns_schema_1/test_ns_schema_1/ on the new namespace tests would also solve things. Those tests don't need "public" in the picture. Nonetheless, +1 for your proposal.
Re: Conflict between regression tests namespace & transactions due to recent changes
On Mon, May 15, 2023 at 11:23:18PM +0300, Marina Polyakova wrote: > On 2023-05-15 19:16, Tom Lane wrote: >> Hmm. I'd actually fix the blame on transactions.sql here. Creating >> a table named as generically as "abc" is horribly bad practice in >> a set of concurrent tests. namespace.sql is arguably okay, since >> it's creating that table name in a private schema. >> >> I'd be inclined to fix this by doing s/abc/something-else/g in >> transactions.sql. > > Maybe use a separate schema for all new objects in the transaction test?.. > See diff_set_tx_schema.patch. Sure, you could do that to bypass the failure (without the "public" actually?), leaving non-generic names around. Still I'd agree with Tom here and just rename the objects to something more in line with the context of the test to make things a bit more greppable. These could be renamed as transaction_tab or transaction_view, for example. -- Michael signature.asc Description: PGP signature
Re: Conflict between regression tests namespace & transactions due to recent changes
On 2023-05-15 19:16, Tom Lane wrote: Marina Polyakova writes: IIUC the conflict was caused by +SET search_path to public, test_ns_schema_1; +CREATE SCHEMA test_ns_schema_2 + CREATE VIEW abc_view AS SELECT a FROM abc; because the parallel regression test transactions had already created the table abc and was trying to drop it. Hmm. I'd actually fix the blame on transactions.sql here. Creating a table named as generically as "abc" is horribly bad practice in a set of concurrent tests. namespace.sql is arguably okay, since it's creating that table name in a private schema. I'd be inclined to fix this by doing s/abc/something-else/g in transactions.sql. regards, tom lane Maybe use a separate schema for all new objects in the transaction test?.. See diff_set_tx_schema.patch. -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Companydiff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out index 2b2cff7d9120a139532d1a6a2f2bc79e463bc4fa..6a3758bafb54327fbe341dfd89457e53ffce47dd 100644 --- a/src/test/regress/expected/transactions.out +++ b/src/test/regress/expected/transactions.out @@ -1,6 +1,10 @@ -- -- TRANSACTIONS -- +-- Use your schema to avoid conflicts with objects with the same names in +-- parallel tests. +CREATE SCHEMA test_tx_schema; +SET search_path TO test_tx_schema,public; BEGIN; CREATE TABLE xacttest (a smallint, b real); INSERT INTO xacttest VALUES diff --git a/src/test/regress/sql/transactions.sql b/src/test/regress/sql/transactions.sql index 7ee5f6aaa55a686f5484664c3fae224c08bde42a..0e6d6453edaa776425524d43abf5ae3cfca3ac45 100644 --- a/src/test/regress/sql/transactions.sql +++ b/src/test/regress/sql/transactions.sql @@ -2,6 +2,11 @@ -- TRANSACTIONS -- +-- Use your schema to avoid conflicts with objects with the same names in +-- parallel tests. +CREATE SCHEMA test_tx_schema; +SET search_path TO test_tx_schema,public; + BEGIN; CREATE TABLE xacttest (a smallint, b real);
Re: Conflict between regression tests namespace & transactions due to recent changes
Marina Polyakova writes: > IIUC the conflict was caused by > +SET search_path to public, test_ns_schema_1; > +CREATE SCHEMA test_ns_schema_2 > + CREATE VIEW abc_view AS SELECT a FROM abc; > because the parallel regression test transactions had already created > the table abc and was trying to drop it. Hmm. I'd actually fix the blame on transactions.sql here. Creating a table named as generically as "abc" is horribly bad practice in a set of concurrent tests. namespace.sql is arguably okay, since it's creating that table name in a private schema. I'd be inclined to fix this by doing s/abc/something-else/g in transactions.sql. regards, tom lane
