Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8be8510

Browse files
committed
Add testing to detect errors of omission in "pin" dependency creation.
It's essential that initdb.c's setup_depend() scan each system catalog that could contain objects that need to have "p" (pin) entries in pg_depend or pg_shdepend. Forgetting to add that, either when a catalog is first invented or when it first acquires DATA() entries, is an obvious bug hazard. We can detect such omissions at reasonable cost by probing every OID-containing system catalog to see whether the lowest-numbered OID in it is pinned. If so, the catalog must have been properly accounted for in setup_depend(). If the lowest OID is above FirstNormalObjectId then the catalog must have been empty at the end of initdb, so it doesn't matter. There are a small number of catalogs whose first entry is made later in initdb than setup_depend(), resulting in nonempty expected output of the test, but these can be manually inspected to see that they are OK. Any future mistake of this ilk will manifest as a new entry in the test's output. Since pg_conversion is already in the test's output, add it to the set of catalogs scanned by setup_depend(). That has no effect today (hence, no catversion bump here) but it will protect us if we ever do add pin-worthy conversions. This test is very much like the catalog sanity checks embodied in opr_sanity.sql and type_sanity.sql, but testing pg_depend doesn't seem to fit naturally into either of those scripts' charters. Hence, invent a new test script misc_sanity.sql, which can be a home for this as well as tests on any other catalogs we might want in future. Discussion: https://postgr.es/m/8068.1498155068@sss.pgh.pa.us
1 parent da23228 commit 8be8510

File tree

5 files changed

+171
-4
lines changed

5 files changed

+171
-4
lines changed

src/bin/initdb/initdb.c

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1478,9 +1478,16 @@ setup_depend(FILE *cmdfd)
14781478
* for instance) but generating only the minimum required set of
14791479
* dependencies seems hard.
14801480
*
1481-
* Note that we deliberately do not pin the system views, which
1482-
* haven't been created yet. Also, no conversions, databases, or
1483-
* tablespaces are pinned.
1481+
* Catalogs that are intentionally not scanned here are:
1482+
*
1483+
* pg_database: it's a feature, not a bug, that template1 is not
1484+
* pinned.
1485+
*
1486+
* pg_extension: a pinned extension isn't really an extension, hmm?
1487+
*
1488+
* pg_tablespace: tablespaces don't participate in the dependency
1489+
* code, and DropTableSpace() explicitly protects the built-in
1490+
* tablespaces.
14841491
*
14851492
* First delete any already-made entries; PINs override all else, and
14861493
* must be the only entries for their objects.
@@ -1501,6 +1508,8 @@ setup_depend(FILE *cmdfd)
15011508
"INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
15021509
" FROM pg_constraint;\n\n",
15031510
"INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
1511+
" FROM pg_conversion;\n\n",
1512+
"INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
15041513
" FROM pg_attrdef;\n\n",
15051514
"INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' "
15061515
" FROM pg_language;\n\n",
@@ -2906,6 +2915,11 @@ initialize_data_directory(void)
29062915

29072916
setup_depend(cmdfd);
29082917

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

29112925
setup_description(cmdfd);
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
--
2+
-- MISC_SANITY
3+
-- Sanity checks for common errors in making system tables that don't fit
4+
-- comfortably into either opr_sanity or type_sanity.
5+
--
6+
-- Every test failure in this file should be closely inspected.
7+
-- The description of the failing test should be read carefully before
8+
-- adjusting the expected output. In most cases, the queries should
9+
-- not find *any* matching entries.
10+
--
11+
-- NB: run this test early, because some later tests create bogus entries.
12+
-- **************** pg_depend ****************
13+
-- Look for illegal values in pg_depend fields.
14+
-- classid/objid can be zero, but only in 'p' entries
15+
SELECT *
16+
FROM pg_depend as d1
17+
WHERE refclassid = 0 OR refobjid = 0 OR
18+
deptype NOT IN ('a', 'e', 'i', 'n', 'p') OR
19+
(deptype != 'p' AND (classid = 0 OR objid = 0)) OR
20+
(deptype = 'p' AND (classid != 0 OR objid != 0 OR objsubid != 0));
21+
classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
22+
---------+-------+----------+------------+----------+-------------+---------
23+
(0 rows)
24+
25+
-- **************** pg_shdepend ****************
26+
-- Look for illegal values in pg_shdepend fields.
27+
-- classid/objid can be zero, but only in 'p' entries
28+
SELECT *
29+
FROM pg_shdepend as d1
30+
WHERE refclassid = 0 OR refobjid = 0 OR
31+
deptype NOT IN ('a', 'o', 'p', 'r') OR
32+
(deptype != 'p' AND (dbid = 0 OR classid = 0 OR objid = 0)) OR
33+
(deptype = 'p' AND (dbid != 0 OR classid != 0 OR objid != 0 OR objsubid != 0));
34+
dbid | classid | objid | objsubid | refclassid | refobjid | deptype
35+
------+---------+-------+----------+------------+----------+---------
36+
(0 rows)
37+
38+
-- Check each OID-containing system catalog to see if its lowest-numbered OID
39+
-- is pinned. If not, and if that OID was generated during initdb, then
40+
-- perhaps initdb forgot to scan that catalog for pinnable entries.
41+
-- Generally, it's okay for a catalog to be listed in the output of this
42+
-- test if that catalog is scanned by initdb.c's setup_depend() function;
43+
-- whatever OID the test is complaining about must have been added later
44+
-- in initdb, where it intentionally isn't pinned. Legitimate exceptions
45+
-- to that rule are listed in the comments in setup_depend().
46+
do $$
47+
declare relnm text;
48+
reloid oid;
49+
shared bool;
50+
lowoid oid;
51+
pinned bool;
52+
begin
53+
for relnm, reloid, shared in
54+
select relname, oid, relisshared from pg_class
55+
where relhasoids and oid < 16384 order by 1
56+
loop
57+
execute 'select min(oid) from ' || relnm into lowoid;
58+
continue when lowoid is null or lowoid >= 16384;
59+
if shared then
60+
pinned := exists(select 1 from pg_shdepend
61+
where refclassid = reloid and refobjid = lowoid
62+
and deptype = 'p');
63+
else
64+
pinned := exists(select 1 from pg_depend
65+
where refclassid = reloid and refobjid = lowoid
66+
and deptype = 'p');
67+
end if;
68+
if not pinned then
69+
raise notice '% contains unpinned initdb-created object(s)', relnm;
70+
end if;
71+
end loop;
72+
end$$;
73+
NOTICE: pg_constraint contains unpinned initdb-created object(s)
74+
NOTICE: pg_conversion contains unpinned initdb-created object(s)
75+
NOTICE: pg_database contains unpinned initdb-created object(s)
76+
NOTICE: pg_extension contains unpinned initdb-created object(s)
77+
NOTICE: pg_rewrite contains unpinned initdb-created object(s)
78+
NOTICE: pg_tablespace contains unpinned initdb-created object(s)

