Re: pgsql: Sort the dependent objects before recursing infindDependentObje

On 2019-01-21 18:48:25 +0000, Tom Lane wrote:
> Sort the dependent objects before recursing in findDependentObjects().
> 
> Historically, the notices output by DROP CASCADE tended to come out
> in uncertain order, and in some cases you might get different claims
> about which object depends on which other one.  This is because we
> just traversed the dependency tree in the order in which pg_depend
> entries are seen, and nbtree has never promised anything about the
> order of equal-keyed index entries.  We've put up with that for years,
> hacking regression tests when necessary to prevent them from emitting
> unstable output.  However, it's a problem for pending work that will
> change nbtree's behavior for equal keys, as that causes unexpected
> changes in the regression test results.
> 
> Hence, adjust findDependentObjects to sort the results of each
> indexscan before processing them.  The sort is on descending OID of
> the dependent objects, hence more or less reverse creation order.
> While this rule could still result in bogus regression test failures
> if an OID wraparound occurred mid-test, that seems unlikely to happen
> in any plausible development or packaging-test scenario.
> 
> This is enough to ensure output stability for ordinary DROP CASCADE
> commands, but not for DROP OWNED BY, because that has a different
> code path with the same problem.  We might later choose to sort in
> the DROP OWNED BY code as well, but this patch doesn't do so.
> 
> I've also not done anything about reverting the existing hacks to
> suppress unstable DROP CASCADE output in specific regression tests.
> It might be worth undoing those, but it seems like a distinct question.
> 
> The first indexscan loop in findDependentObjects is not touched,
> meaning there is a hazard of unstable error reports from that too.
> However, said hazard is not the fault of that code: it was designed
> on the assumption that there could be at most one "owning" object
> to complain about, and that assumption does not seem unreasonable.
> The recent patch that added the possibility of multiple
> DEPENDENCY_INTERNAL_AUTO links broke that assumption, but we should
> fix that situation not band-aid around it.  That's a matter for
> another patch, though.

Seems this affects the sepgsql tests:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2019-01-21%2019%3A45%3A02

--- /opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/expected/ddl.out    2018-11-20
21:45:02.786933457-0800
 
+++ /opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/results/ddl.out    2019-01-21
11:51:23.178651387-0800
 
@@ -421,10 +421,10 @@
 LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_ptable.q"
 
 DROP TABLE regtest_table;
 LOG:  SELinux: allowed { remove_name } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_schema_t:s0tclass=db_schema name="regtest_schema"
 
-LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_seq_t:s0tclass=db_sequence name="regtest_schema.regtest_table_x_seq"
 
-LOG:  SELinux: allowed { remove_name } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_schema_t:s0tclass=db_schema name="regtest_schema"
 
 LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_table name="regtest_schema.regtest_table"
 
 LOG:  SELinux: allowed { remove_name } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_schema_t:s0tclass=db_schema name="regtest_schema"
 
+LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_seq_t:s0tclass=db_sequence name="regtest_schema.regtest_table_x_seq"
 
+LOG:  SELinux: allowed { remove_name } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_schema_t:s0tclass=db_schema name="regtest_schema"
 
 LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_table name="regtest_schema.regtest_table"
 
 LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table.tableoid"
 
 LOG:  SELinux: allowed { drop } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table.cmax"
 

Yay for having to fix tests blindly :(

Greetings,

Andres Freund