[Tom Lane <tgl@sss.pgh.pa.us>]

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(select 1 from pg_depend where refobjid = lowoid and deptype = 'p');
  end if;
  if not pinned then
    raise notice '% contains unpinned 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


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