Обсуждение: 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].

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

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

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

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%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

Thank you! I missed the same changes in version 11 :(

-- 
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company