Re: pgsql: Sort the dependent objects before recursing in findDependentObje

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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=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:s0 tclass=db_column name="regtest_schema.regtest_table.cmax"

Yay for having to fix tests blindly :(

Greetings,

Andres Freund