Thread: pgsql: Sort the dependent objects before recursing infindDependentObje
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. Discussion: https://postgr.es/m/12244.1547854440@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/f1ad067fc3ae3d34dc6b8826913d454cc3fe354f Modified Files -------------- src/backend/catalog/dependency.c | 100 ++++++++++++++++++++++---- src/test/regress/expected/alter_table.out | 2 +- src/test/regress/expected/create_type.out | 8 +-- src/test/regress/expected/domain.out | 4 +- src/test/regress/expected/matview.out | 8 +-- src/test/regress/expected/updatable_views.out | 4 +- 6 files changed, 98 insertions(+), 28 deletions(-)
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
Andres Freund <andres@anarazel.de> writes: > On 2019-01-21 18:48:25 +0000, Tom Lane wrote: >> Sort the dependent objects before recursing in findDependentObjects(). > Seems this affects the sepgsql tests: > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2019-01-21%2019%3A45%3A02 Hah ... I'd just looked at that and guessed that *you* broke it with the pluggable-storage changes. But yeah, it looks more like an order-of-messages issue. Will fix. > Yay for having to fix tests blindly :( I can do better than that, got a Fedora installation right over there. regards, tom lane
Hi, On 2019-01-21 15:16:59 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-01-21 18:48:25 +0000, Tom Lane wrote: > >> Sort the dependent objects before recursing in findDependentObjects(). > > > Seems this affects the sepgsql tests: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2019-01-21%2019%3A45%3A02 > > Hah ... I'd just looked at that and guessed that *you* broke it > with the pluggable-storage changes. But yeah, it looks more like > an order-of-messages issue. Will fix. I'm sure I'll break something down the line, but the stuff I pushed so far is pretty darn boring changes... > > Yay for having to fix tests blindly :( > > I can do better than that, got a Fedora installation right > over there. Oh, I just never got around to do the selinux stuff necessary to get sepgsql working... Greetings, Andres Freund
Hi, On 2019-01-21 15:16:59 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-01-21 18:48:25 +0000, Tom Lane wrote: > >> Sort the dependent objects before recursing in findDependentObjects(). > > > Seems this affects the sepgsql tests: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2019-01-21%2019%3A45%3A02 > > Hah ... I'd just looked at that and guessed that *you* broke it > with the pluggable-storage changes. But yeah, it looks more like > an order-of-messages issue. Will fix. I'm sure I'll break something down the line, but the stuff I pushed so far is pretty darn boring changes... > > Yay for having to fix tests blindly :( > > I can do better than that, got a Fedora installation right > over there. Oh, I just never got around to do the selinux stuff necessary to get sepgsql working... Greetings, Andres Freund