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

While thinking about something else, it started to bother me that
initdb's setup_depend() function knows exactly which catalogs might
contain pinnable objects.  It is not very hard to imagine that somebody
might add a DATA() line to, say, pg_transform.h and expect that the
represented object could not get dropped.  Well, tain't so, because
setup_depend() doesn't collect OIDs from there.

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:

do $$
declare relnm text; shared bool; lowoid oid; pinned bool;
begin
for relnm, shared in  select relname, 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 >= 10000; if shared
then  pinned := exists(select 1 from pg_shdepend where refobjid = lowoid and deptype = 'p'); else   pinned :=
exists(select1 from pg_depend where refobjid = lowoid and deptype = 'p'); end if; if not pinned then   raise notice '%
containsunpinned object with OID %', relnm, lowoid; end if; 
end loop;
end$$;

At the moment, this prints

NOTICE:  pg_database contains unpinned object with OID 1
NOTICE:  pg_tablespace contains unpinned object with OID 1663

It's intentional that no databases are pinned, so I'm inclined to
explicitly skip pg_database in the test, or else to allow the above
to remain in the test's expected output.  The fact that the builtin
tablespaces aren't pinned is okay, because DropTablespace contains an
explicit privilege check preventing dropping them, but I wonder whether
we shouldn't adjust setup_depend() to pin them anyway.  Otherwise we
can do one of the above two things for pg_tablespace as well.

Another thought is that while this test would be enough to ensure that
there are no unpinned OIDs that the C code knows about explicitly through
#defines, it's certainly conceivable that there might be DATA() lines
without hand-assigned OIDs that the system nonetheless is really reliant
on being there.  So it feels like maybe the test isn't strong enough.
We could change the OID cutoff to be 16384 not 10000, in which case
we also get complaints about pg_constraint, pg_conversion, pg_extension,
and pg_rewrite.  pg_constraint and pg_rewrite are safe enough, because
setup_depend() does scan those, and surely pg_extension is too (a pinned
extension seems like a contradiction in terms).  But as for pg_conversion,
it seems a bit scary that setup_depend() doesn't scan it.

So what I'm thinking about doing is adding a test like the above,
with OID cutoff 16384 and no printing of specific OIDs (because they
could change).  The expected output would be annotated with a comment
like "if a new catalog starts appearing here, that's probably okay,
but make sure setup_depend() is scanning it for OIDs that should be
pinned".  I'd want to add pg_conversion to setup_depend(), too.

We might want to pre-emptively add some other catalogs such as pg_policy
and pg_transform while we are at it.  But creating this regression test
ought to be enough to ensure that we don't start needing to scan those
catalogs without knowing it, so I don't feel that it's urgent to do so.

Thoughts?
        regards, tom lane

On Thu, Jun 22, 2017 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While thinking about something else, it started to bother me that
> initdb's setup_depend() function knows exactly which catalogs might
> contain pinnable objects.  It is not very hard to imagine that somebody
> might add a DATA() line to, say, pg_transform.h and expect that the
> represented object could not get dropped.  Well, tain't so, because
> setup_depend() doesn't collect OIDs from there.
>
> 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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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