Thread: 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

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 typstorage
> 
> Perhaps, 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/