Conflict between regression tests namespace & transactions due to recent changes
Hello, hackers!
When running tests for version 15, we found a conflict between
regression tests namespace & transactions due to recent changes [1]https://github.com/postgres/postgres/commit/dbd5795e7539ec9e15c0d4ed2d05b1b18d2a3b09.
diff -w -U3 .../src/test/regress/expected/transactions.out
.../src/bin/pg_upgrade/tmp_check/results/transactions.out
--- .../src/test/regress/expected/transactions.out ...
+++ .../src/bin/pg_upgrade/tmp_check/results/transactions.out ...
@@ -899,6 +899,9 @@ RESET default_transaction_read_only;
DROP TABLE abc;
+ERROR: cannot drop table abc because other objects depend on it
+DETAIL: view test_ns_schema_2.abc_view depends on table abc
+HINT: Use DROP ... CASCADE to drop the dependent objects too.
-- Test assorted behaviors around the implicit transaction block
created
-- when multiple SQL commands are sent in a single Query message.
These
-- tests rely on the fact that psql will not break SQL commands apart
at a
...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.
ISTM the patch diff.patch fixes this problem...
[1]: https://github.com/postgres/postgres/commit/dbd5795e7539ec9e15c0d4ed2d05b1b18d2a3b09
https://github.com/postgres/postgres/commit/dbd5795e7539ec9e15c0d4ed2d05b1b18d2a3b09
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
diff.patchtext/x-diff; name=diff.patchDownload+16-3
Marina Polyakova <m.polyakova@postgrespro.ru> 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
On 2023-05-15 19:16, Tom Lane wrote:
Marina Polyakova <m.polyakova@postgrespro.ru> 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 Company
Attachments:
diff_set_tx_schema.patchtext/x-diff; name=diff_set_tx_schema.patchDownload+9-0
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
On Mon, May 15, 2023 at 12:16:08PM -0400, Tom Lane wrote:
Marina Polyakova <m.polyakova@postgrespro.ru> 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.
Noah Misch <noah@leadboat.com> 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
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 Company
Attachments:
diff_abc_to_txn_table.patchtext/x-diff; name=diff_abc_to_txn_table.patchDownload+70-70
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
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&dt=2023-05-19 04%3A29%3A42
--
Michael
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&dt=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&dt=2023-05-18%2022:37:49
--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Marina Polyakova <m.polyakova@postgrespro.ru> 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&dt=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&dt=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
On 2023-05-19 17:59, Tom Lane wrote:
Marina Polyakova <m.polyakova@postgrespro.ru> 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&dt=2023-05-1904%3A29%3A42Yes, it is the same error. Here's another one in version 13:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2023-05-18%2022:37:49Right. 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