On Thu, Nov 15, 2018 at 10:52 AM Robert Haas <robertmh...@gmail.com> wrote: > I think that the solution that you are proposing sorta sucks, but it's > better than hacking objsubid to do it, which did in fact pass the > laugh test, in that I laughed when I read it. :-)
Probably a good idea to get another cup of coffee if I'm pre-apologizing for my ideas. > In a perfect world, it seems to me that what we ought to do is have > some real logic in the server that figures out which of the various > things we could report would be most likely to be useful to the user > ... but that's probably a non-trivial project involving a fair amount > of human judgement. Reasonable people may differ about what is best, > never mind unreasonable people. I am inclined to think that your > proposal here is good enough for now, and somebody who dislikes it > (surely such a person will exist) can decide to look for ways to make > it better. Great. Actually, the on-disk size of the pg_depend heap relation is *unchanged* in the attached WIP patch, since it fits in a hole previously lost to alignment. And, as I said, the indexes end up smaller with suffix truncation. Even if the only thing you care about is the on-disk size of system catalogs, you'll still pretty reliably come out ahead. The design here is squirrelly, but we're already relying on scan order to reach objsubid = 0 entries first. There is a single tiny behavioral change to the regression test output with this patch applied. I think that that's just because there is one place where this dependency management stuff interacts with pages full of duplicates, and therefore stops putting duplicates in pg_depend indexes in exactly DESC TID order. My other patches add a couple of more tiny changes along similar lines, since of course I'm only doing this with the pg_depend indexes, and not for every system catalog index. -- Peter Geoghegan
From 7158fa1f93447d1f9bb296605a65b0fecfd54210 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan <p...@bowt.ie> Date: Tue, 13 Nov 2018 18:14:23 -0800 Subject: [PATCH 01/18] Add pg_depend index scan tiebreaker column. findDependentObjects() and other code that scans pg_depend evolved to depend on pg_depend_depender_index and pg_depend_reference_index scan order. This is clear from running the regression tests with "ignore_system_indexes=on": much of the test output changes for regression tests that happen to have DROP diagnostic messages. More importantly, a small number of those test failures involve alternative messages that are objectively useless or even harmful. These regressions all involve internal dependencies of one form or another. For example, we can go from a HINT that suggests dropping a partitioning parent table's trigger in order to drop the child's trigger to a HINT that suggests simply dropping the child table. This HINT is technically still correct from the point of view of findDependentObjects(), and yet it's clearly not the intended user-visible behavior. Make dependency.c take responsibility for its dependency on scan order by commenting on it directly. Ensure that the behavior of findDependentObjects() is deterministic in the event of duplicates by adding a new per-backend sequentially assigned number column to pg_depend. Both indexes now use this new column as a final trailing attribute, effectively making it a tiebreaker wherever processing order happens to matter. The new column is a per-backend sequentially assigned number. New values are assigned in decreasing order. That produces the behavior that's already expected from nbtree scans in the event of duplicate index entries (nbtree usually leaves duplicate index entries in reverse insertion order at present). A similar new column is not needed for pg_shdepend because the aforementioned harmful changes only occur with cases involving internal dependencies. The overall effect is to stabilize the behavior of DROP diagnostic messages, making it possible to avoid the "\set VERBOSITY=terse" hack that has been used to paper over test instability in the past. We may wish to go through the regression tests and remove existing instances of the "\set VERBOSITY=terse" hack (see also: commit 8e753726), but that's left for later. An upcoming patch to make nbtree store duplicate entries in a well-defined order by treating heap TID as a tiebreaker tuple attribute more or less flips the order that duplicates appear (the order will change from approximately descending to perfectly ascending). That would cause all sorts of problems for findDependentObjects() callers if this groundwork was skipped. The nbtree patch series will more than compensate for the overhead of adding the new column: both pg_depend indexes end up slightly smaller after the regression tests are run once suffix truncation is used. There is no change in the on-disk size of pg_depend heap relations on 64-bit platforms, since the new column fits in a space that was previously lost to alignment. The MAXALIGN()'d size of pg_depend heap tuples remains 56 bytes (including tuple header overhead). Discussion: https://postgr.es/m/CAH2-Wzkypv1R+teZrr71U23J578NnTBt2X8+Y=odr4podw1...@mail.gmail.com Discussion: https://postgr.es/m/11852.1501610262%40sss.pgh.pa.us --- doc/src/sgml/catalogs.sgml | 17 ++++++++- src/backend/catalog/dependency.c | 10 ++++++ src/backend/catalog/pg_depend.c | 10 +++++- src/bin/initdb/initdb.c | 44 +++++++++++------------ src/include/catalog/indexing.h | 4 +-- src/include/catalog/pg_depend.h | 1 + src/test/regress/expected/alter_table.out | 2 +- src/test/regress/expected/misc_sanity.out | 4 +-- 8 files changed, 63 insertions(+), 29 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 8b7f169d50..af2514913e 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2952,6 +2952,20 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l </entry> </row> + <row> + <entry><structfield>depcreate</structfield></entry> + <entry><type>int4</type></entry> + <entry></entry> + <entry> + A per-backend sequentially assigned number for this dependency + relationship. Used as a tiebreaker in the event of multiple + internal dependency relationships of otherwise equal + precedence. Identifiers are assigned in descending order to + ensure that the most recently entered dependency is the one + referenced by <literal>HINT</literal> fields. + </entry> + </row> + </tbody> </tgroup> </table> @@ -3069,7 +3083,8 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l that the system itself depends on the referenced object, and so that object must never be deleted. Entries of this type are created only by <command>initdb</command>. The columns for the - dependent object contain zeroes. + dependent object and the <structfield>depcreate</structfield> + column contain zeroes. </para> </listitem> </varlistentry> diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 7dfa3278a5..d7889c2cd0 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -532,6 +532,11 @@ findDependentObjects(const ObjectAddress *object, else nkeys = 2; + /* + * Note that we rely on DependDependerIndexId scan order to make + * diagnostic messages deterministic. (e.g., objsubid = 0 entries will be + * processed before other entries for the same dependent object.) + */ scan = systable_beginscan(*depRel, DependDependerIndexId, true, NULL, nkeys, key); @@ -727,6 +732,11 @@ findDependentObjects(const ObjectAddress *object, else nkeys = 2; + /* + * Note that we rely on DependReferenceIndexId scan order to make + * diagnostic messages deterministic. (e.g., refobjsubid = 0 entries will + * be processed before other entries for the same referenced object.) + */ scan = systable_beginscan(*depRel, DependReferenceIndexId, true, NULL, nkeys, key); diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 2ea05f350b..695330c2af 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -64,6 +64,7 @@ recordMultipleDependencies(const ObjectAddress *depender, int i; bool nulls[Natts_pg_depend]; Datum values[Natts_pg_depend]; + static int32 depcreate = INT32_MAX; if (nreferenced <= 0) return; /* nothing to do */ @@ -93,7 +94,10 @@ recordMultipleDependencies(const ObjectAddress *depender, { /* * Record the Dependency. Note we don't bother to check for - * duplicate dependencies; there's no harm in them. + * duplicate dependencies; there's no harm in them. Note that + * depcreate ensures deterministic processing among dependencies + * of otherwise equal precedence (e.g., among multiple entries of + * the same refclassid + refobjid + refobjsubid). */ values[Anum_pg_depend_classid - 1] = ObjectIdGetDatum(depender->classId); values[Anum_pg_depend_objid - 1] = ObjectIdGetDatum(depender->objectId); @@ -104,12 +108,16 @@ recordMultipleDependencies(const ObjectAddress *depender, values[Anum_pg_depend_refobjsubid - 1] = Int32GetDatum(referenced->objectSubId); values[Anum_pg_depend_deptype - 1] = CharGetDatum((char) behavior); + values[Anum_pg_depend_depcreate - 1] = Int32GetDatum(depcreate--); tup = heap_form_tuple(dependDesc->rd_att, values, nulls); /* fetch index info only when we know we need it */ if (indstate == NULL) indstate = CatalogOpenIndexes(dependDesc); + /* avoid signed underflow */ + if (depcreate == INT_MIN) + depcreate = INT_MAX; CatalogTupleInsertWithInfo(dependDesc, tup, indstate); diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index ab5cb7f0c1..d013d184c7 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -1619,55 +1619,55 @@ setup_depend(FILE *cmdfd) "DELETE FROM pg_shdepend;\n\n", "VACUUM pg_shdepend;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_class;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_proc;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_type;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_cast;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_constraint;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_conversion;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_attrdef;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_language;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_operator;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_opclass;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_opfamily;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_am;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_amop;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_amproc;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_rewrite;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_trigger;\n\n", /* * restriction here to avoid pinning the public namespace */ - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_namespace " " WHERE nspname LIKE 'pg%';\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_ts_parser;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_ts_dict;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_ts_template;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_ts_config;\n\n", - "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p' " + "INSERT INTO pg_depend SELECT 0,0,0, tableoid,oid,0, 'p',0 " " FROM pg_collation;\n\n", "INSERT INTO pg_shdepend SELECT 0,0,0,0, tableoid,oid, 'p' " " FROM pg_authid;\n\n", diff --git a/src/include/catalog/indexing.h b/src/include/catalog/indexing.h index 254fbef1f7..d3e7699003 100644 --- a/src/include/catalog/indexing.h +++ b/src/include/catalog/indexing.h @@ -142,9 +142,9 @@ DECLARE_UNIQUE_INDEX(pg_database_datname_index, 2671, on pg_database using btree DECLARE_UNIQUE_INDEX(pg_database_oid_index, 2672, on pg_database using btree(oid oid_ops)); #define DatabaseOidIndexId 2672 -DECLARE_INDEX(pg_depend_depender_index, 2673, on pg_depend using btree(classid oid_ops, objid oid_ops, objsubid int4_ops)); +DECLARE_INDEX(pg_depend_depender_index, 2673, on pg_depend using btree(classid oid_ops, objid oid_ops, objsubid int4_ops, depcreate int4_ops)); #define DependDependerIndexId 2673 -DECLARE_INDEX(pg_depend_reference_index, 2674, on pg_depend using btree(refclassid oid_ops, refobjid oid_ops, refobjsubid int4_ops)); +DECLARE_INDEX(pg_depend_reference_index, 2674, on pg_depend using btree(refclassid oid_ops, refobjid oid_ops, refobjsubid int4_ops, depcreate int4_ops)); #define DependReferenceIndexId 2674 DECLARE_UNIQUE_INDEX(pg_description_o_c_o_index, 2675, on pg_description using btree(objoid oid_ops, classoid oid_ops, objsubid int4_ops)); diff --git a/src/include/catalog/pg_depend.h b/src/include/catalog/pg_depend.h index 482b8bd251..1e748b909e 100644 --- a/src/include/catalog/pg_depend.h +++ b/src/include/catalog/pg_depend.h @@ -61,6 +61,7 @@ CATALOG(pg_depend,2608,DependRelationId) BKI_WITHOUT_OIDS * field. See DependencyType in catalog/dependency.h. */ char deptype; /* see codes in dependency.h */ + int32 depcreate; /* per-backend identifier; tiebreaker */ } FormData_pg_depend; /* ---------------- diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 0aa13b3cec..a8a77c44fd 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2688,10 +2688,10 @@ DETAIL: drop cascades to table alter2.t1 drop cascades to view alter2.v1 drop cascades to function alter2.plus1(integer) drop cascades to type alter2.posint -drop cascades to operator family alter2.ctype_hash_ops for access method hash drop cascades to type alter2.ctype drop cascades to function alter2.same(alter2.ctype,alter2.ctype) drop cascades to operator alter2.=(alter2.ctype,alter2.ctype) +drop cascades to operator family alter2.ctype_hash_ops for access method hash drop cascades to conversion alter2.ascii_to_utf8 drop cascades to text search parser alter2.prs drop cascades to text search configuration alter2.cfg diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out index 2d3522b500..8dcdc1aebe 100644 --- a/src/test/regress/expected/misc_sanity.out +++ b/src/test/regress/expected/misc_sanity.out @@ -18,8 +18,8 @@ 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 ----------+-------+----------+------------+----------+-------------+--------- + classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype | depcreate +---------+-------+----------+------------+----------+-------------+---------+----------- (0 rows) -- **************** pg_shdepend **************** -- 2.17.1
