Re: [HACKERS] Guarding against bugs-of-omission in initdb's setup_depend

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 22, 2017 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So I'm thinking about adding a regression test case, say in dependency.sql,
>> that looks for unpinned objects with OIDs in the hand-assigned range,
>> along the lines of this prototype code:

> I don't have specific thoughts, but I like the general idea.

Here's a more refined proposed patch.  After I tightened the pg_depend
probes to check refclassid, I started to get complaints about
pg_largeobject_metadata.  It turns out that large_object.sql creates a
large object with OID 3001, which triggered the check (but without the
classid test, it'd accidentally been fooled by the existence of a pin
entry for pg_proc OID 3001).  I'm not sure if it's such a hot idea to make
that large object; but on the whole it seems like this needs to be done
early in the regression tests so that it's not fooled by whatever random
objects might be created later in the tests.  I could not quite persuade
myself that checking pg_depend belonged in either opr_sanity or
type_sanity, so this patch sets up a "misc_sanity" test script to go
alongside those two.

            regards, tom lane

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 0f22e6d..b76eb1e 100644
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*************** setup_depend(FILE *cmdfd)
*** 1478,1486 ****
           * for instance) but generating only the minimum required set of
           * dependencies seems hard.
           *
!          * Note that we deliberately do not pin the system views, which
!          * haven't been created yet.  Also, no conversions, databases, or
!          * tablespaces are pinned.
           *
           * First delete any already-made entries; PINs override all else, and
           * must be the only entries for their objects.
--- 1478,1493 ----
           * for instance) but generating only the minimum required set of
           * dependencies seems hard.
           *
!          * Catalogs that are intentionally not scanned here are:
!          *
!          * pg_database: it's a feature, not a bug, that template1 is not
!          * pinned.
!          *
!          * pg_extension: a pinned extension isn't really an extension, hmm?
!          *
!          * pg_tablespace: tablespaces don't participate in the dependency
!          * code, and DropTableSpace() explicitly protects the built-in
!          * tablespaces.
           *
           * First delete any already-made entries; PINs override all else, and
           * must be the only entries for their objects.
*************** setup_depend(FILE *cmdfd)
*** 1501,1506 ****
--- 1508,1515 ----
          "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
          " FROM pg_constraint;\n\n",
          "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
+         " FROM pg_conversion;\n\n",
+         "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
          " FROM pg_attrdef;\n\n",
          "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
          " FROM pg_language;\n\n",
*************** initialize_data_directory(void)
*** 2906,2911 ****
--- 2915,2925 ----

      setup_depend(cmdfd);

+     /*
+      * Note that no objects created after setup_depend() will be "pinned".
+      * They are all droppable at the whim of the DBA.
+      */
+
      setup_sysviews(cmdfd);

      setup_description(cmdfd);
diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out
index ...f026896 .
*** a/src/test/regress/expected/misc_sanity.out
--- b/src/test/regress/expected/misc_sanity.out
***************
*** 0 ****
--- 1,78 ----
+ --
+ -- MISC_SANITY
+ -- Sanity checks for common errors in making system tables that don't fit
+ -- comfortably into either opr_sanity or type_sanity.
+ --
+ -- Every test failure in this file should be closely inspected.
+ -- The description of the failing test should be read carefully before
+ -- adjusting the expected output.  In most cases, the queries should
+ -- not find *any* matching entries.
+ --
+ -- NB: run this test early, because some later tests create bogus entries.
+ -- **************** pg_depend ****************
+ -- Look for illegal values in pg_depend fields.
+ -- classid/objid can be zero, but only in 'p' entries
+ SELECT *
+ FROM pg_depend as d1
+ WHERE refclassid = 0 OR refobjid = 0 OR
+       deptype NOT IN ('a', 'e', 'i', 'n', 'p') OR
+       (deptype != 'p' AND (classid = 0 OR objid = 0)) OR
+       (deptype = 'p' AND (classid != 0 OR objid != 0 OR objsubid != 0));
+  classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
+ ---------+-------+----------+------------+----------+-------------+---------
+ (0 rows)
+
+ -- **************** pg_shdepend ****************
+ -- Look for illegal values in pg_shdepend fields.
+ -- classid/objid can be zero, but only in 'p' entries
+ SELECT *
+ FROM pg_shdepend as d1
+ WHERE refclassid = 0 OR refobjid = 0 OR
+       deptype NOT IN ('a', 'o', 'p', 'r') OR
+       (deptype != 'p' AND (dbid = 0 OR classid = 0 OR objid = 0)) OR
+       (deptype = 'p' AND (dbid != 0 OR classid != 0 OR objid != 0 OR objsubid != 0));
+  dbid | classid | objid | objsubid | refclassid | refobjid | deptype
+ ------+---------+-------+----------+------------+----------+---------
+ (0 rows)
+
+ -- Check each OID-containing system catalog to see if its lowest-numbered OID
+ -- is pinned.  If not, and if that OID was generated during initdb, then
+ -- perhaps initdb forgot to scan that catalog for pinnable entries.
+ -- Generally, it's okay for a catalog to be listed in the output of this
+ -- test if that catalog is scanned by initdb.c's setup_depend() function;
+ -- whatever OID the test is complaining about must have been added later
+ -- in initdb, where it intentionally isn't pinned.  Legitimate exceptions
+ -- to that rule are listed in the comments in setup_depend().
+ do $$
+ declare relnm text;
+   reloid oid;
+   shared bool;
+   lowoid oid;
+   pinned bool;
+ begin
+ for relnm, reloid, shared in
+   select relname, oid, relisshared from pg_class
+   where relhasoids and oid < 16384 order by 1
+ loop
+   execute 'select min(oid) from ' || relnm into lowoid;
+   continue when lowoid is null or lowoid >= 16384;
+   if shared then
+     pinned := exists(select 1 from pg_shdepend
+                      where refclassid = reloid and refobjid = lowoid
+                      and deptype = 'p');
+   else
+     pinned := exists(select 1 from pg_depend
+                      where refclassid = reloid and refobjid = lowoid
+                      and deptype = 'p');
+   end if;
+   if not pinned then
+     raise notice '% contains unpinned initdb-created object(s)', relnm;
+   end if;
+ end loop;
+ end$$;
+ NOTICE:  pg_constraint contains unpinned initdb-created object(s)
+ NOTICE:  pg_conversion contains unpinned initdb-created object(s)
+ NOTICE:  pg_database contains unpinned initdb-created object(s)
+ NOTICE:  pg_extension contains unpinned initdb-created object(s)
+ NOTICE:  pg_rewrite contains unpinned initdb-created object(s)
+ NOTICE:  pg_tablespace contains unpinned initdb-created object(s)
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 1f8f098..2369261 100644
*** a/src/test/regress/parallel_schedule
--- b/src/test/regress/parallel_schedule
*************** test: point lseg line box path polygon c
*** 30,36 ****
  # geometry depends on point, lseg, box, path, polygon and circle
  # horology depends on interval, timetz, timestamp, timestamptz, reltime and abstime
  # ----------
