Small omission in type_sanity.sql
Hi,
I was playing around with splitting up the tablespace test in regress so
that I could use the tablespaces it creates in another test and happened
to notice that the pg_class validity checks in type_sanity.sql are
incomplete.
It seems that 8b08f7d4820fd did not update the pg_class tests in
type_sanity to include partitioned indexes and tables.
patch attached.
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?
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.
- Melanie
Attachments:
v1-0001-Update-pg_class-validity-test.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Update-pg_class-validity-test.patchDownload
From 1531cdf257af92d31836e50e1a0b865131f1a018 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 18 Jan 2023 14:26:36 -0500
Subject: [PATCH v1] Update pg_class validity test
Partitioned tables and indexes were not considered in the pg_class
validity checks in type_sanity.sql. This is not currently exercised
because all partitioned tables and indexes have been dropped by the time
type_sanity is run.
---
src/test/regress/expected/type_sanity.out | 44 ++++++++++++-----------
src/test/regress/sql/type_sanity.sql | 36 ++++++++++---------
2 files changed, 42 insertions(+), 38 deletions(-)
diff --git a/src/test/regress/expected/type_sanity.out b/src/test/regress/expected/type_sanity.out
index a640cfc476..95435e3de6 100644
--- a/src/test/regress/expected/type_sanity.out
+++ b/src/test/regress/expected/type_sanity.out
@@ -498,34 +498,36 @@ ORDER BY 1;
-- **************** pg_class ****************
-- Look for illegal values in pg_class fields
-SELECT c1.oid, c1.relname
-FROM pg_class as c1
-WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR
+SELECT oid, relname, relkind, relpersistence, relreplident
+FROM pg_class
+WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p', 'I') OR
relpersistence NOT IN ('p', 'u', 't') OR
relreplident NOT IN ('d', 'n', 'f', 'i');
- oid | relname
------+---------
+ oid | relname | relkind | relpersistence | relreplident
+-----+---------+---------+----------------+--------------
(0 rows)
--- 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)
--- 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)
--- Indexes should have AMs of type 'i'
+-- Indexes and partitioned indexes should have AMs of type 'i'
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
WHERE pc.relkind IN ('i') and
@@ -534,7 +536,7 @@ WHERE pc.relkind IN ('i') and
-----+---------+--------+--------
(0 rows)
--- Tables, matviews etc should have AMs of type 't'
+-- Tables, matviews etc should have AMs of type 't' (except partitioned tables)
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
WHERE pc.relkind IN ('r', 't', 'm') and
diff --git a/src/test/regress/sql/type_sanity.sql b/src/test/regress/sql/type_sanity.sql
index 79ec410a6c..22a2dba94d 100644
--- a/src/test/regress/sql/type_sanity.sql
+++ b/src/test/regress/sql/type_sanity.sql
@@ -358,31 +358,33 @@ ORDER BY 1;
-- Look for illegal values in pg_class fields
-SELECT c1.oid, c1.relname
-FROM pg_class as c1
-WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p') OR
+SELECT oid, relname, relkind, relpersistence, relreplident
+FROM pg_class
+WHERE relkind NOT IN ('r', 'i', 'S', 't', 'v', 'm', 'c', 'f', 'p', 'I') OR
relpersistence NOT IN ('p', 'u', 't') OR
relreplident NOT IN ('d', 'n', 'f', 'i');
--- 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;
-
--- 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;
-
--- Indexes should have AMs of type 'i'
+-- 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;
+
+-- 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;
+
+-- Indexes and partitioned indexes should have AMs of type 'i'
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
WHERE pc.relkind IN ('i') and
pa.amtype != 'i';
--- Tables, matviews etc should have AMs of type 't'
+-- Tables, matviews etc should have AMs of type 't' (except partitioned tables)
SELECT pc.oid, pc.relname, pa.amname, pa.amtype
FROM pg_class as pc JOIN pg_am AS pa ON (pc.relam = pa.oid)
WHERE pc.relkind IN ('r', 't', 'm') and
--
2.34.1
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
Andres Freund <andres@anarazel.de> writes:
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...
Originally, those tests were mainly needed to sanity-check the
hand-maintained initial catalog data, so it made sense to run them
early. Since we taught genbki.pl to do a bunch more work, that's
perhaps a bit less pressing.
There's at least one test that intentionally sets up a bogus btree
opclass, which we'd have to drop again if we wanted to run the
sanity checks later. Not sure what other issues might surface.
You could find out easily enough, of course ...
Problems:
- "Cross-check against pg_type entry" is far too strict about legal combinations
of typstorage
Perhaps, but it's enforcing policy about what we want in the
initial catalog data, not what is possible to support. So
there's a bit of divergence of goals here too. Maybe we need
to split up the tests into initial-data-only tests (run early)
and tests that should hold for user-created objects too
(run late)?
regards, tom lane
Hi,
On 2023-01-27 20:39:04 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
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...Originally, those tests were mainly needed to sanity-check the
hand-maintained initial catalog data, so it made sense to run them
early.
It's also kinda useful to have some basic validity testing early on, because
if there's something wrong with the catalog values, it'll cause lots of issues
later.
Problems:
- "Cross-check against pg_type entry" is far too strict about legal combinations
of typstoragePerhaps, but it's enforcing policy about what we want in the
initial catalog data, not what is possible to support.
True in generaly, but I don't think it matters much in this specific case. We
don't gain much by forbidding 'e' -> 'x' mismatches, given that we allow 'x'
-> 'p'.
There's a lot more such cases in opr_sanity. There's a lot of tests in it that
only make sense for validating the initial catalog contents. It might be
useful to run a more lenient version of it later though.
So there's a bit of divergence of goals here too. Maybe we need to split up
the tests into initial-data-only tests (run early) and tests that should
hold for user-created objects too (run late)?
Yea, I think so. A bit worried about the duplication that might require.
But the *sanity tests also do also encode a lot of good cross-checks, that are
somewhat easy to break in code (and / or have been broken in the past), so I
think it's worth pursuing.
Greetings,
Andres Freund
On 2023-Jan-27, Andres Freund wrote:
On 2023-01-27 20:39:04 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
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...Originally, those tests were mainly needed to sanity-check the
hand-maintained initial catalog data, so it made sense to run them
early.It's also kinda useful to have some basic validity testing early on, because
if there's something wrong with the catalog values, it'll cause lots of issues
later.
We can just list the tests twice in the schedule ...
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/