src/test/regress/parallel_schedule

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ test: point lseg line box path polygon circle date time timetz timestamp timesta
3030
# geometry depends on point, lseg, box, path, polygon and circle
3131
# horology depends on interval, timetz, timestamp, timestamptz, reltime and abstime
3232
# ----------
33-
test: geometry horology regex oidjoins type_sanity opr_sanity comments expressions
33+
test: geometry horology regex oidjoins type_sanity opr_sanity misc_sanity comments expressions
3434

3535
# ----------
3636
# These four each depend on the previous one

src/test/regress/serial_schedule

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ test: regex
4949
test: oidjoins
5050
test: type_sanity
5151
test: opr_sanity
52+
test: misc_sanity
5253
test: comments
5354
test: expressions
5455
test: insert

src/test/regress/sql/misc_sanity.sql

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
--
2+
-- MISC_SANITY
3+
-- Sanity checks for common errors in making system tables that don't fit
4+
-- comfortably into either opr_sanity or type_sanity.
5+
--
6+
-- Every test failure in this file should be closely inspected.
7+
-- The description of the failing test should be read carefully before
8+
-- adjusting the expected output. In most cases, the queries should
9+
-- not find *any* matching entries.
10+
--
11+
-- NB: run this test early, because some later tests create bogus entries.
12+
13+
14+
-- **************** pg_depend ****************
15+
16+
-- Look for illegal values in pg_depend fields.
17+
-- classid/objid can be zero, but only in 'p' entries
18+
19+
SELECT *
20+
FROM pg_depend as d1
21+
WHERE refclassid = 0 OR refobjid = 0 OR
22+
deptype NOT IN ('a', 'e', 'i', 'n', 'p') OR
23+
(deptype != 'p' AND (classid = 0 OR objid = 0)) OR
24+
(deptype = 'p' AND (classid != 0 OR objid != 0 OR objsubid != 0));
25+
26+
-- **************** pg_shdepend ****************
27+
28+
-- Look for illegal values in pg_shdepend fields.
29+
-- classid/objid can be zero, but only in 'p' entries
30+
31+
SELECT *
32+
FROM pg_shdepend as d1
33+
WHERE refclassid = 0 OR refobjid = 0 OR
34+
deptype NOT IN ('a', 'o', 'p', 'r') OR
35+
(deptype != 'p' AND (dbid = 0 OR classid = 0 OR objid = 0)) OR
36+
(deptype = 'p' AND (dbid != 0 OR classid != 0 OR objid != 0 OR objsubid != 0));
37+
38+
39+
-- Check each OID-containing system catalog to see if its lowest-numbered OID
40+
-- is pinned. If not, and if that OID was generated during initdb, then
41+
-- perhaps initdb forgot to scan that catalog for pinnable entries.
42+
-- Generally, it's okay for a catalog to be listed in the output of this
43+
-- test if that catalog is scanned by initdb.c's setup_depend() function;
44+
-- whatever OID the test is complaining about must have been added later
45+
-- in initdb, where it intentionally isn't pinned. Legitimate exceptions
46+
-- to that rule are listed in the comments in setup_depend().
47+
48+
do $$
49+
declare relnm text;
50+
reloid oid;
51+
shared bool;
52+
lowoid oid;
53+
pinned bool;
54+
begin
55+
for relnm, reloid, shared in
56+
select relname, oid, relisshared from pg_class
57+
where relhasoids and oid < 16384 order by 1
58+
loop
59+
execute 'select min(oid) from ' || relnm into lowoid;
60+
continue when lowoid is null or lowoid >= 16384;
61+
if shared then
62+
pinned := exists(select 1 from pg_shdepend
63+
where refclassid = reloid and refobjid = lowoid
64+
and deptype = 'p');
65+
else
66+
pinned := exists(select 1 from pg_depend
67+
where refclassid = reloid and refobjid = lowoid
68+
and deptype = 'p');
69+
end if;
70+
if not pinned then
71+
raise notice '% contains unpinned initdb-created object(s)', relnm;
72+
end if;
73+
end loop;
74+
end$$;

0 commit comments

Comments
 (0)