Feedback-ID: id4a34324:Fastmail
Date: Fri, 27 Jan 2023 17:25:09 -0800
From: Andres Freund <andres@anarazel.de>
To: Melanie Plageman <melanieplageman@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Small omission in type_sanity.sql
Message-ID: <20230128012509.5iwjktq4vu4kblef@awork3.anarazel.de>
References: <CAAKRu_ZZnN7XBDsWwEDyE1X=y3xs7GX-L=ftDk9gUg_4sX_awg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAKRu_ZZnN7XBDsWwEDyE1X=y3xs7GX-L=ftDk9gUg_4sX_awg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230128012509.5iwjktq4vu4kblef%40awork3.anarazel.de>
Precedence: bulk

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