Horton

Thread

Commits

  1. Add testing to detect errors of omission in "pin" dependency creation.

  1. Guarding against bugs-of-omission in initdb's setup_depend

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-22T18:11:08Z

    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

  2. Re: Guarding against bugs-of-omission in initdb's setup_depend

    Robert Haas <robertmhaas@gmail.com> — 2017-06-22T20:35:34Z

    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

  3. Re: Guarding against bugs-of-omission in initdb's setup_depend

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-23T04:33:33Z

    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