Re: Small omission in type_sanity.sql

Hi,

On 2023-01-18 14:51:32 -0500, Melanie Plageman wrote:
> I only changed these few lines in type_sanity to be more correct; I
> didn't change anything else in regress to actually exercise them (e.g.
> ensuring a partitioned table is around when running type_sanity). It
> might be worth moving type_sanity down in the parallel schedule?

Unfortunately it's not entirely trivial to do that. Despite the comment at the
top of type_sanity.sql:
-- None of the SELECTs here should ever find any matching entries,
-- so the expected output is easy to maintain ;-).

there are actually a few queries in there that are expected to return
objects. And would return more at the end of the tests.

That doesn't seem great.

Tom, is there a reason we run the various sanity tests early-ish in the
schedule? It does seem to reduce their effectiveness a bit...

Problems:
- shell types show up in a bunch of queries, e.g. "Look for illegal values in
pg_type fields." due to NOT t1.typisdefined
- the omission of various relkinds noted by Melanie shows in "Look for illegal
values in pg_class fields"
- pg_attribute query doesn't know about dropped columns
- "Cross-check against pg_type entry" is far too strict about legal combinations
of typstorage

> It does seem a bit hard to remember to update these tests in
> type_sanity.sql when adding some new value for a pg_class field. I
> wonder if there is a better way of testing this.

As evidenced by the above list of failures, moving the test to the end of the
regression tests would help, if we can get there.

> --- All tables and indexes should have an access method.
> -SELECT c1.oid, c1.relname
> -FROM pg_class as c1
> -WHERE c1.relkind NOT IN ('S', 'v', 'f', 'c') and
> - c1.relam = 0;
> - oid | relname
> ------+---------
> +-- All tables and indexes except partitioned tables should have an access
> +-- method.
> +SELECT oid, relname, relkind, relam
> +FROM pg_class
> +WHERE relkind NOT IN ('S', 'v', 'f', 'c', 'p') and
> + relam = 0;
> + oid | relname | relkind | relam
> +-----+---------+---------+-------
> (0 rows)

Don't think that one is right, a partitioned table doesn't have an AM.

> --- Conversely, sequences, views, types shouldn't have them
> -SELECT c1.oid, c1.relname
> -FROM pg_class as c1
> -WHERE c1.relkind IN ('S', 'v', 'f', 'c') and
> - c1.relam != 0;
> - oid | relname
> ------+---------
> +-- Conversely, sequences, views, types, and partitioned tables shouldn't have
> +-- them
> +SELECT oid, relname, relkind, relam
> +FROM pg_class
> +WHERE relkind IN ('S', 'v', 'f', 'c', 'p') and
> + relam != 0;
> + oid | relname | relkind | relam
> +-----+---------+---------+-------
> (0 rows)

Particularly because you include them again here :)

Greetings,

Andres Freund