Обсуждение: maybe a type_sanity. sql bug

hi.
The test seems to assume the following sql query should return zero row.
but it does not. I don't know much about the "relreplident" column.

https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/expected/type_sanity.out#n499
demo: https://dbfiddle.uk/QFM88S2e

test1=# \dt
Did not find any relations.
test1=# SELECT c1.oid, c1.relname, relkind, relpersistence, relreplident
FROM pg_class as c1
WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR
    relpersistence NOT IN ('p', 'u', 't') OR
    relreplident NOT IN ('d', 'n', 'f', 'i');
 oid | relname | relkind | relpersistence | relreplident
-----+---------+---------+----------------+--------------
(0 rows)

test1=# CREATE TABLE test_partition (
        id int4range,
        valid_at daterange,
        name text,
        CONSTRAINT test_partition_uq1 UNIQUE (id, valid_at )
) PARTITION BY LIST (id);
CREATE TABLE
test1=# SELECT c1.oid, c1.relname, relkind, relpersistence, relreplident
FROM pg_class as c1
WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR
    relpersistence NOT IN ('p', 'u', 't') OR
    relreplident NOT IN ('d', 'n', 'f', 'i');
   oid   |      relname       | relkind | relpersistence | relreplident
---------+--------------------+---------+----------------+--------------
 1034304 | test_partition_uq1 | I       | p              | n
(1 row)

test1=# select version();
                              version
--------------------------------------------------------------------
 PostgreSQL 16beta1 on x86_64-linux, compiled by gcc-11.3.0, 64-bit
(1 row)

On Fri, Oct 27, 2023 at 11:45:44AM +0800, jian he wrote:
> The test seems to assume the following sql query should return zero row.
> but it does not. I don't know much about the "relreplident" column.

This is not about relreplident here, that refers to a relation's
replica identity.

> test1=# SELECT c1.oid, c1.relname, relkind, relpersistence, relreplident
> FROM pg_class as c1
> WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR
>     relpersistence NOT IN ('p', 'u', 't') OR
>     relreplident NOT IN ('d', 'n', 'f', 'i');
>  oid | relname | relkind | relpersistence | relreplident
> -----+---------+---------+----------------+--------------
> (0 rows)

The problem is about relkind, as 'I' refers to a partitioned index.
That is a legal value in pg_class.relkind, but we forgot to list it in
this test.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Oct 27, 2023 at 11:45:44AM +0800, jian he wrote:
>> The test seems to assume the following sql query should return zero row.
>> but it does not. I don't know much about the "relreplident" column.

> The problem is about relkind, as 'I' refers to a partitioned index.
> That is a legal value in pg_class.relkind, but we forgot to list it in
> this test.

Yeah, in principle this check should allow any permissible relkind
value.  In practice, because it runs so early in the regression tests,
there's not many values present.  I added a quick check and found that
type_sanity only sees these values:
 
 -- **************** pg_class ****************
 -- Look for illegal values in pg_class fields
+select distinct relkind from pg_class order by 1;
+ relkind 
+---------
+ i
+ r
+ t
+ v
+(4 rows)
+
 SELECT c1.oid, c1.relname
 FROM pg_class as c1
 WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR

We've had some prior discussions about moving type_sanity, opr_sanity
etc to run later when there's a wider variety of objects present.
I'm not sure about that being a great idea though --- for example,
there's a test that creates an intentionally incomplete opclass
and even leaves it around for pg_dump stress testing.  That'd
probably annoy opr_sanity if it ran after that one.

The original motivation for type_sanity and friends was mostly
to detect mistakes in the hand-rolled initial catalog contents,
and for that purpose it's fine if they run early.  Some of what
they check is now redundant with genbki.pl I think.

Anyway, we should fix this if only for clarity's sake.
I do not feel a need to back-patch though.

            regards, tom lane

On Fri, Oct 27, 2023 at 09:44:30PM -0400, Tom Lane wrote:
> Anyway, we should fix this if only for clarity's sake.
> I do not feel a need to back-patch though.

Agreed.  Thanks for the commit.
--
Michael

looking around.
I found other three minor issues. attached.

I am not sure the pg_class "relam" description part is correct. since
partitioned indexes (relkind "I") also have the access method, but no
storage.
"
If this is a table or an index, the access method used (heap, B-tree,
hash, etc.); otherwise zero (zero occurs for sequences, as well as
relations without storage, such as views)
"

On Sat, Nov 11, 2023 at 08:00:00AM +0800, jian he wrote:
> I am not sure the pg_class "relam" description part is correct. since
> partitioned indexes (relkind "I") also have the access method, but no
> storage.
> "
> If this is a table or an index, the access method used (heap, B-tree,
> hash, etc.); otherwise zero (zero occurs for sequences, as well as
> relations without storage, such as views)
> "

This should be adjusted as well in the docs, IMO.  I would propose
something slightly more complicated:
"
If this is a table, index, materialized view or partitioned index, the
access method used (heap, B-tree, hash, etc.); otherwise zero (zero
occurs for sequences, as well as relations without storage, like
views).
"

> diff --git a/src/test/regress/sql/type_sanity.sql b/src/test/regress/sql/type_sanity.sql
> index a546ba89..6d806941 100644
> --- a/src/test/regress/sql/type_sanity.sql
> +++ b/src/test/regress/sql/type_sanity.sql

Ahah, nice catches.  I'll go adjust that on HEAD like the other one
you pointed out.  Just note that materialized views have a relam
defined, so the first comment you have changed is not completely
correct.
--
Michael

On Sat, Nov 11, 2023 at 09:38:53AM +0900, Michael Paquier wrote:
> On Sat, Nov 11, 2023 at 08:00:00AM +0800, jian he wrote:
>> diff --git a/src/test/regress/sql/type_sanity.sql b/src/test/regress/sql/type_sanity.sql
>> index a546ba89..6d806941 100644
>> --- a/src/test/regress/sql/type_sanity.sql
>> +++ b/src/test/regress/sql/type_sanity.sql
>
> Ahah, nice catches.  I'll go adjust that on HEAD like the other one
> you pointed out.  Just note that materialized views have a relam
> defined, so the first comment you have changed is not completely
> correct.

Fixed all that with a9f19c1349c2 for now.
--
Michael