! test: geometry horology regex oidjoins type_sanity opr_sanity comments expressions

  # ----------
  # These four each depend on the previous one
--- 30,36 ----
  # geometry depends on point, lseg, box, path, polygon and circle
  # horology depends on interval, timetz, timestamp, timestamptz, reltime and abstime
  # ----------
! test: geometry horology regex oidjoins type_sanity opr_sanity misc_sanity comments expressions

  # ----------
  # These four each depend on the previous one
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 04206c3..5e8b7e9 100644
*** a/src/test/regress/serial_schedule
--- b/src/test/regress/serial_schedule
*************** test: regex
*** 49,54 ****
--- 49,55 ----
  test: oidjoins
  test: type_sanity
  test: opr_sanity
+ test: misc_sanity
  test: comments
  test: expressions
  test: insert
diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql
index ...5130a4a .
*** a/src/test/regress/sql/misc_sanity.sql
--- b/src/test/regress/sql/misc_sanity.sql
***************
*** 0 ****
--- 1,74 ----
+ --
+ -- MISC_SANITY
+ -- Sanity checks for common errors in making system tables that don't fit
+ -- comfortably into either opr_sanity or type_sanity.
+ --
+ -- Every test failure in this file should be closely inspected.
+ -- The description of the failing test should be read carefully before
+ -- adjusting the expected output.  In most cases, the queries should
+ -- not find *any* matching entries.
+ --
+ -- NB: run this test early, because some later tests create bogus entries.
+
+
+ -- **************** pg_depend ****************
+
+ -- Look for illegal values in pg_depend fields.
+ -- classid/objid can be zero, but only in 'p' entries
+
+ SELECT *
+ FROM pg_depend as d1
+ WHERE refclassid = 0 OR refobjid = 0 OR
+       deptype NOT IN ('a', 'e', 'i', 'n', 'p') OR
+       (deptype != 'p' AND (classid = 0 OR objid = 0)) OR
+       (deptype = 'p' AND (classid != 0 OR objid != 0 OR objsubid != 0));
+
+ -- **************** pg_shdepend ****************
+
+ -- Look for illegal values in pg_shdepend fields.
+ -- classid/objid can be zero, but only in 'p' entries
+
+ SELECT *
+ FROM pg_shdepend as d1
+ WHERE refclassid = 0 OR refobjid = 0 OR
+       deptype NOT IN ('a', 'o', 'p', 'r') OR
+       (deptype != 'p' AND (dbid = 0 OR classid = 0 OR objid = 0)) OR
+       (deptype = 'p' AND (dbid != 0 OR classid != 0 OR objid != 0 OR objsubid != 0));
+
+
+ -- Check each OID-containing system catalog to see if its lowest-numbered OID
+ -- is pinned.  If not, and if that OID was generated during initdb, then
+ -- perhaps initdb forgot to scan that catalog for pinnable entries.
+ -- Generally, it's okay for a catalog to be listed in the output of this
+ -- test if that catalog is scanned by initdb.c's setup_depend() function;
+ -- whatever OID the test is complaining about must have been added later
+ -- in initdb, where it intentionally isn't pinned.  Legitimate exceptions
+ -- to that rule are listed in the comments in setup_depend().
+
+ do $$
+ declare relnm text;
+   reloid oid;
+   shared bool;
+   lowoid oid;
+   pinned bool;
+ begin
+ for relnm, reloid, shared in
+   select relname, oid, relisshared from pg_class
+   where relhasoids and oid < 16384 order by 1
+ loop
+   execute 'select min(oid) from ' || relnm into lowoid;
+   continue when lowoid is null or lowoid >= 16384;
+   if shared then
+     pinned := exists(select 1 from pg_shdepend
+                      where refclassid = reloid and refobjid = lowoid
+                      and deptype = 'p');
+   else
+     pinned := exists(select 1 from pg_depend
+                      where refclassid = reloid and refobjid = lowoid
+                      and deptype = 'p');
+   end if;
+   if not pinned then
+     raise notice '% contains unpinned initdb-created object(s)', relnm;
+   end if;
+ end loop;
+ end$$;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers