Обсуждение: Fixing findDependentObjects()'s dependency on scan order (regressionsin DROP diagnostic messages)
Fixing findDependentObjects()'s dependency on scan order (regressionsin DROP diagnostic messages)
От
Peter Geoghegan
Дата:
I've realized that my patch to make nbtree keys unique by treating heap TID as a tie-breaker attribute must use ASC ordering, for reasons that I won't go into here. Now that I'm not using DESC ordering, there are changes to a small number of DROP...CASCADE messages that leave users with something much less useful than what they'll see today -- see attached patch for full details. Some of these problematic cases involve partitioning: """ create table trigpart (a int, b int) partition by range (a); create table trigpart1 partition of trigpart for values from (0) to (1000); create trigger trg1 after insert on trigpart for each row execute procedure trigger_nothing(); ... drop trigger trg1 on trigpart1; -- fail -ERROR: cannot drop trigger trg1 on table trigpart1 because trigger trg1 on table trigpart requires it -HINT: You can drop trigger trg1 on table trigpart instead. +ERROR: cannot drop trigger trg1 on table trigpart1 because table trigpart1 requires it +HINT: You can drop table trigpart1 instead. """ As you can see, the original hint suggests "you need to drop the object on the partition parent instead of its child", which is useful. The new hint suggests "instead of dropping the trigger on the partition child, maybe drop the child itself!", which is less than useless. This is a problem that needs to be treated as a prerequisite to committing the nbtree patch, so I'd like to get it out of the way soon. The high level issue is that findDependentObjects() relies on the scan order of duplicates within the DependDependerIndexId/pg_depend_depender_index index in a way that nbtree doesn't actually guarantee, and never has guaranteed. As I've shown, findDependentObjects()'s assumptions around where nbtree will leave duplicates accidentally affects the quality of various diagnostic messages. My example also breaks with ignore_system_indexes=on, even on the master branch, so technically this isn't a new problem. I've looked into a way to fix findDependentObjects(). As far as I can tell, I can fix issues by adding a kludgey special case along these lines: 1 diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c 2 index 7dfa3278a5..7454d4e6f8 100644 3 --- a/src/backend/catalog/dependency.c 4 +++ b/src/backend/catalog/dependency.c 5 @@ -605,6 +605,15 @@ findDependentObjects(const ObjectAddress *object, 6 ReleaseDeletionLock(object); 7 return; 8 } 9 + /* 10 + * Assume that another pg_depend entry more suitably 11 + * represents dependency when an entry for a partition 12 + * child's index references a column of the partition 13 + * itself. 14 + */ 15 + if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO && 16 + otherObject.objectSubId != 0) 17 + break; This is obviously brittle, but maybe it hints at a better approach. Notably, it doesn't fix other similar issues, such as this: --- a/contrib/earthdistance/expected/earthdistance.out +++ b/contrib/earthdistance/expected/earthdistance.out @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90), '(0)'::cube) / earth() - 1) < drop extension cube; -- fail, earthdistance requires it ERROR: cannot drop extension cube because other objects depend on it -DETAIL: extension earthdistance depends on extension cube +DETAIL: extension earthdistance depends on function cube_out(cube) Can anyone think of a workable, scalable approach to fixing the processing order of this findDependentObjects() pg_depend scan so that we reliably get the user-visible behavior we already tacitly expect? -- Peter Geoghegan
Вложения
In my opinion, your patch detected three problems:
1. Unsteady order of query results/system messages ('DROP...CASCADE'
detects it).
2. Hide info about a child object dropping ('drop cascades to 62
other objects' detects it).
3. Possible non-informative messages about dependencies ('drop trigger
trg1' detects it)
Problem No. 1 will be amplified with new asynchronous operations,
background workers and distributing query execution. It is not problem
of DBMS. The solution is change the tests: include sorting of query
results, sorting of system messages before diff operation.
If steady order of messages is critical for users we can sort
targetObjects array in the begin of reportDependentObjects() routine by
classId, objectId and objectSubId fields.
Problem No. 2: we can suppress some output optimizations in
object_address_present_add_flags() routine and print all deleted objects.
Problem No. 3: I suppose we can go one of two ways:
a) print all depended objects returned by scan of DependDependerIndexId
relation, not only the first.
b) search a root of dependence and print only it.
On 06.11.2018 5:04, Peter Geoghegan wrote:
> I've realized that my patch to make nbtree keys unique by treating
> heap TID as a tie-breaker attribute must use ASC ordering, for reasons
> that I won't go into here. Now that I'm not using DESC ordering, there
> are changes to a small number of DROP...CASCADE messages that leave
> users with something much less useful than what they'll see today --
> see attached patch for full details. Some of these problematic cases
> involve partitioning:
>
> """
> create table trigpart (a int, b int) partition by range (a);
> create table trigpart1 partition of trigpart for values from (0) to (1000);
> create trigger trg1 after insert on trigpart for each row execute
> procedure trigger_nothing();
> ...
> drop trigger trg1 on trigpart1; -- fail
> -ERROR: cannot drop trigger trg1 on table trigpart1 because trigger
> trg1 on table trigpart requires it
> -HINT: You can drop trigger trg1 on table trigpart instead.
> +ERROR: cannot drop trigger trg1 on table trigpart1 because table
> trigpart1 requires it
> +HINT: You can drop table trigpart1 instead.
> """
>
> As you can see, the original hint suggests "you need to drop the
> object on the partition parent instead of its child", which is useful.
> The new hint suggests "instead of dropping the trigger on the
> partition child, maybe drop the child itself!", which is less than
> useless. This is a problem that needs to be treated as a prerequisite
> to committing the nbtree patch, so I'd like to get it out of the way
> soon.
>
> The high level issue is that findDependentObjects() relies on the scan
> order of duplicates within the
> DependDependerIndexId/pg_depend_depender_index index in a way that
> nbtree doesn't actually guarantee, and never has guaranteed. As I've
> shown, findDependentObjects()'s assumptions around where nbtree will
> leave duplicates accidentally affects the quality of various
> diagnostic messages. My example also breaks with
> ignore_system_indexes=on, even on the master branch, so technically
> this isn't a new problem.
>
> I've looked into a way to fix findDependentObjects(). As far as I can
> tell, I can fix issues by adding a kludgey special case along these
> lines:
>
> 1 diff --git a/src/backend/catalog/dependency.c
> b/src/backend/catalog/dependency.c
> 2 index 7dfa3278a5..7454d4e6f8 100644
> 3 --- a/src/backend/catalog/dependency.c
> 4 +++ b/src/backend/catalog/dependency.c
> 5 @@ -605,6 +605,15 @@ findDependentObjects(const ObjectAddress *object,
> 6 ReleaseDeletionLock(object);
> 7 return;
> 8 }
> 9 + /*
> 10 + * Assume that another pg_depend entry more suitably
> 11 + * represents dependency when an entry for a partition
> 12 + * child's index references a column of the partition
> 13 + * itself.
> 14 + */
> 15 + if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO &&
> 16 + otherObject.objectSubId != 0)
> 17 + break;
>
> This is obviously brittle, but maybe it hints at a better approach.
> Notably, it doesn't fix other similar issues, such as this:
>
> --- a/contrib/earthdistance/expected/earthdistance.out
> +++ b/contrib/earthdistance/expected/earthdistance.out
> @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
> '(0)'::cube) / earth() - 1) <
>
> drop extension cube; -- fail, earthdistance requires it
> ERROR: cannot drop extension cube because other objects depend on it
> -DETAIL: extension earthdistance depends on extension cube
> +DETAIL: extension earthdistance depends on function cube_out(cube)
>
> Can anyone think of a workable, scalable approach to fixing the
> processing order of this findDependentObjects() pg_depend scan so that
> we reliably get the user-visible behavior we already tacitly expect?
>
> --
> Peter Geoghegan
>
--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company
On Mon, Nov 5, 2018 at 7:46 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > Problem No. 1 will be amplified with new asynchronous operations, > background workers and distributing query execution. It is not problem > of DBMS. The solution is change the tests: include sorting of query > results, sorting of system messages before diff operation. > If steady order of messages is critical for users we can sort > targetObjects array in the begin of reportDependentObjects() routine by > classId, objectId and objectSubId fields. Yeah, maybe. I'm not that worried about the order of objects; we can probably continue to get away with suppressing the list of objects by placing a "\set VERBOSITY terse" where needed -- that's something we've already been doing for some time [1]. I accept that it would be better to sort the output, but I'm concerned that that would be a difficult, risky project. What if there was a huge number of dependent objects? What if a memory allocation fails? > Problem No. 2: we can suppress some output optimizations in > object_address_present_add_flags() routine and print all deleted objects. If there is anything that makes it necessary to sort, it's this -- the order of visitation can affect whether or not object_address_present_add_flags() suppresses redundant entries. But I still prefer to fix the problem by changing the scan order to be what we actually want it to be. > Problem No. 3: I suppose we can go one of two ways: > a) print all depended objects returned by scan of DependDependerIndexId > relation, not only the first. > b) search a root of dependence and print only it. A solution does occur to me that I'm kind of embarrassed to suggest, but that would nonetheless probably do the job: The general problem here is that the scan order of a scan that uses pg_depend_depender_index doesn't reliably give us the order we actually want among duplicate index entries (at least, you could choose to characterize it in that narrow way). The index pg_depend_depender_index looks like this: Column │ Type │ Key? │ Definition ──────────┼─────────┼──────┼──────────── classid │ oid │ yes │ classid objid │ oid │ yes │ objid objsubid │ integer │ yes │ objsubid btree, for table "pg_catalog.pg_depend" Note that this isn't a unique index -- there are no unique indexes on pg_depend at all, in fact. (Note also that the same observations apply to pg_shdepend.) The objsubid (not refobjsubid) is 0 in all cases that we have problems with -- we're never going to have a problem with multiple dependent-object-is-column entries that have the same '(classid, objid, objsubid)' value, as far as I can tell: pg@regression[2201]=# SELECT count(*), classid, objid, objsubid FROM pg_depend WHERE objsubid != 0 GROUP BY classid, objid, objsubid HAVING count(*) > 1; count │ classid │ objid │ objsubid ───────┼─────────┼───────┼────────── (0 rows) (i.e. We're only having problems with some of the entries/values that you'll see when the above query is changed to have "bjsubid = 0" in its WHERE clause.) Why not vary the objsubid value among entries that don't use it anyway, so that they have a negative integer objsubid values rather than 0? That would have the effect of forcing the scan order of pg_depend_depender_index scans to be whatever we want it to be. dependent-object-is-column entries would not have their sort order affected anyway, because in practice there is only one entry with the same '(classid, objid, objsubid)' value when objsubid > 0. We could invent some scheme that made pg_depend/pg_shdepend entries use fixed objsubid negative integer codes according to the *referenced* object type (refobjid), and/or the entry's deptype. Places that look at ObjectAddress.objectSubId would look for entries that were > 0 rather than != 0, so we wouldn't really be changing the meaning of objsubid (though it would need to be documented in the user docs, and have a release note compatibility entry). findDependentObjects() would still use "nkeys = 2" for these entries; it would be one of the places that would be taught to check objectSubId > 0 rather than objectSubId != 0. But it would get the scan order it actually needs. I haven't attempted to put this together just yet, because I want to see if it passes the laugh test. My sense is that the best way to fix the problem is to force the scan order to be the one that we accidentally depend on using some mechanism or other. Though not necessarily the one I've sketched out. [1] https://postgr.es/m/24279.1525401104@sss.pgh.pa.us -- Peter Geoghegan
On Tue, Nov 13, 2018 at 1:29 PM Peter Geoghegan <pg@bowt.ie> wrote: > A solution does occur to me that I'm kind of embarrassed to suggest, > but that would nonetheless probably do the job: > Why not vary the objsubid value among entries that don't use it > anyway, so that they have a negative integer objsubid values rather > than 0? That would have the effect of forcing the scan order of > pg_depend_depender_index scans to be whatever we want it to be. I tried to implement this today, and it almost worked. However, what I came up with was even more brittle than I thought it would be, because knowledge of objsubid is all over the place. It's even in pg_dump. Then I had a better idea: add a monotonically decreasing column to pg_depend, and make it the last attribute in both pg_depend_depender_index and pg_depend_reference_index (it has to be both indexes). This makes almost all the regression test output go back to the master branch output, since index-wise duplicates are now accessed in a well defined order: reverse chronological order (the reverse of the order of the creation of a pg_depend entry). There is a tiny amount of stuff that still has a different order in the regression test output, but the changes are trivial, and obviously still correct. Is this approach really so bad? I might want to find a way to make it a monotonically increasing DESC column (there is a question around bki grammar support for that), but that's just a detail. Some scheme involving a new column seems like it will work reasonably well. It's much more maintainable than anything else I'm likely to come up with. The indexes are actually smaller with the change following a run of the regression tests, provided the entire patch series is applied. So even if the only thing you care about is system catalog size, you still win (the table is slightly bigger, though). This approach will fix most or all of the test flappiness that we have been papering over with "\set VERBOSITY terse" before now. I can't promise that just making this one pg_depend change will reliably make that class of problem go away forever, since you could still see something like that elsewhere, but it looks like the vast majority of problem cases involve pg_depend. So we might just end up killing the "\set VERBOSITY terse" hack completely this way. We're already relying on the scan order being in reverse chronological order, so we might as well formalize the dependency. I don't think that it's possible to sort the pg_depend entries as a way of fixing the breakage while avoiding storing this extra information -- what is there to sort on that's there already? You'd have to infer a whole bunch of things about the object types associated with pg_depend entries to do that, and teach dependency.c about its callers. That seems pretty brittle to me. -- Peter Geoghegan
On Wed, Nov 14, 2018 at 1:28 AM Peter Geoghegan <pg@bowt.ie> wrote: > We're already relying on the scan order being in reverse chronological > order, so we might as well formalize the dependency. I don't think > that it's possible to sort the pg_depend entries as a way of fixing > the breakage while avoiding storing this extra information -- what is > there to sort on that's there already? You'd have to infer a whole > bunch of things about the object types associated with pg_depend > entries to do that, and teach dependency.c about its callers. That > seems pretty brittle to me. 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. :-) 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Nov 15, 2018 at 10:52 AM Robert Haas <robertmhaas@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
Вложения
On 14.11.2018 11:28, Peter Geoghegan wrote: > We're already relying on the scan order being in reverse chronological > order, so we might as well formalize the dependency. I don't think > that it's possible to sort the pg_depend entries as a way of fixing > the breakage while avoiding storing this extra information -- what is > there to sort on that's there already? You'd have to infer a whole > bunch of things about the object types associated with pg_depend > entries to do that, and teach dependency.c about its callers. That > seems pretty brittle to me. This solution changes pg_depend relation for solve a problem, which exists only in regression tests. Very rarely it can be in the partitioning cases. Or is it not? I think this decision is some excessive. May be you consider another approach: 1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test tools, not DBMS. And here we can use 'verbose terse'. 2. Print all dependencies in findDependentObjects() on a drop error (see attachment as a prototype). -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
Вложения
On Wed, Dec 5, 2018 at 10:35 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > This solution changes pg_depend relation for solve a problem, which > exists only in regression tests. Very rarely it can be in the > partitioning cases. Or is it not? I don't think it's a matter of how rarely this will happen. We're trying to avoid these diagnostic message changes because they are wrong. I don't think that there is much ambiguity about that. Still, it will happen however often the user drops objects belonging to partition children, which could be quite often. > I think this decision is some excessive. > May be you consider another approach: > 1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test > tools, not DBMS. And here we can use 'verbose terse'. > 2. Print all dependencies in findDependentObjects() on a drop error (see > attachment as a prototype). You didn't include changes to the regression test output, which seems like a big oversight, given that this has a lot to do with diagnostic messages that are represented in the regression tests. Anyway, I don't think it's acceptable to change the messages like this. It makes them much less useful. These stability issue keeps coming up, which makes a comprehensive approach seem attractive to me. At least 95% of the test instability comes from pg_depend. -- Peter Geoghegan
On 06.12.2018 11:52, Peter Geoghegan wrote: > On Wed, Dec 5, 2018 at 10:35 PM Andrey Lepikhov > <a.lepikhov@postgrespro.ru> wrote: >> This solution changes pg_depend relation for solve a problem, which >> exists only in regression tests. Very rarely it can be in the >> partitioning cases. Or is it not? > > I don't think it's a matter of how rarely this will happen. We're > trying to avoid these diagnostic message changes because they are > wrong. I don't think that there is much ambiguity about that. Still, > it will happen however often the user drops objects belonging to > partition children, which could be quite often. I want to say that we need to localize the rules for the order of the diagnostic messages as much as possible in dependency.c. > >> I think this decision is some excessive. >> May be you consider another approach: >> 1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test >> tools, not DBMS. And here we can use 'verbose terse'. >> 2. Print all dependencies in findDependentObjects() on a drop error (see >> attachment as a prototype). > > You didn't include changes to the regression test output, which seems > like a big oversight... It was done knowingly to show the differences in messages that introduces this approach. > messages that are represented in the regression tests. Anyway, I don't > think it's acceptable to change the messages like this. It makes them > much less useful. May you clarify this? If I understand correctly, your solution is that user receives a report about the last inserted internal dependency on the object. Why the full info about internal dependencies of the object much less useful? > > These stability issue keeps coming up, which makes a comprehensive > approach seem attractive to me. At least 95% of the test instability > comes from pg_depend. > During the retail index tuple deletion project (heap cleaner subsystem) we have non-fixed order of tuples at database relations. This caused to unsteady order of text strings in some check-world test results. Thus, I realized that the order of messages in the test results is mostly a game of chance. For this reason I think it is necessary to find more general solution of the messages ordering problem. -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
On Thu, Dec 6, 2018 at 8:52 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > I want to say that we need to localize the rules for the order of the > diagnostic messages as much as possible in dependency.c. But the issue *isn't* confined to dependency.c, anyway. It bleeds into a couple of other modules, like extension.c and tablecmds.c. > > messages that are represented in the regression tests. Anyway, I don't > > think it's acceptable to change the messages like this. It makes them > > much less useful. > > May you clarify this? If I understand correctly, your solution is that > user receives a report about the last inserted internal dependency on > the object. Why the full info about internal dependencies of the object > much less useful? I don't know why for sure -- I just know that it is. It must have evolved that way, as software often does. It's not surprising that the most recently entered dependency is usually the most marginal among dependencies of equal precedence in the real world, and therefore the best suggestion. I've shown this with two examples. To return to one of the examples: are you arguing that there is no practical difference between "you need to drop the object on the partition parent instead of its child" and "instead of dropping the trigger on the partition child, maybe drop the child itself"? I think it's *extremely* obvious that there is a big practical difference to users in that particular case. Now, I agree that there are many other examples where it doesn't really matter to users, but I don't think that that's very relevant. Yes, these are the exceptions, but the exceptions are often the interesting cases. > During the retail index tuple deletion project (heap cleaner subsystem) > we have non-fixed order of tuples at database relations. This caused to > unsteady order of text strings in some check-world test results. > Thus, I realized that the order of messages in the test results is > mostly a game of chance. For this reason I think it is necessary to find > more general solution of the messages ordering problem. I have no idea what you mean here. I'm proposing a patch that stops it being a game of chance, while preserving the existing slightly-random behavior to the greatest extent possible. I think that my patch would have stopped that problem altogether. Are you suggesting that it wouldn't have? -- Peter Geoghegan
On 08.12.2018 6:58, Peter Geoghegan wrote: > I have no idea what you mean here. I'm proposing a patch that stops > it being a game of chance, while preserving the existing > slightly-random behavior to the greatest extent possible. I think that > my patch would have stopped that problem altogether. Are you > suggesting that it wouldn't have? I did many tests of your solution inside the 'quick vacuum' strategy [1] and the background worker called 'heap cleaner' [2]. I must admit that when I use your patch, there is no problem with dependencies. This patch needs opinion of an another reviewer. [1] https://www.postgresql.org/message-id/425db134-8bba-005c-b59d-56e50de3b41e%40postgrespro.ru [2] https://www.postgresql.org/message-id/f49bb262-d246-829d-f835-3950ddac503c%40postgrespro.ru -- Andrey Lepikhov Postgres Professional https://postgrespro.com The Russian Postgres Company
On Mon, Dec 17, 2018 at 10:52 PM Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote: > I did many tests of your solution inside the 'quick vacuum' strategy [1] > and the background worker called 'heap cleaner' [2]. I must admit that > when I use your patch, there is no problem with dependencies. Cool. > This patch needs opinion of an another reviewer. Was I unclear on why the patch fixes the problem? Sorry, but I thought it was obvious. -- Peter Geoghegan
On 2018-Nov-05, Peter Geoghegan wrote: > I've realized that my patch to make nbtree keys unique by treating > heap TID as a tie-breaker attribute must use ASC ordering, for reasons > that I won't go into here. Now that I'm not using DESC ordering, there > are changes to a small number of DROP...CASCADE messages that leave > users with something much less useful than what they'll see today -- > see attached patch for full details. Some of these problematic cases > involve partitioning: Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO entries? I wonder if I just haven't broken the algorithm when introducing that, and I worry that we're adding a complicated kludge to paper over that problem. Maybe instead of the depcreate contortions we need to adjust the algorithm to deal with INTERNAL_AUTO objects in a different way. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Nov-05, Peter Geoghegan wrote:
>> I've realized that my patch to make nbtree keys unique by treating
>> heap TID as a tie-breaker attribute must use ASC ordering, for reasons
>> that I won't go into here. Now that I'm not using DESC ordering, there
>> are changes to a small number of DROP...CASCADE messages that leave
>> users with something much less useful than what they'll see today --
>> see attached patch for full details. Some of these problematic cases
>> involve partitioning:
> Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO
> entries? I wonder if I just haven't broken the algorithm when
> introducing that, and I worry that we're adding a complicated kludge to
> paper over that problem. Maybe instead of the depcreate contortions we
> need to adjust the algorithm to deal with INTERNAL_AUTO objects in a
> different way.
Yeah, I've been wondering about that as well. The original intention
for dependency traversal was that it'd work independently of the ordering
of entries in pg_depend. If it's not doing so, I'd call that a bug in
dependency traversal rather than something the index code needs to be
responsible for.
(Note that this statement doesn't disagree with our issues about needing
to suppress dependency reports in the regression tests; that's because
the order of reports about independent objects can legitimately depend on
the index order. But there shouldn't be any semantic differences.)
regards, tom lane
On Tue, Dec 18, 2018 at 10:07 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO > entries? I wonder if I just haven't broken the algorithm when > introducing that, and I worry that we're adding a complicated kludge to > paper over that problem. Maybe instead of the depcreate contortions we > need to adjust the algorithm to deal with INTERNAL_AUTO objects in a > different way. Well, you also have cases like this: --- a/contrib/earthdistance/expected/earthdistance.out +++ b/contrib/earthdistance/expected/earthdistance.out @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90), '(0)'::cube) / earth() - 1) < drop extension cube; -- fail, earthdistance requires it ERROR: cannot drop extension cube because other objects depend on it -DETAIL: extension earthdistance depends on extension cube +DETAIL: extension earthdistance depends on function cube_out(cube) This is a further example of "wrong, not just annoying". Technically this is a broader problem than DEPENDENCY_INTERNAL_AUTO, I think, though perhaps not too much broader. -- Peter Geoghegan
On Tue, Dec 18, 2018 at 10:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah, I've been wondering about that as well. The original intention > for dependency traversal was that it'd work independently of the ordering > of entries in pg_depend. If it's not doing so, I'd call that a bug in > dependency traversal rather than something the index code needs to be > responsible for. It's clearly not doing so. Is somebody else actually going to fix it, though? I'm quite happy to stand aside to make way for a better solution, but I don't consider myself particularly qualified to come up with that better solution. The findDependentObjects() code is pretty subtle. > (Note that this statement doesn't disagree with our issues about needing > to suppress dependency reports in the regression tests; that's because > the order of reports about independent objects can legitimately depend on > the index order. But there shouldn't be any semantic differences.) The advantage of my admittedly kludgy approach is that it will almost completely eliminate any further need to suppress acceptable regression test differences -- those non-semantic output differences that we're already suppressing semi-regularly. In practice, adding a trailing attribute to each of the two pg_depend indexes almost entirely eliminates the need to play whack-a-mole. That has to be an advantage for the kind of approach that I've suggested. -- Peter Geoghegan
On 2018-Dec-18, Peter Geoghegan wrote:
> Well, you also have cases like this:
>
> --- a/contrib/earthdistance/expected/earthdistance.out
> +++ b/contrib/earthdistance/expected/earthdistance.out
> @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
> '(0)'::cube) / earth() - 1) <
>
> drop extension cube; -- fail, earthdistance requires it
> ERROR: cannot drop extension cube because other objects depend on it
> -DETAIL: extension earthdistance depends on extension cube
> +DETAIL: extension earthdistance depends on function cube_out(cube)
>
> This is a further example of "wrong, not just annoying". Technically
> this is a broader problem than DEPENDENCY_INTERNAL_AUTO, I think,
> though perhaps not too much broader.
Hmm, interesting. I wonder if this is just a case of never testing this
code under "postgres --ignore-system-indexes". I can reproduce the
reported problem without your patch by using that flag. Here's a
recipe:
create extension cube;
create table dep as select ctid as tid,* from pg_depend;
create extension earthdistance;
select tid, deptype, (dep).type, (dep).identity, (ref).type, (ref).identity
from (select tid, deptype, pg_identify_object(classid, objid, objsubid) as dep,
pg_identify_object(refclassid, refobjid, refobjsubid) as ref
from (select ctid as tid, * from pg_depend except select * from dep) a
) b;
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Dec 18, 2018 at 1:20 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2018-Dec-18, Peter Geoghegan wrote: > Hmm, interesting. I wonder if this is just a case of never testing this > code under "postgres --ignore-system-indexes". I suppose that you could say that. The regression tests will fail at many points with --ignore-system-indexes, almost all of which are due to well understood issues. For example, you'll get load of WARNINGs about needing to use a system index despite the server being run under --ignore-system-indexes. > I can reproduce the > reported problem without your patch by using that flag. Here's a > recipe: > > create extension cube; > create table dep as select ctid as tid,* from pg_depend; > create extension earthdistance; > select tid, deptype, (dep).type, (dep).identity, (ref).type, (ref).identity > from (select tid, deptype, pg_identify_object(classid, objid, objsubid) as dep, > pg_identify_object(refclassid, refobjid, refobjsubid) as ref > from (select ctid as tid, * from pg_depend except select * from dep) a > ) b; Interesting. Note that if the standard that we're going to hold a solution to here is "must produce sane output with --ignore-system-indexes", then my solution will not meet that standard. However, I fear that it's going to be really hard to accomplish that goal some other way (besides which, as I said, the tests will still fail with --ignore-system-indexes for reasons that have nothing to do with dependency management). -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 1:20 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I can reproduce the
>> reported problem without your patch by using that flag. Here's a
>> recipe:
> Interesting.
> Note that if the standard that we're going to hold a solution to here
> is "must produce sane output with --ignore-system-indexes", then my
> solution will not meet that standard.
Do you mean "same" output, or "sane" output? I'd certainly expect
the latter.
I think though that Alvaro was just offering this as a way to poke
at the posited bug in dependency.c without having to install your
whole patch.
regards, tom lane
On Tue, Dec 18, 2018 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Interesting. > > Note that if the standard that we're going to hold a solution to here > > is "must produce sane output with --ignore-system-indexes", then my > > solution will not meet that standard. > > Do you mean "same" output, or "sane" output? I'd certainly expect > the latter. I meant sane. --ignore-system-indexes leads to slightly wrong answers in a number of the diagnostic messages run by the regression tests (I recall that the number of objects affected by CASCADE sometimes differed, and I think that there was also a certain amount of this DEPENDENCY_INTERNAL_AUTO business that Alvaro looked into). I think that this must have always been true. My patch will not improve matters with --ignore-system-indexes. It merely makes the currently expected output when scanning pg_depend indexes occur reliably. For better or for worse. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Do you mean "same" output, or "sane" output? I'd certainly expect
>> the latter.
> I meant sane.
> --ignore-system-indexes leads to slightly wrong answers in a number of
> the diagnostic messages run by the regression tests (I recall that the
> number of objects affected by CASCADE sometimes differed, and I think
> that there was also a certain amount of this DEPENDENCY_INTERNAL_AUTO
> business that Alvaro looked into). I think that this must have always
> been true.
Hm, that definitely leads me to feel that we've got bug(s) in
dependency.c. I'll take a look sometime soon.
regards, tom lane
On Tue, Dec 18, 2018 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hm, that definitely leads me to feel that we've got bug(s) in > dependency.c. I'll take a look sometime soon. Thanks! I'm greatly relieved that I probably won't have to become an expert on dependency.c after all. :-) -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm, that definitely leads me to feel that we've got bug(s) in
>> dependency.c. I'll take a look sometime soon.
> Thanks!
> I'm greatly relieved that I probably won't have to become an expert on
> dependency.c after all. :-)
So I poked around for awhile with running the regression tests under
ignore_system_indexes. There seem to be a number of issues involved
here. To a significant extent, they aren't bugs, at least not according
to the original conception of the dependency code: it was not a design
goal that different dependencies of the same object-to-be-deleted would
be processed in a fixed order. That leads to the well-known behavior
that cascade drops report the dropped objects in an unstable order.
Now, perhaps we should make such stability a design goal, as it'd allow
us to get rid of some "suppress the cascade outputs" hacks in the
regression tests. But it's a bit of a new feature. If we wanted to
do that, I'd be inclined to do it by absorbing all the pg_depend entries
for a particular object into an ObjectAddress array and then sorting
them before we process them. The main stumbling block here is "what
would the sort order be?". The best idea I can come up with offhand
is to sort by OID, which at least for regression test purposes would
mean objects would be listed/processed more or less in creation order.
However, there are a couple of places where the ignore_system_indexes
output does something weird like
DROP TABLE PKTABLE;
ERROR: cannot drop table pktable because other objects depend on it
-DETAIL: constraint constrname2 on table fktable depends on table pktable
+DETAIL: constraint constrname2 on table fktable depends on index pktable_pkey
HINT: Use DROP ... CASCADE to drop the dependent objects too.
The reason for this is that the reported "nearest dependency" is the
object that we first reached the named object by recursing from.
If there are multiple dependency paths to the same object then it's
order-traversal-dependent which path we take first. Sorting the
pg_depend entries before processing, as I suggested above, would
remove the instability, but it's not real clear whether we'd get a
desirable choice of reported object that way. Perhaps we could
improve matters by having the early exits from findDependentObjects
(at stack_address_present_add_flags and object_address_present_add_flags)
consider replacing the already-recorded dependee with the current
source object, according to some set of rules. One rule that I think
would be useful is to compare dependency types: I think a normal
dependency is more interesting than an auto dependency which is
more interesting than an internal one. Beyond that, though, I don't
see anything we could do but compare OIDs. (If we do compare OIDs,
then the result should be stable with or without pre-sorting of the
pg_depend entries.)
I also noticed some places where the output reports a different number
of objects dropped by a cascade. This happens when a table column
is reached by some dependency path, while the whole table is reached
by another one. If we find the whole table first, then when we find
the table column we just ignore it. But in the other order, they're
reported as two separate droppable objects. The reasoning is
explained in object_address_present_add_flags:
* We get here if we find a need to delete a whole table after
* having already decided to drop one of its columns. We
* can't report that the whole object is in the array, but we
* should mark the subobject with the whole object's flags.
*
* It might seem attractive to physically delete the column's
* array entry, or at least mark it as no longer needing
* separate deletion. But that could lead to, e.g., dropping
* the column's datatype before we drop the table, which does
* not seem like a good idea. This is a very rare situation
* in practice, so we just take the hit of doing a separate
* DROP COLUMN action even though we know we're gonna delete
* the table later.
I think this reasoning is sound, and we should continue to do the separate
DROP COLUMN step, but it occurs to me that just because we drop the column
separately doesn't mean we have to report it separately to the user.
I propose that we handle this case by adding a new DEPFLAG_IS_SUBOBJECT
flag to the column object's flags, denoting that we know the whole table
will be dropped later. The only effect of this flag is to suppress
reporting of the column object in reportDependentObjects.
Another order-dependent effect that can be seen in the regression tests
comes from the fact that the first loop in findDependentObjects (over
the target's referenced objects) errors out immediately on the first
DEPENDENCY_INTERNAL, DEPENDENCY_INTERNAL_AUTO, or DEPENDENCY_EXTENSION
entry it finds. When this code was written, that was fine because the
only possibility was DEPENDENCY_INTERNAL, and there can be only one
DEPENDENCY_INTERNAL dependency for an object. The addition of
DEPENDENCY_EXTENSION makes that shaky: if you have a couple of
internally-related objects inside an extension, and you try to drop the
dependent one, in theory you might get told either to drop the extension
or to drop the parent object. However, I believe we generally avoid
making DEPENDENCY_EXTENSION entries for internally-dependent objects
(they usually don't have their own owner dependencies either) so I'm not
sure the case arises in practice, or needs to arise in practice. It'd
be reasonable to expect that any given object has at most one INTERNAL+
EXTENSION dependency. (Whether it's reasonable to try to enforce that
is hard to say, though.)
DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
has no hesitation about making multiple entries of that kind. After
rather cursorily looking at that code, I'm leaning to the position
that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
nuked from orbit. In the cases where it's being used, such as
partitioned indexes, I think that probably the right design is one
DEPENDENCY_INTERNAL dependency on the partition master index, and
then one DEPENDENCY_AUTO dependency on the matching partitioned table.
It does not make sense to claim that an object is "part of the
implementation of" more than one thing.
If we can't do that, then ensuring stability of the report would again
require sorting of the referenced objects before we examine them.
I've not attempted to write patches for any of these ideas yet;
I thought I'd throw them out for comments first.
regards, tom lane
I wrote:
> I propose that we handle this case by adding a new DEPFLAG_IS_SUBOBJECT
> flag to the column object's flags, denoting that we know the whole table
> will be dropped later. The only effect of this flag is to suppress
> reporting of the column object in reportDependentObjects.
Here's a proposed patch for that bit. As expected, it seems to eliminate
the variation in number-of-cascaded-drops-reported under
ignore_system_indexes. I do not see any regression outputs change
otherwise.
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 3529084..6b8c735 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** typedef struct
*** 102,107 ****
--- 102,108 ----
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
#define DEPFLAG_EXTENSION 0x0010 /* reached via extension dependency */
#define DEPFLAG_REVERSE 0x0020 /* reverse internal/extension link */
+ #define DEPFLAG_SUBOBJECT 0x0040 /* subobject of another deletable object */
/* expansible list of ObjectAddresses */
*************** reportDependentObjects(const ObjectAddre
*** 886,891 ****
--- 887,896 ----
if (extra->flags & DEPFLAG_ORIGINAL)
continue;
+ /* Also ignore sub-objects; we'll report the whole object elsewhere */
+ if (extra->flags & DEPFLAG_SUBOBJECT)
+ continue;
+
objDesc = getObjectDescription(obj);
/*
*************** object_address_present_add_flags(const O
*** 2320,2332 ****
* DROP COLUMN action even though we know we're gonna delete
* the table later.
*
* Because there could be other subobjects of this object in
* the array, this case means we always have to loop through
* the whole array; we cannot exit early on a match.
*/
ObjectAddressExtra *thisextra = addrs->extras + i;
! thisextra->flags |= flags;
}
}
}
--- 2325,2343 ----
* DROP COLUMN action even though we know we're gonna delete
* the table later.
*
+ * What we can do, though, is mark this as a subobject so that
+ * we don't report it separately, which is confusing because
+ * it's unpredictable whether it happens or not. But do so
+ * only if flags != 0 (flags == 0 is a read-only probe).
+ *
* Because there could be other subobjects of this object in
* the array, this case means we always have to loop through
* the whole array; we cannot exit early on a match.
*/
ObjectAddressExtra *thisextra = addrs->extras + i;
! if (flags)
! thisextra->flags |= (flags | DEPFLAG_SUBOBJECT);
}
}
}
*************** stack_address_present_add_flags(const Ob
*** 2374,2380 ****
* object_address_present_add_flags(), we should propagate
* flags for the whole object to each of its subobjects.
*/
! stackptr->flags |= flags;
}
}
}
--- 2385,2392 ----
* object_address_present_add_flags(), we should propagate
* flags for the whole object to each of its subobjects.
*/
! if (flags)
! stackptr->flags |= (flags | DEPFLAG_SUBOBJECT);
}
}
}
On 2019-Jan-17, Tom Lane wrote: > DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code > has no hesitation about making multiple entries of that kind. After > rather cursorily looking at that code, I'm leaning to the position > that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be > nuked from orbit. In the cases where it's being used, such as > partitioned indexes, I think that probably the right design is one > DEPENDENCY_INTERNAL dependency on the partition master index, and > then one DEPENDENCY_AUTO dependency on the matching partitioned table. As I recall, the problem with that approach is that you can't drop the partition when a partitioned index exists, because it follows the link to the parent index and tries to drop that. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-17, Tom Lane wrote:
>> DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
>> has no hesitation about making multiple entries of that kind. After
>> rather cursorily looking at that code, I'm leaning to the position
>> that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
>> nuked from orbit. In the cases where it's being used, such as
>> partitioned indexes, I think that probably the right design is one
>> DEPENDENCY_INTERNAL dependency on the partition master index, and
>> then one DEPENDENCY_AUTO dependency on the matching partitioned table.
> As I recall, the problem with that approach is that you can't drop the
> partition when a partitioned index exists, because it follows the link
> to the parent index and tries to drop that.
Hm. Still, I can't believe that it's appropriate for a partitioned index
to have exactly the same kind of dependency on the master index as it
does on the associated table.
regards, tom lane
On 2019-Jan-17, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Jan-17, Tom Lane wrote: > >> DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code > >> has no hesitation about making multiple entries of that kind. After > >> rather cursorily looking at that code, I'm leaning to the position > >> that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be > >> nuked from orbit. In the cases where it's being used, such as > >> partitioned indexes, I think that probably the right design is one > >> DEPENDENCY_INTERNAL dependency on the partition master index, and > >> then one DEPENDENCY_AUTO dependency on the matching partitioned table. > > > As I recall, the problem with that approach is that you can't drop the > > partition when a partitioned index exists, because it follows the link > > to the parent index and tries to drop that. > > Hm. Still, I can't believe that it's appropriate for a partitioned index > to have exactly the same kind of dependency on the master index as it > does on the associated table. So there are three necessary features: * The partition can be dropped, which takes down the index partition * The master index can be dropped, which takes down the index partition * The index partition must never be allowed to be dropped on its own. There is a symmetry to these that led me to have the same kind of dependency from the index partition to the other two. I'm now wondering if it would work to have the one from index partition to table partition as DEPENDENCY_AUTO and the one from index partition to parent index as DEPENDENCY_INTERNAL_AUTO. I stared at a lot of possible dependency graphs, but I'm not sure if this one was one of them. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-17, Tom Lane wrote:
>> Hm. Still, I can't believe that it's appropriate for a partitioned index
>> to have exactly the same kind of dependency on the master index as it
>> does on the associated table.
> So there are three necessary features:
> * The partition can be dropped, which takes down the index partition
> * The master index can be dropped, which takes down the index partition
> * The index partition must never be allowed to be dropped on its own.
> There is a symmetry to these that led me to have the same kind of
> dependency from the index partition to the other two.
It's symmetric as long as you suppose that the above are the only
requirements. However, there's another requirement, which is that
if you do try to drop the index partition directly, we would like
the error message to suggest dropping the master index, not the
table. The only way to be sure about what will be suggested is
if there can be only one "owning object".
Also, I am suspicious that this implementation fails on point 3
anyway. It looks to me like if a recursive drop reaches the
index partition from a dependency other than either the table
partition or the master index, it will let the index partition
be dropped by itself. Ordinarily, this would likely not matter
because the index partition would share any other dependencies
(opclasses, for instance) with one or the other owning object.
But I don't think it's too hard to construct cases where that's
not true. If nothing else, it looks like ALTER OBJECT DEPENDS ON
EXTENSION can be used to attach a random dependency to just
about anything.
regards, tom lane
On Thu, Jan 17, 2019 at 3:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > There is a symmetry to these that led me to have the same kind of > > dependency from the index partition to the other two. > > It's symmetric as long as you suppose that the above are the only > requirements. However, there's another requirement, which is that > if you do try to drop the index partition directly, we would like > the error message to suggest dropping the master index, not the > table. The only way to be sure about what will be suggested is > if there can be only one "owning object". +1. This is certainly a necessary requirement. Absurd error messages are not okay. -- Peter Geoghegan
I wrote:
> Also, I am suspicious that this implementation fails on point 3
> anyway ... If nothing else, it looks like ALTER OBJECT DEPENDS ON
> EXTENSION can be used to attach a random dependency to just
> about anything.
Yup:
regression=# drop table if exists idxpart cascade;
DROP TABLE
regression=# create table idxpart (a int) partition by range (a);
CREATE TABLE
regression=# create index on idxpart (a);
CREATE INDEX
regression=# create table idxpart1 partition of idxpart for values from (0) to (10);
CREATE TABLE
regression=# drop index idxpart1_a_idx; -- no way
ERROR: cannot drop index idxpart1_a_idx because index idxpart_a_idx requires it
HINT: You can drop index idxpart_a_idx instead.
regression=# \d idxpart1
Table "public.idxpart1"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Partition of: idxpart FOR VALUES FROM (0) TO (10)
Indexes:
"idxpart1_a_idx" btree (a)
regression=# create extension cube;
CREATE EXTENSION
regression=# alter index idxpart1_a_idx depends on extension cube;
ALTER INDEX
regression=# drop extension cube;
DROP EXTENSION
regression=# \d idxpart1
Table "public.idxpart1"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Partition of: idxpart FOR VALUES FROM (0) TO (10)
regards, tom lane
On Thu, Jan 17, 2019 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > So I poked around for awhile with running the regression tests under > ignore_system_indexes. There seem to be a number of issues involved > here. To a significant extent, they aren't bugs, at least not according > to the original conception of the dependency code: it was not a design > goal that different dependencies of the same object-to-be-deleted would > be processed in a fixed order. I agree that it's the exceptional cases that are of concern here. The vast majority of the changes you'll see with "ignore_system_indexes=on" are noise. > Now, perhaps we should make such stability a design goal, as it'd allow > us to get rid of some "suppress the cascade outputs" hacks in the > regression tests. But it's a bit of a new feature. If we wanted to > do that, I'd be inclined to do it by absorbing all the pg_depend entries > for a particular object into an ObjectAddress array and then sorting > them before we process them. The main stumbling block here is "what > would the sort order be?". The best idea I can come up with offhand > is to sort by OID, which at least for regression test purposes would > mean objects would be listed/processed more or less in creation order. I think that we might as well have a stable order. Maybe an explicit sort step is unnecessary -- we can actually rely on scan order, while accepting you'll get a different order with "ignore_system_indexes=on" (though without getting substantively different/incorrect messages). I'm slightly concerned that an explicit sort step might present difficulties in extreme cases. How much memory are we prepared to allocate, just to get a stable order? It probably won't really matter what the specific order is, once the current problems (the DEPENDENCY_INTERNAL_AUTO issue and the issue you'll fix with DEPFLAG_IS_SUBOBJECT) are handled in a direct manner. As I've pointed out a couple of times already, we can add a 4 byte tie-breaker column to both pg_depend indexes without increasing the size of the on-disk representation, since the extra space is already lost to alignment (we could even add a new 4 byte column to the table without any storage overhead, if that happened to make sense). What is the likelihood that somebody will ever find a better use for this alignment padding? These two indexes are typically the largest system catalog indexes by far, so the opportunity cost matters. I don't think that the direct cost (more cycles) is worth worrying about, though. Nobody has added a pg_depend column since it was first introduced back in 2002. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, Jan 17, 2019 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now, perhaps we should make such stability a design goal, as it'd allow
>> us to get rid of some "suppress the cascade outputs" hacks in the
>> regression tests. But it's a bit of a new feature. If we wanted to
>> do that, I'd be inclined to do it by absorbing all the pg_depend entries
>> for a particular object into an ObjectAddress array and then sorting
>> them before we process them. The main stumbling block here is "what
>> would the sort order be?". The best idea I can come up with offhand
>> is to sort by OID, which at least for regression test purposes would
>> mean objects would be listed/processed more or less in creation order.
> I think that we might as well have a stable order. Maybe an explicit
> sort step is unnecessary -- we can actually rely on scan order, while
> accepting you'll get a different order with "ignore_system_indexes=on"
> (though without getting substantively different/incorrect messages).
Yeah, that's the policy we've followed so far, but I remain concerned
about its effects on the regression tests. There are a lot of places
where we print full DROP CASCADE output because "it hasn't failed yet".
I fear every one of those is a gotcha that's waiting to bite us.
Also, is index scan order really guaranteed for equal-keyed items?
It isn't today, and I didn't think your patches were going to make
it so.
> I'm slightly concerned that an explicit sort step might present
> difficulties in extreme cases. How much memory are we prepared to
> allocate, just to get a stable order?
We're going to stick all these items into an ObjectAddress array anyway,
so at worst it'd be 2X growth, most likely a lot less since we'd only
be sorting one level of dependency at a time.
> As I've pointed out a couple of times already, we can add a 4 byte
> tie-breaker column to both pg_depend indexes without increasing the
> size of the on-disk representation, since the extra space is already
> lost to alignment (we could even add a new 4 byte column to the table
> without any storage overhead, if that happened to make sense).
Meh ... where do you get the 4-byte value from?
regards, tom lane
On Thu, Jan 17, 2019 at 4:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah, that's the policy we've followed so far, but I remain concerned > about its effects on the regression tests. There are a lot of places > where we print full DROP CASCADE output because "it hasn't failed yet". > I fear every one of those is a gotcha that's waiting to bite us. Right. I don't want to have to play whack-a-mole with this later on, and risk suppressing the wrong thing. > Also, is index scan order really guaranteed for equal-keyed items? > It isn't today, and I didn't think your patches were going to make > it so. The idea is that you'd have an extra column, so they wouldn't be equal-keyed (the keys that the scan was interested in would be duplicated, but we could still rely on the ordering). Are you suggesting that there'd be a stability issue regardless? My patch does guarantee order for equal-keyed items, since heap TID is treated as just another attribute, at least as far as the nbtree key space is concerned. The big unknown is how exactly that works out when the regression tests are run in parallel, on a busy system, since heap TID isn't just another column outside of nbtree. I think that this will probably cause test flappiness if we don't nail it down now. That was certainly my experience before I nailed it down in my own tentative way. My sense is that without addressing this issue, we're going to be sensitive to concurrent heap TID recycling by VACUUM in a way that might become an annoyance. I see no reason to not go fix it once and for all, since the vast majority of all problems are around the two pg_depend indexes. > We're going to stick all these items into an ObjectAddress array anyway, > so at worst it'd be 2X growth, most likely a lot less since we'd only > be sorting one level of dependency at a time. It sounds like we don't have a good reason to not just sort them explicitly, then. I'm happy to go that way. I mostly just wanted to be sure that you were aware that we could add a tie-breaker column without any storage overhead. > > As I've pointed out a couple of times already, we can add a 4 byte > > tie-breaker column to both pg_depend indexes without increasing the > > size of the on-disk representation, since the extra space is already > > lost to alignment (we could even add a new 4 byte column to the table > > without any storage overhead, if that happened to make sense). > > Meh ... where do you get the 4-byte value from? In the kludgey patch that I posted, the 4-byte value is manufactured artificially within a backend in descending order. That may have a slight advantage over object oid, even after the pg_depend correctness issues are addressed. A fixed order within a backend or originating transaction seems like it might avoid a few remaining instability issues. Not sure. I seem to recall there being some disagreement between you and Andres on this very point (is object oid a perfectly stable tie-breaker in practice?) on a similar thread from 2017. -- Peter Geoghegan
On Thu, Jan 17, 2019 at 5:09 PM Peter Geoghegan <pg@bowt.ie> wrote: > In the kludgey patch that I posted, the 4-byte value is manufactured > artificially within a backend in descending order. That may have a > slight advantage over object oid, even after the pg_depend correctness > issues are addressed. A fixed order within a backend or originating > transaction seems like it might avoid a few remaining instability > issues. Not sure. I seem to recall there being some disagreement > between you and Andres on this very point (is object oid a perfectly > stable tie-breaker in practice?) on a similar thread from 2017. Here are your remarks about it on that 2017 thread: https://www.postgresql.org/message-id/11852.1501610262%40sss.pgh.pa.us -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, Jan 17, 2019 at 4:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> We're going to stick all these items into an ObjectAddress array anyway,
>> so at worst it'd be 2X growth, most likely a lot less since we'd only
>> be sorting one level of dependency at a time.
> It sounds like we don't have a good reason to not just sort them
> explicitly, then. I'm happy to go that way. I mostly just wanted to be
> sure that you were aware that we could add a tie-breaker column
> without any storage overhead.
I think the tiebreaker idea is just a hack, because it'd only be stable
to the extent that the added tiebreaker values are stable, and they
wouldn't be very much so if the counter state is only kept per-backend.
Attached is a draft patch to sort objects before the recursion step
in findDependentObjects. I found that sorting by descending OID is
really the right thing; if we sort by increasing OID then we get a
whole lot more diffs in the DROP CASCADE output. As shown, there
are just a few such diffs, and many of them seem to be for the better
anyway.
I repurposed object_address_comparator for this, which means this
has a side-effect of changing the order in which pg_depend entries
for a new object are inserted into that catalog. I don't think
this is an issue.
I did not do anything here about reverting the various hacks to
suppress DROP CASCADE printouts in specific regression tests.
I'm not very sure whether doing so would be useful or not.
Testing this under ignore_system_indexes, it fixes most of the
cases where the output changes, but there are still two categories
it doesn't fix:
* Objects-to-drop output from DROP ROLE is still unstable. I suppose
this would be fixed by also doing sorting in that code path, but
I've not looked into it.
* There is still instability in which object you get told to drop
when attempting to drop an index partition or trigger, as a consequence
of there being two possible DEPENDENCY_INTERNAL_AUTO targets. I still
feel that the right fix there involves changing the design for what
dependency types we store, but I've not worked on it yet.
Comments?
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 6b8c735..aae30b5 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** typedef struct ObjectAddressStack
*** 124,129 ****
--- 124,136 ----
struct ObjectAddressStack *next; /* next outer stack level */
} ObjectAddressStack;
+ /* temporary storage in findDependentObjects */
+ typedef struct
+ {
+ ObjectAddress obj; /* object to be deleted --- MUST BE FIRST */
+ int subflags; /* flags to pass down when recursing to obj */
+ } ObjectAddressAndFlags;
+
/* for find_expr_references_walker */
typedef struct
{
*************** findDependentObjects(const ObjectAddress
*** 472,477 ****
--- 479,487 ----
SysScanDesc scan;
HeapTuple tup;
ObjectAddress otherObject;
+ ObjectAddressAndFlags *dependentObjects;
+ int numDependentObjects;
+ int maxDependentObjects;
ObjectAddressStack mystack;
ObjectAddressExtra extra;
*************** findDependentObjects(const ObjectAddress
*** 704,715 ****
systable_endscan(scan);
/*
! * Now recurse to any dependent objects. We must visit them first since
! * they have to be deleted before the current object.
*/
! mystack.object = object; /* set up a new stack level */
! mystack.flags = objflags;
! mystack.next = stack;
ScanKeyInit(&key[0],
Anum_pg_depend_refclassid,
--- 714,727 ----
systable_endscan(scan);
/*
! * Next, identify all objects dependent on the current object. To ensure
! * predictable deletion order, we collect them up in dependentObjects and
! * sort the list before actually recursing.
*/
! maxDependentObjects = 128; /* arbitrary initial allocation */
! dependentObjects = (ObjectAddressAndFlags *)
! palloc(maxDependentObjects * sizeof(ObjectAddressAndFlags));
! numDependentObjects = 0;
ScanKeyInit(&key[0],
Anum_pg_depend_refclassid,
*************** findDependentObjects(const ObjectAddress
*** 762,768 ****
continue;
}
! /* Recurse, passing objflags indicating the dependency type */
switch (foundDep->deptype)
{
case DEPENDENCY_NORMAL:
--- 774,783 ----
continue;
}
! /*
! * We do need to delete it, so identify objflags to be passed down,
! * which depend on the dependency type.
! */
switch (foundDep->deptype)
{
case DEPENDENCY_NORMAL:
*************** findDependentObjects(const ObjectAddress
*** 798,805 ****
break;
}
! findDependentObjects(&otherObject,
! subflags,
flags,
&mystack,
targetObjects,
--- 813,859 ----
break;
}
! /* And add it to the pending-objects list */
! if (numDependentObjects >= maxDependentObjects)
! {
! /* enlarge array if needed */
! maxDependentObjects *= 2;
! dependentObjects = (ObjectAddressAndFlags *)
! repalloc(dependentObjects,
! maxDependentObjects * sizeof(ObjectAddressAndFlags));
! }
!
! dependentObjects[numDependentObjects].obj = otherObject;
! dependentObjects[numDependentObjects].subflags = subflags;
! numDependentObjects++;
! }
!
! systable_endscan(scan);
!
! /*
! * Now we can sort the dependent objects into a stable visitation order.
! * It's safe to use object_address_comparator here since the obj field is
! * first within ObjectAddressAndFlags.
! */
! if (numDependentObjects > 1)
! qsort((void *) dependentObjects, numDependentObjects,
! sizeof(ObjectAddressAndFlags),
! object_address_comparator);
!
! /*
! * Now recurse to any dependent objects. We must visit them first since
! * they have to be deleted before the current object.
! */
! mystack.object = object; /* set up a new stack level */
! mystack.flags = objflags;
! mystack.next = stack;
!
! for (int i = 0; i < numDependentObjects; i++)
! {
! ObjectAddressAndFlags *depObj = dependentObjects + i;
!
! findDependentObjects(&depObj->obj,
! depObj->subflags,
flags,
&mystack,
targetObjects,
*************** findDependentObjects(const ObjectAddress
*** 807,813 ****
depRel);
}
! systable_endscan(scan);
/*
* Finally, we can add the target object to targetObjects. Be careful to
--- 861,867 ----
depRel);
}
! pfree(dependentObjects);
/*
* Finally, we can add the target object to targetObjects. Be careful to
*************** object_address_comparator(const void *a,
*** 2109,2126 ****
const ObjectAddress *obja = (const ObjectAddress *) a;
const ObjectAddress *objb = (const ObjectAddress *) b;
! if (obja->classId < objb->classId)
return -1;
- if (obja->classId > objb->classId)
- return 1;
if (obja->objectId < objb->objectId)
return -1;
! if (obja->objectId > objb->objectId)
return 1;
/*
! * We sort the subId as an unsigned int so that 0 will come first. See
! * logic in eliminate_duplicate_dependencies.
*/
if ((unsigned int) obja->objectSubId < (unsigned int) objb->objectSubId)
return -1;
--- 2163,2193 ----
const ObjectAddress *obja = (const ObjectAddress *) a;
const ObjectAddress *objb = (const ObjectAddress *) b;
! /*
! * Primary sort key is OID descending. Most of the time, this will result
! * in putting newer objects before older ones, which is likely to be the
! * right order to delete in.
! */
! if (obja->objectId > objb->objectId)
return -1;
if (obja->objectId < objb->objectId)
+ return 1;
+
+ /*
+ * Next sort on catalog ID, in case identical OIDs appear in different
+ * catalogs. Sort direction is pretty arbitrary here.
+ */
+ if (obja->classId < objb->classId)
return -1;
! if (obja->classId > objb->classId)
return 1;
/*
! * Last, sort on object subId.
! *
! * We sort the subId as an unsigned int so that 0 (the whole object) will
! * come first. This is essential for eliminate_duplicate_dependencies,
! * and is also the best order for findDependentObjects.
*/
if ((unsigned int) obja->objectSubId < (unsigned int) objb->objectSubId)
return -1;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 27cf550..7bb8ca9 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
*************** DETAIL: drop cascades to table alter2.t
*** 2583,2592 ****
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 conversion alter2.ascii_to_utf8
drop cascades to text search parser alter2.prs
drop cascades to text search configuration alter2.cfg
--- 2583,2592 ----
drop cascades to view alter2.v1
drop cascades to function alter2.plus1(integer)
drop cascades to type alter2.posint
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/create_type.out b/src/test/regress/expected/create_type.out
index 2f7d5f9..8309756 100644
*** a/src/test/regress/expected/create_type.out
--- b/src/test/regress/expected/create_type.out
*************** DROP FUNCTION base_fn_out(opaque); -- er
*** 161,173 ****
ERROR: function base_fn_out(opaque) does not exist
DROP TYPE base_type; -- error
ERROR: cannot drop type base_type because other objects depend on it
! DETAIL: function base_fn_out(base_type) depends on type base_type
! function base_fn_in(cstring) depends on type base_type
HINT: Use DROP ... CASCADE to drop the dependent objects too.
DROP TYPE base_type CASCADE;
NOTICE: drop cascades to 2 other objects
! DETAIL: drop cascades to function base_fn_out(base_type)
! drop cascades to function base_fn_in(cstring)
-- Check usage of typmod with a user-defined type
-- (we have borrowed numeric's typmod functions)
CREATE TEMP TABLE mytab (foo widget(42,13,7)); -- should fail
--- 161,173 ----
ERROR: function base_fn_out(opaque) does not exist
DROP TYPE base_type; -- error
ERROR: cannot drop type base_type because other objects depend on it
! DETAIL: function base_fn_in(cstring) depends on type base_type
! function base_fn_out(base_type) depends on type base_type
HINT: Use DROP ... CASCADE to drop the dependent objects too.
DROP TYPE base_type CASCADE;
NOTICE: drop cascades to 2 other objects
! DETAIL: drop cascades to function base_fn_in(cstring)
! drop cascades to function base_fn_out(base_type)
-- Check usage of typmod with a user-defined type
-- (we have borrowed numeric's typmod functions)
CREATE TEMP TABLE mytab (foo widget(42,13,7)); -- should fail
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index 11bc772..4ff1b4a 100644
*** a/src/test/regress/expected/domain.out
--- b/src/test/regress/expected/domain.out
*************** alter domain dnotnulltest drop not null;
*** 645,652 ****
update domnotnull set col1 = null;
drop domain dnotnulltest cascade;
NOTICE: drop cascades to 2 other objects
! DETAIL: drop cascades to column col1 of table domnotnull
! drop cascades to column col2 of table domnotnull
-- Test ALTER DOMAIN .. DEFAULT ..
create table domdeftest (col1 ddef1);
insert into domdeftest default values;
--- 645,652 ----
update domnotnull set col1 = null;
drop domain dnotnulltest cascade;
NOTICE: drop cascades to 2 other objects
! DETAIL: drop cascades to column col2 of table domnotnull
! drop cascades to column col1 of table domnotnull
-- Test ALTER DOMAIN .. DEFAULT ..
create table domdeftest (col1 ddef1);
insert into domdeftest default values;
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 08cd4be..d0121a7 100644
*** a/src/test/regress/expected/matview.out
--- b/src/test/regress/expected/matview.out
*************** SELECT type, m.totamt AS mtot, v.totamt
*** 311,322 ****
DROP TABLE mvtest_t;
ERROR: cannot drop table mvtest_t because other objects depend on it
DETAIL: view mvtest_tv depends on table mvtest_t
view mvtest_tvv depends on view mvtest_tv
materialized view mvtest_tvvm depends on view mvtest_tvv
view mvtest_tvvmv depends on materialized view mvtest_tvvm
materialized view mvtest_bb depends on view mvtest_tvvmv
- materialized view mvtest_mvschema.mvtest_tvm depends on view mvtest_tv
- materialized view mvtest_tvmm depends on materialized view mvtest_mvschema.mvtest_tvm
materialized view mvtest_tm depends on table mvtest_t
materialized view mvtest_tmm depends on materialized view mvtest_tm
HINT: Use DROP ... CASCADE to drop the dependent objects too.
--- 311,322 ----
DROP TABLE mvtest_t;
ERROR: cannot drop table mvtest_t because other objects depend on it
DETAIL: view mvtest_tv depends on table mvtest_t
+ materialized view mvtest_mvschema.mvtest_tvm depends on view mvtest_tv
+ materialized view mvtest_tvmm depends on materialized view mvtest_mvschema.mvtest_tvm
view mvtest_tvv depends on view mvtest_tv
materialized view mvtest_tvvm depends on view mvtest_tvv
view mvtest_tvvmv depends on materialized view mvtest_tvvm
materialized view mvtest_bb depends on view mvtest_tvvmv
materialized view mvtest_tm depends on table mvtest_t
materialized view mvtest_tmm depends on materialized view mvtest_tm
HINT: Use DROP ... CASCADE to drop the dependent objects too.
*************** BEGIN;
*** 327,338 ****
DROP TABLE mvtest_t CASCADE;
NOTICE: drop cascades to 9 other objects
DETAIL: drop cascades to view mvtest_tv
drop cascades to view mvtest_tvv
drop cascades to materialized view mvtest_tvvm
drop cascades to view mvtest_tvvmv
drop cascades to materialized view mvtest_bb
- drop cascades to materialized view mvtest_mvschema.mvtest_tvm
- drop cascades to materialized view mvtest_tvmm
drop cascades to materialized view mvtest_tm
drop cascades to materialized view mvtest_tmm
ROLLBACK;
--- 327,338 ----
DROP TABLE mvtest_t CASCADE;
NOTICE: drop cascades to 9 other objects
DETAIL: drop cascades to view mvtest_tv
+ drop cascades to materialized view mvtest_mvschema.mvtest_tvm
+ drop cascades to materialized view mvtest_tvmm
drop cascades to view mvtest_tvv
drop cascades to materialized view mvtest_tvvm
drop cascades to view mvtest_tvvmv
drop cascades to materialized view mvtest_bb
drop cascades to materialized view mvtest_tm
drop cascades to materialized view mvtest_tmm
ROLLBACK;
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 0ac08ca..420c5a5 100644
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
*************** DETAIL: drop cascades to view ro_view1
*** 334,339 ****
--- 334,340 ----
drop cascades to view ro_view17
drop cascades to view ro_view2
drop cascades to view ro_view3
+ drop cascades to view ro_view4
drop cascades to view ro_view5
drop cascades to view ro_view6
drop cascades to view ro_view7
*************** drop cascades to view ro_view8
*** 341,351 ****
drop cascades to view ro_view9
drop cascades to view ro_view11
drop cascades to view ro_view13
drop cascades to view rw_view15
drop cascades to view rw_view16
drop cascades to view ro_view20
- drop cascades to view ro_view4
- drop cascades to view rw_view14
DROP VIEW ro_view10, ro_view12, ro_view18;
DROP SEQUENCE uv_seq CASCADE;
NOTICE: drop cascades to view ro_view19
--- 342,351 ----
drop cascades to view ro_view9
drop cascades to view ro_view11
drop cascades to view ro_view13
+ drop cascades to view rw_view14
drop cascades to view rw_view15
drop cascades to view rw_view16
drop cascades to view ro_view20
DROP VIEW ro_view10, ro_view12, ro_view18;
DROP SEQUENCE uv_seq CASCADE;
NOTICE: drop cascades to view ro_view19
On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think the tiebreaker idea is just a hack, because it'd only be stable > to the extent that the added tiebreaker values are stable, and they > wouldn't be very much so if the counter state is only kept per-backend. > > Attached is a draft patch to sort objects before the recursion step > in findDependentObjects. I found that sorting by descending OID is > really the right thing; if we sort by increasing OID then we get a > whole lot more diffs in the DROP CASCADE output. As shown, there > are just a few such diffs, and many of them seem to be for the better > anyway. This reminds me of the output that I saw back when my patch used DESC heap TID order. I agree that those regression test changes are improvements. I think that they're caused by the existing nbtree code's preference for storing duplicates on the first leaf page it could go on that is found to be empty. It stores duplicates in approximately descending order, but now you're artificially forcing perfectly descending order -- one or two things that were not in the expected (reverse) chronological order, by accident, now appear in order. (I've not verified this explanation, though.) > I did not do anything here about reverting the various hacks to > suppress DROP CASCADE printouts in specific regression tests. > I'm not very sure whether doing so would be useful or not. I doubt it myself. We can always change it later on. > Testing this under ignore_system_indexes, it fixes most of the > cases where the output changes, but there are still two categories > it doesn't fix: > > * Objects-to-drop output from DROP ROLE is still unstable. I suppose > this would be fixed by also doing sorting in that code path, but > I've not looked into it. The nbtree patch is not dependent on doing better here, since the order here is merely unstable, without leading to incorrect diagnostic messages. I can just change the regress output mechanically. That said, I cannot be sure that the instability won't become more of an annoyance with heap TID being treated as a tie-breaker attribute within nbtree. It's probably fine to reserve the option to do better here, and do so if and when it becomes clear that it matters. I defer to you. > * There is still instability in which object you get told to drop > when attempting to drop an index partition or trigger, as a consequence > of there being two possible DEPENDENCY_INTERNAL_AUTO targets. I still > feel that the right fix there involves changing the design for what > dependency types we store, but I've not worked on it yet. I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the case for fixing that directly inarguable. I'm slightly surprised that you're not fully convinced of this already. Have I missed some subtlety? Thanks -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached is a draft patch to sort objects before the recursion step
>> in findDependentObjects. I found that sorting by descending OID is
>> really the right thing; if we sort by increasing OID then we get a
>> whole lot more diffs in the DROP CASCADE output. As shown, there
>> are just a few such diffs, and many of them seem to be for the better
>> anyway.
> This reminds me of the output that I saw back when my patch used DESC
> heap TID order. I agree that those regression test changes are
> improvements. I think that they're caused by the existing nbtree
> code's preference for storing duplicates on the first leaf page it
> could go on that is found to be empty.
Yeah, I figured the explanation for the weirder changes was somewhere
around there. Like you, I haven't bothered to verify it.
>> * There is still instability in which object you get told to drop
>> when attempting to drop an index partition or trigger, as a consequence
>> of there being two possible DEPENDENCY_INTERNAL_AUTO targets. I still
>> feel that the right fix there involves changing the design for what
>> dependency types we store, but I've not worked on it yet.
> I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the
> case for fixing that directly inarguable. I'm slightly surprised that
> you're not fully convinced of this already. Have I missed some
> subtlety?
It's clear that we must change *something* in that area. I'm not yet
wedded to a particular fix, just expressing a guess as to what might
be the cleanest fix.
Also, we evidently need something we can back-patch into v11, which might
end up being very far from clean :-(. I have no opinions yet on what
would make sense in that branch.
regards, tom lane
On Fri, Jan 18, 2019 at 4:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the > > case for fixing that directly inarguable. I'm slightly surprised that > > you're not fully convinced of this already. Have I missed some > > subtlety? > > It's clear that we must change *something* in that area. I'm not yet > wedded to a particular fix, just expressing a guess as to what might > be the cleanest fix. I'm surprised that you're not "wedded" to that fix in some sense, though. Your analysis about the right design having one DEPENDENCY_INTERNAL dependency on the partition master index, and one DEPENDENCY_AUTO dependency on the matching partitioned table seemed spot on to me. > Also, we evidently need something we can back-patch into v11, which might > end up being very far from clean :-(. I have no opinions yet on what > would make sense in that branch. Me neither, but, as I said, I think that you've identified the right design for the master branch. And, I tend to doubt that you'll find something that works for the backbranches that is also worth using in the master branch. Why does it seem necessary to fix the bug in the backbranches? I agree that it's broken, but it's not obvious to me that it'll cause serious problems in the real world that outweigh the potential downsides. Perhaps I've missed some obvious downside. -- Peter Geoghegan
On 2019-Jan-18, Peter Geoghegan wrote: > On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > * There is still instability in which object you get told to drop > > when attempting to drop an index partition or trigger, as a consequence > > of there being two possible DEPENDENCY_INTERNAL_AUTO targets. I still > > feel that the right fix there involves changing the design for what > > dependency types we store, but I've not worked on it yet. > > I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the > case for fixing that directly inarguable. I'm slightly surprised that > you're not fully convinced of this already. Have I missed some > subtlety? I agree that it needs fixed, but I don't think we know what to change it *to*. The suggestion to use one AUTO and one INTERNAL seems to me to break some use cases. Maybe one INTERNAL and one INTERNAL_AUTO works well, not sure. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 18, 2019 at 4:06 PM Peter Geoghegan <pg@bowt.ie> wrote: > > * Objects-to-drop output from DROP ROLE is still unstable. I suppose > > this would be fixed by also doing sorting in that code path, but > > I've not looked into it. > > The nbtree patch is not dependent on doing better here, since the > order here is merely unstable, without leading to incorrect diagnostic > messages. I can just change the regress output mechanically. That > said, I cannot be sure that the instability won't become more of an > annoyance with heap TID being treated as a tie-breaker attribute > within nbtree. It's probably fine to reserve the option to do better > here, and do so if and when it becomes clear that it matters. I defer > to you. Good news: Testing my patch (with the hacky pg_depend tie-breaker commit removed) on top of your recent commit f1ad067f confirms the only "real" remaining problem is with two tests that relate to partitioning (the multiple DEPENDENCY_INTERNAL_AUTO entry issue). It seems likely that objects-to-drop output from DROP ROLE can remain "unstable" without bothering anybody -- I've run "make -Otarget -j10 -s check-world" with the same source tree multiple times, and have yet to see a test failure. In practice *all* of the instability that's of practical concern (that could cause buildfarm failures) related to the two pg_depend indexes. It very much looks that way, at least -- only the buildfarm can confirm this. I attach a patch that shows how I'll adjust the harmless objects-to-drop output from DROP ROLE to make relevant tests pass -- the scope of changes is very limited, and the changes are harmless/mechanical. Separately, I attach the patch that I'd have to use to paper over the partitioning/DEPENDENCY_INTERNAL_AUTO issue (a patch that makes the regression tests actively ignore visible manifestations of the DEPENDENCY_INTERNAL_AUTO bug). As I said, this confirms that there are only two "real" remaining tests that fail. Thanks -- Peter Geoghegan
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-18, Peter Geoghegan wrote:
>> I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the
>> case for fixing that directly inarguable. I'm slightly surprised that
>> you're not fully convinced of this already. Have I missed some
>> subtlety?
> I agree that it needs fixed, but I don't think we know what to change it
> *to*. The suggestion to use one AUTO and one INTERNAL seems to me to
> break some use cases. Maybe one INTERNAL and one INTERNAL_AUTO works
> well, not sure.
I've been chewing on this problem off-list, and frankly it's a mess.
The closest I could get to solving it along the original lines
went like this:
* In addition, we support INTERNAL_AUTO dependencies, which alter the
* rules for this. If the target has both INTERNAL and INTERNAL_AUTO
* dependencies, then it can be dropped if any one of those objects is
* dropped, but not unless at least one of them is. In this situation we
* mustn't automatically transform a random deletion request into a
* parent-object deletion. Instead, we proceed with deletion if we are
* recursing from any owning object, and otherwise set the object aside to
* recheck at the end. If we don't later find a reason to delete one of
* the owning objects, we'll throw an error.
I think that's ugly as sin: INTERNAL means something different if there's
another dependency beside it? It also turns out to have some
implementation problems we need not get into here, but which would create
a performance penalty for deletions.
A theoretically purer answer is to split INTERNAL_AUTO into two new
dependency types, one linking to the real "owning" object (the parent
index or trigger) and the other to the secondary owner (the partition
table), while leaving regular INTERNAL as it stands. I don't especially
want to go that way, though: it seems overcomplicated from the standpoint
of outside inspections of pg_depend, and it'd be very risky to back-patch
into v11. (For instance, if someone went backwards on their postmaster
minor release, the older v11 version would not know what to do with the
new dependency type.)
It strikes me however that we can stick with the existing catalog contents
by making this definition: of the INTERNAL_AUTO dependencies of a given
object, the "true owner" to be reported in errors is the dependency
that is of the same classId as that object. If this doesn't uniquely
identify one dependency, the report will mention a random one. This is
ugly as well, but it's certainly a lot more practical to cram into v11,
and it seems like it would cover the current and likely future use-cases
for partition-related objects. When and if we find a use-case for which
this doesn't work, we can think about extending the catalog representation
to identify a unique owner in a less ad-hoc way.
With either of these, the implementation I envision is to let the first
scan loop in findDependentObjects examine all the ref dependencies before
deciding what to do. If any one of the INTERNAL_AUTO dependencies is
being recursed from, we can allow the deletion to proceed. If not, do not
continue with the deletion, but save the target object's ID into a side
array, along with the ID of the object we determined to be its "true
owner". After completing the recursive dependency-tree walk, check each
object listed in the side array to see if it got deleted (by looking in
the main array of deletable objects that findDependentObjects reports).
If not, complain, citing the also-saved owning object ID as the object to
delete instead. This closes the hole I identified that the existing code
just assumes it can skip deleting the dependent object.
There are still some subtleties here: even if we didn't delete the
dependent object, someone else might've done so concurrently, which'd
result in getObjectDescription() failing if we just blindly try to
print an error message. We could fix that by re-locking the object
and seeing if it still exists before complaining; but I wonder if that
raises the risks of deadlocks materially.
regards, tom lane
On 2019-Jan-29, Tom Lane wrote: > The closest I could get to solving it along the original lines > went like this: > > * In addition, we support INTERNAL_AUTO dependencies, which alter the > * rules for this. If the target has both INTERNAL and INTERNAL_AUTO > * dependencies, then it can be dropped if any one of those objects is > * dropped, but not unless at least one of them is. In this situation we > * mustn't automatically transform a random deletion request into a > * parent-object deletion. Instead, we proceed with deletion if we are > * recursing from any owning object, and otherwise set the object aside to > * recheck at the end. If we don't later find a reason to delete one of > * the owning objects, we'll throw an error. > > I think that's ugly as sin: INTERNAL means something different if there's > another dependency beside it? I agree it's pretty weird :-( > A theoretically purer answer is to split INTERNAL_AUTO into two new > dependency types, one linking to the real "owning" object (the parent > index or trigger) and the other to the secondary owner (the partition > table), while leaving regular INTERNAL as it stands. I don't especially > want to go that way, though: it seems overcomplicated from the standpoint > of outside inspections of pg_depend, and it'd be very risky to back-patch > into v11. (For instance, if someone went backwards on their postmaster > minor release, the older v11 version would not know what to do with the > new dependency type.) I don't put a lot of value in the idea of people going back in minor releases, but I agree with the general principle of not making pg_depend any harder to inspect than it already is. Even so, I wouldn't dismiss a solution along these lines, if it was clean enough; we could provide better tools for pg_depend inspection (some sensible view, at least). It might make sense to implement this in master only, if it turns out to be better in some way. I'm not volunteering right now, but maybe in the future. > It strikes me however that we can stick with the existing catalog contents > by making this definition: of the INTERNAL_AUTO dependencies of a given > object, the "true owner" to be reported in errors is the dependency > that is of the same classId as that object. If this doesn't uniquely > identify one dependency, the report will mention a random one. This is > ugly as well, but it's certainly a lot more practical to cram into v11, > and it seems like it would cover the current and likely future use-cases > for partition-related objects. When and if we find a use-case for which > this doesn't work, we can think about extending the catalog representation > to identify a unique owner in a less ad-hoc way. This seems a great idea. Do you want me to implement it? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-29, Tom Lane wrote:
>> It strikes me however that we can stick with the existing catalog contents
>> by making this definition: of the INTERNAL_AUTO dependencies of a given
>> object, the "true owner" to be reported in errors is the dependency
>> that is of the same classId as that object. If this doesn't uniquely
>> identify one dependency, the report will mention a random one. This is
>> ugly as well, but it's certainly a lot more practical to cram into v11,
>> and it seems like it would cover the current and likely future use-cases
>> for partition-related objects. When and if we find a use-case for which
>> this doesn't work, we can think about extending the catalog representation
>> to identify a unique owner in a less ad-hoc way.
> This seems a great idea. Do you want me to implement it?
I've got much of the code for it already (in the wreckage of my failed
attempts), so I'll go back and finish that up. I was just waiting to see
how loudly people would howl about using object type as a condition for
figuring out what a pg_depend entry really means. If we're okay with
that hack, I think I can make it work.
regards, tom lane
On Tue, Feb 5, 2019 at 2:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I've got much of the code for it already (in the wreckage of my failed > attempts), so I'll go back and finish that up. I was just waiting to see > how loudly people would howl about using object type as a condition for > figuring out what a pg_depend entry really means. If we're okay with > that hack, I think I can make it work. Perhaps I've missed some subtlety, but I'm not sure that it's all that ugly. If splitting INTERNAL_AUTO into two new dependency types amounts to the same thing as what you suggest here, then what's the difference? If this secondary INTERNAL_AUTO entry property can be determined from the pg_depend record alone with either approach, then it's not obvious to me that an "explicit representation" buys us anything. Yes, you must introduce a special case...but isn't it a special case either way? -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Feb 5, 2019 at 2:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've got much of the code for it already (in the wreckage of my failed
>> attempts), so I'll go back and finish that up. I was just waiting to see
>> how loudly people would howl about using object type as a condition for
>> figuring out what a pg_depend entry really means. If we're okay with
>> that hack, I think I can make it work.
> Perhaps I've missed some subtlety, but I'm not sure that it's all that
> ugly. If splitting INTERNAL_AUTO into two new dependency types amounts
> to the same thing as what you suggest here, then what's the
> difference?
It's the same as long as we always think that the "real owner" of a
subsidiary object is of the same type as that object, eg that the
real owner of a per-partition trigger is the parent trigger, the real
owner of a per-partition index is the parent index, etc ... and that
there's only going to be one parent object of that type.
I don't immediately have a counterexample to this, which is why I feel
like this is an OK solution for now. But I'm not sure it'll hold good
indefinitely.
Actually, the index case is already on the edge of being a problem:
both parents will be relations, and I suspect the code will have to
look at relkinds to identify which parent to consider the "real owner".
BTW, does anyone else feel like our terminology around partitions is
a dead loss? I have not seen anybody use the word in a way that makes
a clear distinction between the parent "partitioned" object and the
child "partition" objects, at least not when it comes to subsidiary
objects like triggers.
regards, tom lane
I wrote:
>>> I've got much of the code for it already (in the wreckage of my failed
>>> attempts), so I'll go back and finish that up.
So I've been poking at that for most of the day, and I've despaired of
being able to fix this in v11. The problem is that somebody was rolling
dice while deciding which dependencies of what types to insert for
partitioning cases. In particular, try this (extracted from
foreign_key.sql):
CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b));
CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b);
ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;
CREATE TABLE fk_partitioned_fk_2 PARTITION OF fk_partitioned_fk FOR VALUES FROM (1,1) TO (10,10);
At this point the dependencies of the child table's FK constraint look
like
select
pg_describe_object(classid, objid, objsubid) as obj,
pg_describe_object(refclassid, refobjid, refobjsubid) as ref,
deptype, objid, refobjid
from pg_depend where objid =
(select oid from pg_constraint where conrelid = 'fk_partitioned_fk_2'::regclass);
obj | ref
| deptype | objid | refobjid
------------------------------------------------------------------+----------------------------------------------------------------+---------+-------+----------
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column a of table fk_partitioned_fk_2
| a | 52787 | 52784
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column b of table fk_partitioned_fk_2
| a | 52787 | 52784
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column a of table fk_notpartitioned_pk
| n | 52787 | 37209
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column b of table fk_notpartitioned_pk
| n | 52787 | 37209
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | index fk_notpartitioned_pk_pkey
| n | 52787 | 37215
constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | constraint fk_partitioned_fk_a_fkey on table
fk_partitioned_fk| I | 52787 | 52781
(6 rows)
Now, v11 will let you drop the fk_partitioned_fk_2 table, which
recurses to dropping this constraint, and it does not then complain
that you should've dropped the fk_partitioned_fk_a_fkey constraint.
Given these dependencies, that's just *wrong*. This dependency set
is no different from the bug case I exhibited upthread: arriving at
this object via an auto dependency should not be enough to let it
be dropped, if there's an 'I' dependency for it.
It's also fair to wonder what we're doing applying a single 'I'
dependency to an object in the first place; why not use a regular
internal dependency in that case?
Another problem I came across can be exhibited like this:
CREATE TABLE prt1 (a int, b int, c int) PARTITION BY RANGE(a);
CREATE INDEX ON prt1(c);
CREATE TABLE prt1_p3 PARTITION OF prt1 FOR VALUES FROM (500) TO (600);
alter table prt1 DETACH PARTITION prt1_p3;
At this point we have
select
pg_describe_object(classid, objid, objsubid) as obj,
pg_describe_object(refclassid, refobjid, refobjsubid) as ref,
deptype, objid, refobjid
from pg_depend where 'prt1_p3_c_idx'::regclass in (objid,refobjid);
obj | ref | deptype | objid | refobjid
---------------------+---------------+---------+-------+----------
index prt1_p3_c_idx | table prt1_p3 | a | 52797 | 52794
(1 row)
This is also flat wrong, because it wil let you do
alter table prt1_p3 drop column c;
and the index still exists, though in a broken state:
\d prt1_p3
Table "public.prt1_p3"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
b | integer | | |
Indexes:
"prt1_p3_c_idx" btree ("........pg.dropped.3........" int4_ops)
The reason for that is that IndexSetParentIndex thinks that if it's
unlinking an index from a partition index, all it needs to put back
is an auto dependency on the whole table. This has exactly nothing
to do with the dependencies that the index would normally have;
those are usually column-level not table-level. As this example shows,
you do not get to take shortcuts.
I believe that we need to establish the following principles:
* The terminology "internal_auto" is disastrously unhelpful.
I propose calling these things "partition" dependencies instead.
* Partition dependencies are not singletons. They should *always*
come in pairs, one on the parent partitioned object (partitioned
index, constraint, trigger, etc) and one on the child partitioned
table. (Have I mentioned that our partition/partitioned terminology
is also a disaster?) Maybe someday there'll be a reason to have
three or more of these for the same dependent object, but there's
no reason to have just one --- you should use an internal dependency
instead for that case.
* Partition dependencies are made *in addition to*, not instead of,
the dependencies the object would normally have. In this way,
for example, the unlink action in IndexSetParentIndex would just
delete the partition dependencies and not have to worry about putting
back the index's usual dependencies. Consistent with that, it's
simply wrong that index_create sometimes marks the "usual"
dependencies as internal_auto rather than auto. (It wasn't even
doing that consistently; expression and predicate column dependencies
were still "auto", which makes no sense at all.) They should go
back to being plain auto with the partition dependencies being added
separately. That will work properly given the changes that say that
arriving at a partition-dependent object via an auto dependency is
not sufficient license to delete the object.
I have a mostly-working patch along these lines that I hope to
finish up and post tomorrow. But there's no hope of applying it
to v11 --- if presented with the dependencies that v11 is currently
storing, the patch would refuse deletion of partitioned tables altogether
in many common cases. That cure's certainly worse than the disease.
regards, tom lane
On Wed, Feb 6, 2019 at 9:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I have a mostly-working patch along these lines that I hope to > finish up and post tomorrow. Do you think that you'll end up pushing the HEAD-only fix shortly? It would be nice to avoid immediate bitrot of an upcoming revision of the nbtree patch series. Thanks -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Wed, Feb 6, 2019 at 9:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I have a mostly-working patch along these lines that I hope to
>> finish up and post tomorrow.
> Do you think that you'll end up pushing the HEAD-only fix shortly?
I have a working patch now, but I think I'm too tired to explain it,
so I'm going to post it tomorrow instead. It's a big enough change
that it might be advisable for someone to review it --- are you
interested?
regards, tom lane
On Thu, Feb 7, 2019 at 7:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I have a working patch now, but I think I'm too tired to explain it, > so I'm going to post it tomorrow instead. It's a big enough change > that it might be advisable for someone to review it --- are you > interested? I'd be happy to review it. -- Peter Geoghegan
I wrote:
> I believe that we need to establish the following principles:
>
> * The terminology "internal_auto" is disastrously unhelpful.
> I propose calling these things "partition" dependencies instead.
>
> * Partition dependencies are not singletons. They should *always*
> come in pairs, one on the parent partitioned object (partitioned
> index, constraint, trigger, etc) and one on the child partitioned
> table. (Have I mentioned that our partition/partitioned terminology
> is also a disaster?) Maybe someday there'll be a reason to have
> three or more of these for the same dependent object, but there's
> no reason to have just one --- you should use an internal dependency
> instead for that case.
>
> * Partition dependencies are made *in addition to*, not instead of,
> the dependencies the object would normally have. In this way,
> for example, the unlink action in IndexSetParentIndex would just
> delete the partition dependencies and not have to worry about putting
> back the index's usual dependencies. Consistent with that, it's
> simply wrong that index_create sometimes marks the "usual"
> dependencies as internal_auto rather than auto. (It wasn't even
> doing that consistently; expression and predicate column dependencies
> were still "auto", which makes no sense at all.) They should go
> back to being plain auto with the partition dependencies being added
> separately. That will work properly given the changes that say that
> arriving at a partition-dependent object via an auto dependency is
> not sufficient license to delete the object.
Here's a patch along these lines. Some notes worth making:
* After spending a fair amount of effort on the description of the
dependency types in catalogs.sgml, I decided to just rip out the
duplicative text in dependency.h in favor of a pointer to catalogs.sgml.
If anybody's really hot to have two copies, we could put that back, but
I don't see the point.
* The core idea of the changes in dependency.c is just to wait till the
end of the entire dependency tree traversal, and then (before we start
actually deleting anything) check to make sure that each targeted
partition-dependent object has been reached via a partition-type
dependency, implying that at least one of its partition owners was
deleted. The existing data structure handles this nicely, we just
need a couple more flag bits for the specific need. (BTW, I experimented
with whether we could handle internal dependencies the same way; but
it didn't work, because of the previously-poorly-documented fact that
an internal dependency is immediately turned into a reverse-direction
recursion. We can't really muck with the dependency traversal order,
or we find ourselves deleting tables before their indices, etc.)
* I found two different ways in which dependency.c was still producing
traversal-order-sensitive results. One we already understood is that the
first loop in findDependentObjects threw an error for the first internal
or partition dependency it came across; since an object can have both of
those, the results varied. This was fixed by postponing the actual error
report to after the loop and adding an explicit preference order for what
to report.
* The other such issue is a pre-existing bug, which maybe we ought to
back-patch, though I can't recall seeing any field reports that seem
to match it: after recursing to an internal/extension dependency,
we need to ensure that whatever objflags were passed down to our level
get applied to the targetObjects entry for the current object. Otherwise
the final state of the entry can vary depending on traversal order
(since orders in which the current call sees the object already in
targetObjects, or on the stack, would apply the objflags). This also
provides a chance to verify, not just assume, that processing of the
internal/extension dependency deleted the current object. As I mentioned
the other day, failing to ensure that the current object gets deleted
is very bad, and the pg_depend network is no longer so immutable that
we really ought to just assume that control came back to our object.
* The changes outside dependency.c just amount to applying the rules
stated above about how to use partition dependencies. I reverted the
places where v11 had decided that partition dependencies could be
substituted for auto dependencies, and made sure they are always
created in pairs.
There are a couple of things I didn't do here because those parts
of the patch were written while I still had some hope of applying
it to v11. Given that a catversion bump is needed anyway due to
the changes in what pg_depend entries are created for partitions,
we could change these:
* I renamed the dependency type to DEPENDENCY_PARTITION internally,
but left its pg_depend representation as 'I'. In a green field
we'd probably have made it 'P' instead.
* We could get rid of partition_dependency_matches(), which is not
really anything but a kluge, by splitting DEPENDENCY_PARTITION
into two dependency types, say DEPENDENCY_PARTITION_PRI ('P')
and DEPENDENCY_PARTITION_SEC ('S').
The main argument against changing these would be the risk that
client code already knows about 'I'. But neither pg_dump nor psql
do, so I judge that probably there's little if anything out there
that is special-casing that dependency type.
Also, I came across some coding in CloneFkReferencing() that looks fishy
as hell: that function imagines that it can delete an existing trigger
with nothing more than a summary CatalogTupleDelete(). I didn't do
anything about that here, but if it's not broken, I'd like to see an
explanation why not. I added a comment complaining about the lack of
pg_depend cleanup, and there's also the question of whether we don't
need to broadcast a relcache inval for the trigger's table.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index af4d062..7731d46 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
*************** SCRAM-SHA-256$<replaceable><iteration
*** 2970,2976 ****
referenced object, and should be automatically dropped
(regardless of <literal>RESTRICT</literal> or <literal>CASCADE</literal>
mode) if the referenced object is dropped. Example: a named
! constraint on a table is made autodependent on the table, so
that it will go away if the table is dropped.
</para>
</listitem>
--- 2970,2976 ----
referenced object, and should be automatically dropped
(regardless of <literal>RESTRICT</literal> or <literal>CASCADE</literal>
mode) if the referenced object is dropped. Example: a named
! constraint on a table is made auto-dependent on the table, so
that it will go away if the table is dropped.
</para>
</listitem>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 2982,3019 ****
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A <command>DROP</command> of the dependent object
! will be disallowed outright (we'll tell the user to issue a
! <command>DROP</command> against the referenced object, instead). A
! <command>DROP</command> of the referenced object will be propagated
! through to drop the dependent object whether
! <command>CASCADE</command> is specified or not. Example: a trigger
! that's created to enforce a foreign-key constraint is made
! internally dependent on the constraint's
! <structname>pg_constraint</structname> entry.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><symbol>DEPENDENCY_INTERNAL_AUTO</symbol> (<literal>I</literal>)</term>
<listitem>
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A <command>DROP</command> of the dependent object
! will be disallowed outright (we'll tell the user to issue a
! <command>DROP</command> against the referenced object, instead).
! While a regular internal dependency will prevent
! the dependent object from being dropped while any such dependencies
! remain, <literal>DEPENDENCY_INTERNAL_AUTO</literal> will allow such
! a drop as long as the object can be found by following any of such
! dependencies.
! Example: an index on a partition is made internal-auto-dependent on
! both the partition itself as well as on the index on the parent
! partitioned table; so the partition index is dropped together with
! either the partition it indexes, or with the parent index it is
! attached to.
</para>
</listitem>
</varlistentry>
--- 2982,3032 ----
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A direct <command>DROP</command> of the dependent
! object will be disallowed outright (we'll tell the user to issue
! a <command>DROP</command> against the referenced object, instead).
! A <command>DROP</command> of the referenced object will result in
! automatically dropping the dependent object
! whether <literal>CASCADE</literal> is specified or not. If the
! dependent object is reached due to a dependency on some other object,
! the drop is converted to a drop of the referenced object, so
! that <literal>NORMAL</literal> and <literal>AUTO</literal>
! dependencies of the dependent object behave much like they were
! dependencies of the referenced object.
! Example: a view's <literal>ON SELECT</literal> rule is made
! internally dependent on the view, preventing it from being dropped
! while the view remains. Dependencies of the rule (such as tables it
! refers to) act as if they were dependencies of the view.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><symbol>DEPENDENCY_PARTITION</symbol> (<literal>I</literal>)</term>
<listitem>
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation; however, unlike <literal>INTERNAL</literal>,
! there is more than one such referenced object. The dependent object
! must not be dropped unless at least one of these referenced objects
! is dropped; if any one is, the dependent object should be dropped
! whether or not <literal>CASCADE</literal> is specified. Also
! unlike <literal>INTERNAL</literal>, a drop of some other object
! that the dependent object depends on does not result in automatic
! deletion of any partition-referenced object. Hence, if the drop
! does not cascade to at least one of these objects via some other
! path, it will be refused. (In most cases, the dependent object
! shares all its non-partition dependencies with at least one
! partition-referenced object, so that this restriction does not
! result in blocking any cascaded delete.)
! Note that partition dependencies are made in addition to, not
! instead of, any dependencies the object would normally have. This
! simplifies <command>ATTACH/DETACH PARTITION</command> operations:
! the partition dependencies need only be added or removed.
! Example: a child partitioned index is made partition-dependent
! on both the partition table it is on and the parent partitioned
! index, so that it goes away if either of those is dropped.
</para>
</listitem>
</varlistentry>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 3026,3034 ****
the referenced object (see
<link linkend="catalog-pg-extension"><structname>pg_extension</structname></link>).
The dependent object can be dropped only via
! <command>DROP EXTENSION</command> on the referenced object. Functionally
! this dependency type acts the same as an internal dependency, but
! it's kept separate for clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
--- 3039,3048 ----
the referenced object (see
<link linkend="catalog-pg-extension"><structname>pg_extension</structname></link>).
The dependent object can be dropped only via
! <command>DROP EXTENSION</command> on the referenced object.
! Functionally this dependency type acts the same as
! an <literal>INTERNAL</literal> dependency, but it's kept separate for
! clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 3038,3047 ****
<listitem>
<para>
The dependent object is not a member of the extension that is the
! referenced object (and so should not be ignored by pg_dump), but
! cannot function without it and should be dropped when the
! extension itself is. The dependent object may be dropped on its
! own as well.
</para>
</listitem>
</varlistentry>
--- 3052,3064 ----
<listitem>
<para>
The dependent object is not a member of the extension that is the
! referenced object (and so it should not be ignored
! by <application>pg_dump</application>), but it cannot function
! without the extension and should be auto-dropped if the extension is.
! The dependent object may be dropped on its own as well.
! Functionally this dependency type acts the same as
! an <literal>AUTO</literal> dependency, but it's kept separate for
! clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2c54895..702dbf3 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** typedef struct
*** 99,107 ****
#define DEPFLAG_NORMAL 0x0002 /* reached via normal dependency */
#define DEPFLAG_AUTO 0x0004 /* reached via auto dependency */
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
! #define DEPFLAG_EXTENSION 0x0010 /* reached via extension dependency */
! #define DEPFLAG_REVERSE 0x0020 /* reverse internal/extension link */
! #define DEPFLAG_SUBOBJECT 0x0040 /* subobject of another deletable object */
/* expansible list of ObjectAddresses */
--- 99,109 ----
#define DEPFLAG_NORMAL 0x0002 /* reached via normal dependency */
#define DEPFLAG_AUTO 0x0004 /* reached via auto dependency */
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
! #define DEPFLAG_PARTITION 0x0010 /* reached via partition dependency */
! #define DEPFLAG_EXTENSION 0x0020 /* reached via extension dependency */
! #define DEPFLAG_REVERSE 0x0040 /* reverse internal/extension link */
! #define DEPFLAG_IS_PART 0x0080 /* has a partition dependency */
! #define DEPFLAG_SUBOBJECT 0x0100 /* subobject of another deletable object */
/* expansible list of ObjectAddresses */
*************** static void reportDependentObjects(const
*** 194,199 ****
--- 196,203 ----
DropBehavior behavior,
int flags,
const ObjectAddress *origObject);
+ static bool partition_dependency_matches(const ObjectAddress *object1,
+ const ObjectAddress *object2);
static void deleteOneObject(const ObjectAddress *object,
Relation *depRel, int32 flags);
static void doDeletion(const ObjectAddress *object, int flags);
*************** findDependentObjects(const ObjectAddress
*** 478,483 ****
--- 482,489 ----
SysScanDesc scan;
HeapTuple tup;
ObjectAddress otherObject;
+ ObjectAddress owningObject;
+ ObjectAddress partitionObject;
ObjectAddressAndFlags *dependentObjects;
int numDependentObjects;
int maxDependentObjects;
*************** findDependentObjects(const ObjectAddress
*** 547,552 ****
--- 553,562 ----
scan = systable_beginscan(*depRel, DependDependerIndexId, true,
NULL, nkeys, key);
+ /* initialize variables that loop may fill */
+ memset(&owningObject, 0, sizeof(owningObject));
+ memset(&partitionObject, 0, sizeof(partitionObject));
+
while (HeapTupleIsValid(tup = systable_getnext(scan)))
{
Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup);
*************** findDependentObjects(const ObjectAddress
*** 591,614 ****
/* FALL THRU */
case DEPENDENCY_INTERNAL:
- case DEPENDENCY_INTERNAL_AUTO:
/*
* This object is part of the internal implementation of
* another object, or is part of the extension that is the
* other object. We have three cases:
*
! * 1. At the outermost recursion level, disallow the DROP. (We
! * just ereport here, rather than proceeding, since no other
! * dependencies are likely to be interesting.) However, if
! * the owning object is listed in pendingObjects, just release
! * the caller's lock and return; we'll eventually complete the
! * DROP when we reach that entry in the pending list.
*/
if (stack == NULL)
{
- char *otherObjDesc;
-
if (pendingObjects &&
object_address_present(&otherObject, pendingObjects))
{
--- 601,626 ----
/* FALL THRU */
case DEPENDENCY_INTERNAL:
/*
* This object is part of the internal implementation of
* another object, or is part of the extension that is the
* other object. We have three cases:
*
! * 1. At the outermost recursion level, we must disallow the
! * DROP. However, if the owning object is listed in
! * pendingObjects, just release the caller's lock and return;
! * we'll eventually complete the DROP when we reach that entry
! * in the pending list.
! *
! * Note: the above statement is true only if this pg_depend
! * entry still exists by then; in principle, therefore, we
! * could miss deleting an item the user told us to delete.
! * However, no inconsistency can result: since we're at outer
! * level, there is no object depending on this one.
*/
if (stack == NULL)
{
if (pendingObjects &&
object_address_present(&otherObject, pendingObjects))
{
*************** findDependentObjects(const ObjectAddress
*** 617,630 ****
ReleaseDeletionLock(object);
return;
}
! otherObjDesc = getObjectDescription(&otherObject);
! ereport(ERROR,
! (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
! errmsg("cannot drop %s because %s requires it",
! getObjectDescription(object),
! otherObjDesc),
! errhint("You can drop %s instead.",
! otherObjDesc)));
}
/*
--- 629,645 ----
ReleaseDeletionLock(object);
return;
}
!
! /*
! * We postpone actually issuing the error message until
! * after this loop, so that we can make the behavior
! * independent of the ordering of pg_depend entries, at
! * least so long as there's just one INTERNAL + EXTENSION
! * dependency. (If there's more, we'll complain about a
! * random one of them.)
! */
! owningObject = otherObject;
! break;
}
/*
*************** findDependentObjects(const ObjectAddress
*** 643,656 ****
* transform this deletion request into a delete of this
* owning object.
*
- * For INTERNAL_AUTO dependencies, we don't enforce this; in
- * other words, we don't follow the links back to the owning
- * object.
- */
- if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO)
- break;
-
- /*
* First, release caller's lock on this object and get
* deletion lock on the owning object. (We must release
* caller's lock to avoid deadlock against a concurrent
--- 658,663 ----
*************** findDependentObjects(const ObjectAddress
*** 673,678 ****
--- 680,692 ----
}
/*
+ * One way or the other, we're done with the scan; might as
+ * well close it down before recursing, to reduce peak
+ * resource consumption.
+ */
+ systable_endscan(scan);
+
+ /*
* Okay, recurse to the owning object instead of proceeding.
*
* We do not need to stack the current object; we want the
*************** findDependentObjects(const ObjectAddress
*** 690,699 ****
targetObjects,
pendingObjects,
depRel);
/* And we're done here. */
- systable_endscan(scan);
return;
case DEPENDENCY_PIN:
/*
--- 704,750 ----
targetObjects,
pendingObjects,
depRel);
+
+ /*
+ * We should have added the current target object while
+ * processing the owning object; if not, something's very
+ * wrong. While we're checking that, also make sure that the
+ * current object gets marked with whatever objflags we were
+ * given, just as though it'd been in targetObjects when we
+ * got here. (Otherwise, we might get a bogus failure from
+ * reportDependentObjects.)
+ */
+ if (!object_address_present_add_flags(object, objflags,
+ targetObjects))
+ elog(ERROR, "deletion of owning object %s failed to delete %s",
+ getObjectDescription(&otherObject),
+ getObjectDescription(object));
+
/* And we're done here. */
return;
+ case DEPENDENCY_PARTITION:
+
+ /*
+ * If this is the "most important" partition dependency,
+ * remember it for possible use in the error message. We
+ * consider the most important dependency to be one of the
+ * same classId (and relkind, if they're relations) as the
+ * target object. There should be exactly one such
+ * dependency, or we'll report a random one of them...
+ */
+ if (!(objflags & DEPFLAG_IS_PART) ||
+ partition_dependency_matches(object, &otherObject))
+ partitionObject = otherObject;
+
+ /*
+ * Remember that this object has a partition-type dependency.
+ * After the dependency scan, we'll complain if we didn't find
+ * a reason to delete one of its partition dependencies.
+ */
+ objflags |= DEPFLAG_IS_PART;
+ break;
+
case DEPENDENCY_PIN:
/*
*************** findDependentObjects(const ObjectAddress
*** 713,718 ****
--- 764,791 ----
systable_endscan(scan);
/*
+ * If we found an INTERNAL or EXTENSION dependency when we're at outer
+ * level, complain about it now. If we also found a PARTITION dependency,
+ * we prefer to report the PARTITION dependency. This is arbitrary but
+ * seems to be more useful in practice.
+ */
+ if (OidIsValid(owningObject.classId))
+ {
+ char *otherObjDesc;
+
+ if (OidIsValid(partitionObject.classId))
+ otherObjDesc = getObjectDescription(&partitionObject);
+ else
+ otherObjDesc = getObjectDescription(&owningObject);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s requires it",
+ getObjectDescription(object), otherObjDesc),
+ errhint("You can drop %s instead.", otherObjDesc)));
+ }
+
+ /*
* Next, identify all objects that directly depend on the current object.
* To ensure predictable deletion order, we collect them up in
* dependentObjects and sort the list before actually recursing. (The
*************** findDependentObjects(const ObjectAddress
*** 789,798 ****
case DEPENDENCY_AUTO_EXTENSION:
subflags = DEPFLAG_AUTO;
break;
- case DEPENDENCY_INTERNAL_AUTO:
case DEPENDENCY_INTERNAL:
subflags = DEPFLAG_INTERNAL;
break;
case DEPENDENCY_EXTENSION:
subflags = DEPFLAG_EXTENSION;
break;
--- 862,873 ----
case DEPENDENCY_AUTO_EXTENSION:
subflags = DEPFLAG_AUTO;
break;
case DEPENDENCY_INTERNAL:
subflags = DEPFLAG_INTERNAL;
break;
+ case DEPENDENCY_PARTITION:
+ subflags = DEPFLAG_PARTITION;
+ break;
case DEPENDENCY_EXTENSION:
subflags = DEPFLAG_EXTENSION;
break;
*************** findDependentObjects(const ObjectAddress
*** 868,877 ****
/*
* Finally, we can add the target object to targetObjects. Be careful to
* include any flags that were passed back down to us from inner recursion
! * levels.
*/
extra.flags = mystack.flags;
! if (stack)
extra.dependee = *stack->object;
else
memset(&extra.dependee, 0, sizeof(extra.dependee));
--- 943,957 ----
/*
* Finally, we can add the target object to targetObjects. Be careful to
* include any flags that were passed back down to us from inner recursion
! * levels. Record the "dependee" as being either the most important
! * partition owner if there is one, else the object we recursed from, if
! * any. (The logic in reportDependentObjects() is such that it can only
! * need one of those objects.)
*/
extra.flags = mystack.flags;
! if (extra.flags & DEPFLAG_IS_PART)
! extra.dependee = partitionObject;
! else if (stack)
extra.dependee = *stack->object;
else
memset(&extra.dependee, 0, sizeof(extra.dependee));
*************** reportDependentObjects(const ObjectAddre
*** 906,913 ****
int i;
/*
* If no error is to be thrown, and the msglevel is too low to be shown to
! * either client or server log, there's no need to do any of the work.
*
* Note: this code doesn't know all there is to be known about elog
* levels, but it works for NOTICE and DEBUG2, which are the only values
--- 986,1022 ----
int i;
/*
+ * If we need to delete any partition-dependent objects, make sure that
+ * we're deleting at least one of their partition dependencies, too. That
+ * can be detected by checking that we reached them by a PARTITION
+ * dependency at some point.
+ *
+ * We just report the first such object, as in most cases the only way to
+ * trigger this complaint is to explicitly try to delete one partition of
+ * a partitioned object.
+ */
+ for (i = 0; i < targetObjects->numrefs; i++)
+ {
+ const ObjectAddressExtra *extra = &targetObjects->extras[i];
+
+ if ((extra->flags & DEPFLAG_IS_PART) &&
+ !(extra->flags & DEPFLAG_PARTITION))
+ {
+ const ObjectAddress *object = &targetObjects->refs[i];
+ char *otherObjDesc = getObjectDescription(&extra->dependee);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s requires it",
+ getObjectDescription(object), otherObjDesc),
+ errhint("You can drop %s instead.", otherObjDesc)));
+ }
+ }
+
+ /*
* If no error is to be thrown, and the msglevel is too low to be shown to
! * either client or server log, there's no need to do any of the rest of
! * the work.
*
* Note: this code doesn't know all there is to be known about elog
* levels, but it works for NOTICE and DEBUG2, which are the only values
*************** reportDependentObjects(const ObjectAddre
*** 951,961 ****
/*
* If, at any stage of the recursive search, we reached the object via
! * an AUTO, INTERNAL, or EXTENSION dependency, then it's okay to
! * delete it even in RESTRICT mode.
*/
if (extra->flags & (DEPFLAG_AUTO |
DEPFLAG_INTERNAL |
DEPFLAG_EXTENSION))
{
/*
--- 1060,1071 ----
/*
* If, at any stage of the recursive search, we reached the object via
! * an AUTO, INTERNAL, PARTITION, or EXTENSION dependency, then it's
! * okay to delete it even in RESTRICT mode.
*/
if (extra->flags & (DEPFLAG_AUTO |
DEPFLAG_INTERNAL |
+ DEPFLAG_PARTITION |
DEPFLAG_EXTENSION))
{
/*
*************** reportDependentObjects(const ObjectAddre
*** 1063,1068 ****
--- 1173,1208 ----
}
/*
+ * Decide whether object1 and object2 are "the same type" of object.
+ * This is a bit of a hack.
+ */
+ static bool
+ partition_dependency_matches(const ObjectAddress *object1,
+ const ObjectAddress *object2)
+ {
+ if (object1->classId == object2->classId)
+ {
+ if (object1->classId == RelationRelationId)
+ {
+ char relkind1 = get_rel_relkind(object1->objectId);
+ char relkind2 = get_rel_relkind(object2->objectId);
+
+ /* Indexes and partitioned indexes are the same type of object */
+ if (relkind1 == RELKIND_PARTITIONED_INDEX)
+ relkind1 = RELKIND_INDEX;
+ if (relkind2 == RELKIND_PARTITIONED_INDEX)
+ relkind2 = RELKIND_INDEX;
+
+ if (relkind1 == relkind2)
+ return true;
+ }
+ else
+ return true;
+ }
+ return false;
+ }
+
+ /*
* deleteOneObject: delete a single object for performDeletion.
*
* *depRel is the already-open pg_depend relation.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index faf6956..fb01999 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
*************** index_create(Relation heapRelation,
*** 1041,1049 ****
else
{
bool have_simple_col = false;
- DependencyType deptype;
-
- deptype = OidIsValid(parentIndexRelid) ? DEPENDENCY_INTERNAL_AUTO : DEPENDENCY_AUTO;
/* Create auto dependencies on simply-referenced columns */
for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
--- 1041,1046 ----
*************** index_create(Relation heapRelation,
*** 1054,1060 ****
referenced.objectId = heapRelationId;
referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
! recordDependencyOn(&myself, &referenced, deptype);
have_simple_col = true;
}
--- 1051,1057 ----
referenced.objectId = heapRelationId;
referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
have_simple_col = true;
}
*************** index_create(Relation heapRelation,
*** 1072,1089 ****
referenced.objectId = heapRelationId;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, deptype);
}
}
! /* Store dependency on parent index, if any */
if (OidIsValid(parentIndexRelid))
{
referenced.classId = RelationRelationId;
referenced.objectId = parentIndexRelid;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
/* Store dependency on collations */
--- 1069,1097 ----
referenced.objectId = heapRelationId;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
}
}
! /*
! * If this is an index partition, create partition dependencies on
! * both the parent index and the table. (Note: these must be *in
! * addition to*, not instead of, all other dependencies. Otherwise
! * we'll be short some dependencies after DETACH PARTITION.)
! */
if (OidIsValid(parentIndexRelid))
{
referenced.classId = RelationRelationId;
referenced.objectId = parentIndexRelid;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
!
! referenced.classId = RelationRelationId;
! referenced.objectId = heapRelationId;
! referenced.objectSubId = 0;
!
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
}
/* Store dependency on collations */
*************** index_constraint_create(Relation heapRel
*** 1342,1356 ****
recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
/*
! * Also, if this is a constraint on a partition, mark it as depending on
! * the constraint in the parent.
*/
if (OidIsValid(parentConstraintId))
{
! ObjectAddress parentConstr;
!
! ObjectAddressSet(parentConstr, ConstraintRelationId, parentConstraintId);
! recordDependencyOn(&referenced, &parentConstr, DEPENDENCY_INTERNAL_AUTO);
}
/*
--- 1350,1366 ----
recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
/*
! * Also, if this is a constraint on a partition, give it partition-type
! * dependencies on the parent constraint as well as the table.
*/
if (OidIsValid(parentConstraintId))
{
! ObjectAddressSet(myself, ConstraintRelationId, conOid);
! ObjectAddressSet(referenced, ConstraintRelationId, parentConstraintId);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
! ObjectAddressSet(referenced, RelationRelationId,
! RelationGetRelid(heapRelation));
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
}
/*
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 698b493..4023c3d 100644
*** a/src/backend/catalog/pg_constraint.c
--- b/src/backend/catalog/pg_constraint.c
*************** AlterConstraintNamespaces(Oid ownerId, O
*** 760,772 ****
/*
* ConstraintSetParentConstraint
! * Set a partition's constraint as child of its parent table's
*
* This updates the constraint's pg_constraint row to show it as inherited, and
! * add a dependency to the parent so that it cannot be removed on its own.
*/
void
! ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
{
Relation constrRel;
Form_pg_constraint constrForm;
--- 760,776 ----
/*
* ConstraintSetParentConstraint
! * Set a partition's constraint as child of its parent constraint,
! * or remove the linkage if parentConstrId is InvalidOid.
*
* This updates the constraint's pg_constraint row to show it as inherited, and
! * adds PARTITION dependencies to prevent the constraint from being deleted
! * on its own. Alternatively, reverse that.
*/
void
! ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId,
! Oid childTableId)
{
Relation constrRel;
Form_pg_constraint constrForm;
*************** ConstraintSetParentConstraint(Oid childC
*** 795,804 ****
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
- ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId);
ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
else
{
--- 799,811 ----
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
! ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION);
!
! ObjectAddressSet(referenced, RelationRelationId, childTableId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION);
}
else
{
*************** ConstraintSetParentConstraint(Oid childC
*** 809,818 ****
/* Make sure there's no further inheritance. */
Assert(constrForm->coninhcount == 0);
deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
ConstraintRelationId,
! DEPENDENCY_INTERNAL_AUTO);
! CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
}
ReleaseSysCache(tuple);
--- 816,829 ----
/* Make sure there's no further inheritance. */
Assert(constrForm->coninhcount == 0);
+ CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
+
deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
ConstraintRelationId,
! DEPENDENCY_PARTITION);
! deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
! RelationRelationId,
! DEPENDENCY_PARTITION);
}
ReleaseSysCache(tuple);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index bd85099..207e1ff 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
*************** DefineIndex(Oid relationId,
*** 971,977 ****
IndexSetParentIndex(cldidx, indexRelationId);
if (createdConstraintId != InvalidOid)
ConstraintSetParentConstraint(cldConstrOid,
! createdConstraintId);
if (!cldidx->rd_index->indisvalid)
invalidate_parent = true;
--- 971,978 ----
IndexSetParentIndex(cldidx, indexRelationId);
if (createdConstraintId != InvalidOid)
ConstraintSetParentConstraint(cldConstrOid,
! createdConstraintId,
! childRelid);
if (!cldidx->rd_index->indisvalid)
invalidate_parent = true;
*************** IndexSetParentIndex(Relation partitionId
*** 2622,2656 ****
if (fix_dependencies)
{
- ObjectAddress partIdx;
-
/*
! * Insert/delete pg_depend rows. If setting a parent, add an
! * INTERNAL_AUTO dependency to the parent index; if making standalone,
! * remove all existing rows and put back the regular dependency on the
! * table.
*/
- ObjectAddressSet(partIdx, RelationRelationId, partRelid);
-
if (OidIsValid(parentOid))
{
ObjectAddress parentIdx;
ObjectAddressSet(parentIdx, RelationRelationId, parentOid);
! recordDependencyOn(&partIdx, &parentIdx, DEPENDENCY_INTERNAL_AUTO);
}
else
{
- ObjectAddress partitionTbl;
-
- ObjectAddressSet(partitionTbl, RelationRelationId,
- partitionIdx->rd_index->indrelid);
-
deleteDependencyRecordsForClass(RelationRelationId, partRelid,
RelationRelationId,
! DEPENDENCY_INTERNAL_AUTO);
!
! recordDependencyOn(&partIdx, &partitionTbl, DEPENDENCY_AUTO);
}
/* make our updates visible */
--- 2623,2651 ----
if (fix_dependencies)
{
/*
! * Insert/delete pg_depend rows. If setting a parent, add PARTITION
! * dependencies on the parent index and the table; if removing a
! * parent, delete PARTITION dependencies.
*/
if (OidIsValid(parentOid))
{
+ ObjectAddress partIdx;
ObjectAddress parentIdx;
+ ObjectAddress partitionTbl;
+ ObjectAddressSet(partIdx, RelationRelationId, partRelid);
ObjectAddressSet(parentIdx, RelationRelationId, parentOid);
! ObjectAddressSet(partitionTbl, RelationRelationId,
! partitionIdx->rd_index->indrelid);
! recordDependencyOn(&partIdx, &parentIdx, DEPENDENCY_PARTITION);
! recordDependencyOn(&partIdx, &partitionTbl, DEPENDENCY_PARTITION);
}
else
{
deleteDependencyRecordsForClass(RelationRelationId, partRelid,
RelationRelationId,
! DEPENDENCY_PARTITION);
}
/* make our updates visible */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 35a9ade..f4d95e0 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** CloneFkReferencing(Relation pg_constrain
*** 7825,7831 ****
bool attach_it;
Oid constrOid;
ObjectAddress parentAddr,
! childAddr;
ListCell *cell;
int i;
--- 7825,7832 ----
bool attach_it;
Oid constrOid;
ObjectAddress parentAddr,
! childAddr,
! childTableAddr;
ListCell *cell;
int i;
*************** CloneFkReferencing(Relation pg_constrain
*** 7945,7957 ****
trgform->oid,
ConstraintRelationId,
DEPENDENCY_INTERNAL);
CatalogTupleDelete(trigrel, &trigtup->t_self);
}
systable_endscan(scan);
table_close(trigrel, RowExclusiveLock);
! ConstraintSetParentConstraint(fk->conoid, parentConstrOid);
CommandCounterIncrement();
attach_it = true;
break;
--- 7946,7963 ----
trgform->oid,
ConstraintRelationId,
DEPENDENCY_INTERNAL);
+ /*
+ * XXX surely this is an entirely inadequate way to delete
+ * a trigger. What of other dependencies, for example?
+ */
CatalogTupleDelete(trigrel, &trigtup->t_self);
}
systable_endscan(scan);
table_close(trigrel, RowExclusiveLock);
! ConstraintSetParentConstraint(fk->conoid, parentConstrOid,
! RelationGetRelid(partRel));
CommandCounterIncrement();
attach_it = true;
break;
*************** CloneFkReferencing(Relation pg_constrain
*** 7998,8005 ****
1, false, true);
subclone = lappend_oid(subclone, constrOid);
ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
! recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_INTERNAL_AUTO);
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
--- 8004,8015 ----
1, false, true);
subclone = lappend_oid(subclone, constrOid);
+ /* Set up partition dependencies for the new constraint */
ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
! recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_PARTITION);
! ObjectAddressSet(childTableAddr, RelationRelationId,
! RelationGetRelid(partRel));
! recordDependencyOn(&childAddr, &childTableAddr, DEPENDENCY_PARTITION);
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
*************** AttachPartitionEnsureIndexes(Relation re
*** 14878,14884 ****
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrOid, constraintOid);
update_relispartition(NULL, cldIdxId, true);
found = true;
break;
--- 14888,14895 ----
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrOid, constraintOid,
! RelationGetRelid(attachrel));
update_relispartition(NULL, cldIdxId, true);
found = true;
break;
*************** ATExecDetachPartition(Relation rel, Rang
*** 15136,15142 ****
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);
if (OidIsValid(constrOid))
! ConstraintSetParentConstraint(constrOid, InvalidOid);
index_close(idx, NoLock);
}
--- 15147,15153 ----
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);
if (OidIsValid(constrOid))
! ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid);
index_close(idx, NoLock);
}
*************** ATExecDetachPartition(Relation rel, Rang
*** 15168,15174 ****
}
/* unset conparentid and adjust conislocal, coninhcount, etc. */
! ConstraintSetParentConstraint(fk->conoid, InvalidOid);
/*
* Make the action triggers on the referenced relation. When this was
--- 15179,15185 ----
}
/* unset conparentid and adjust conislocal, coninhcount, etc. */
! ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
/*
* Make the action triggers on the referenced relation. When this was
*************** ATExecAttachPartitionIdx(List **wqueue,
*** 15404,15410 ****
/* All good -- do it */
IndexSetParentIndex(partIdx, RelationGetRelid(parentIdx));
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrId, constraintOid);
update_relispartition(NULL, partIdxId, true);
pfree(attmap);
--- 15415,15422 ----
/* All good -- do it */
IndexSetParentIndex(partIdx, RelationGetRelid(parentIdx));
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrId, constraintOid,
! RelationGetRelid(partTbl));
update_relispartition(NULL, partIdxId, true);
pfree(attmap);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 0b245a6..5e6437a 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 1012,1028 ****
* User CREATE TRIGGER, so place dependencies. We make trigger be
* auto-dropped if its relation is dropped or if the FK relation is
* dropped. (Auto drop is compatible with our pre-7.3 behavior.)
- *
- * Exception: if this trigger comes from a parent partitioned table,
- * then it's not separately drop-able, but goes away if the partition
- * does.
*/
referenced.classId = RelationRelationId;
referenced.objectId = RelationGetRelid(rel);
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, OidIsValid(parentTriggerOid) ?
! DEPENDENCY_INTERNAL_AUTO :
! DEPENDENCY_AUTO);
if (OidIsValid(constrrelid))
{
--- 1012,1022 ----
* User CREATE TRIGGER, so place dependencies. We make trigger be
* auto-dropped if its relation is dropped or if the FK relation is
* dropped. (Auto drop is compatible with our pre-7.3 behavior.)
*/
referenced.classId = RelationRelationId;
referenced.objectId = RelationGetRelid(rel);
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
if (OidIsValid(constrrelid))
{
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 1046,1056 ****
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
}
! /* Depends on the parent trigger, if there is one. */
if (OidIsValid(parentTriggerOid))
{
ObjectAddressSet(referenced, TriggerRelationId, parentTriggerOid);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
}
--- 1040,1054 ----
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
}
! /*
! * If it's a partition trigger, create the partition dependencies.
! */
if (OidIsValid(parentTriggerOid))
{
ObjectAddressSet(referenced, TriggerRelationId, parentTriggerOid);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
! ObjectAddressSet(referenced, RelationRelationId, RelationGetRelid(rel));
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION);
}
}
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 5dea270..ad7cc78 100644
*** a/src/include/catalog/dependency.h
--- b/src/include/catalog/dependency.h
***************
*** 24,87 ****
*
* In all cases, a dependency relationship indicates that the referenced
* object may not be dropped without also dropping the dependent object.
! * However, there are several subflavors:
! *
! * DEPENDENCY_NORMAL ('n'): normal relationship between separately-created
! * objects. The dependent object may be dropped without affecting the
! * referenced object. The referenced object may only be dropped by
! * specifying CASCADE, in which case the dependent object is dropped too.
! * Example: a table column has a normal dependency on its datatype.
! *
! * DEPENDENCY_AUTO ('a'): the dependent object can be dropped separately
! * from the referenced object, and should be automatically dropped
! * (regardless of RESTRICT or CASCADE mode) if the referenced object
! * is dropped.
! * Example: a named constraint on a table is made auto-dependent on
! * the table, so that it will go away if the table is dropped.
! *
! * DEPENDENCY_INTERNAL ('i'): the dependent object was created as part
! * of creation of the referenced object, and is really just a part of
! * its internal implementation. A DROP of the dependent object will be
! * disallowed outright (we'll tell the user to issue a DROP against the
! * referenced object, instead). A DROP of the referenced object will be
! * propagated through to drop the dependent object whether CASCADE is
! * specified or not.
! * Example: a trigger that's created to enforce a foreign-key constraint
! * is made internally dependent on the constraint's pg_constraint entry.
! *
! * DEPENDENCY_INTERNAL_AUTO ('I'): the dependent object was created as
! * part of creation of the referenced object, and is really just a part
! * of its internal implementation. A DROP of the dependent object will
! * be disallowed outright (we'll tell the user to issue a DROP against the
! * referenced object, instead). While a regular internal dependency will
! * prevent the dependent object from being dropped while any such
! * dependencies remain, DEPENDENCY_INTERNAL_AUTO will allow such a drop as
! * long as the object can be found by following any of such dependencies.
! * Example: an index on a partition is made internal-auto-dependent on
! * both the partition itself as well as on the index on the parent
! * partitioned table; so the partition index is dropped together with
! * either the partition it indexes, or with the parent index it is attached
! * to.
!
! * DEPENDENCY_EXTENSION ('e'): the dependent object is a member of the
! * extension that is the referenced object. The dependent object can be
! * dropped only via DROP EXTENSION on the referenced object. Functionally
! * this dependency type acts the same as an internal dependency, but it's
! * kept separate for clarity and to simplify pg_dump.
! *
! * DEPENDENCY_AUTO_EXTENSION ('x'): the dependent object is not a member
! * of the extension that is the referenced object (and so should not be
! * ignored by pg_dump), but cannot function without the extension and
! * should be dropped when the extension itself is. The dependent object
! * may be dropped on its own as well.
! *
! * DEPENDENCY_PIN ('p'): there is no dependent object; this type of entry
! * is a signal that the system itself depends on the referenced object,
! * and so that object must never be deleted. Entries of this type are
! * created only during initdb. The fields for the dependent object
! * contain zeroes.
! *
! * Other dependency flavors may be needed in future.
*/
typedef enum DependencyType
--- 24,31 ----
*
* In all cases, a dependency relationship indicates that the referenced
* object may not be dropped without also dropping the dependent object.
! * However, there are several subflavors; see the description of pg_depend
! * in catalogs.sgml for details.
*/
typedef enum DependencyType
*************** typedef enum DependencyType
*** 89,95 ****
DEPENDENCY_NORMAL = 'n',
DEPENDENCY_AUTO = 'a',
DEPENDENCY_INTERNAL = 'i',
! DEPENDENCY_INTERNAL_AUTO = 'I',
DEPENDENCY_EXTENSION = 'e',
DEPENDENCY_AUTO_EXTENSION = 'x',
DEPENDENCY_PIN = 'p'
--- 33,39 ----
DEPENDENCY_NORMAL = 'n',
DEPENDENCY_AUTO = 'a',
DEPENDENCY_INTERNAL = 'i',
! DEPENDENCY_PARTITION = 'I',
DEPENDENCY_EXTENSION = 'e',
DEPENDENCY_AUTO_EXTENSION = 'x',
DEPENDENCY_PIN = 'p'
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 55a2694..c87bded 100644
*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** extern char *ChooseConstraintName(const
*** 239,245 ****
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
extern void ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId);
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
bool missing_ok, Oid *constraintOid);
--- 239,246 ----
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
extern void ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId,
! Oid childTableId);
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
bool missing_ok, Oid *constraintOid);
On 2019-Feb-08, Tom Lane wrote: > Also, I came across some coding in CloneFkReferencing() that looks fishy > as hell: that function imagines that it can delete an existing trigger > with nothing more than a summary CatalogTupleDelete(). I didn't do > anything about that here, but if it's not broken, I'd like to see an > explanation why not. I added a comment complaining about the lack of > pg_depend cleanup, and there's also the question of whether we don't > need to broadcast a relcache inval for the trigger's table. Oops, this is new code in 0464fdf07f69 (Jan 21st). Unless you object, I'll study a fix for this now, to avoid letting it appear in the minor next week. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 8, 2019 at 8:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > * Partition dependencies are not singletons. They should *always* > > come in pairs, one on the parent partitioned object (partitioned > > index, constraint, trigger, etc) and one on the child partitioned > > table. > > * Partition dependencies are made *in addition to*, not instead of, > > the dependencies the object would normally have. > Here's a patch along these lines. Formality: Unsurprisingly, your patch removes any need for the nbtree patch series to paper-over test failures in indexing.out and triggers.out, without creating any new issues with the regression tests (a tiny number of harmless noise changes are needed -- no change there). I can confirm that your patch fixes remaining regression test breakage of the general variety described in my original e-mail of November 5th. Once you commit this patch, I won't need to include any kind of wonky workaround with each new revision of the patch series. Many thanks for your help here. > * After spending a fair amount of effort on the description of the > dependency types in catalogs.sgml, I decided to just rip out the > duplicative text in dependency.h in favor of a pointer to catalogs.sgml. +1 -- I see no need for repetition. In general, the pg_depend docs seem easier to follow now. > * The core idea of the changes in dependency.c is just to wait till the > end of the entire dependency tree traversal, and then (before we start > actually deleting anything) check to make sure that each targeted > partition-dependent object has been reached via a partition-type > dependency, implying that at least one of its partition owners was > deleted. The existing data structure handles this nicely, we just > need a couple more flag bits for the specific need. (BTW, I experimented > with whether we could handle internal dependencies the same way; but > it didn't work, because of the previously-poorly-documented fact that > an internal dependency is immediately turned into a reverse-direction > recursion. We can't really muck with the dependency traversal order, > or we find ourselves deleting tables before their indices, etc.) > > * I found two different ways in which dependency.c was still producing > traversal-order-sensitive results. One we already understood is that the > first loop in findDependentObjects threw an error for the first internal > or partition dependency it came across; since an object can have both of > those, the results varied. This was fixed by postponing the actual error > report to after the loop and adding an explicit preference order for what > to report. Makes sense. > * The other such issue is a pre-existing bug, which maybe we ought to > back-patch, though I can't recall seeing any field reports that seem > to match it: after recursing to an internal/extension dependency, > we need to ensure that whatever objflags were passed down to our level > get applied to the targetObjects entry for the current object. Otherwise > the final state of the entry can vary depending on traversal order > (since orders in which the current call sees the object already in > targetObjects, or on the stack, would apply the objflags). Hmm. This seems very subtle to me. Perhaps the comment you've added above the new object_address_present_add_flags() call in findDependentObjects() ought to explain the "current object gets marked with objflags" issue first, while only then mentioning the cross-check. The interface that object_address_present_add_flags() presents seems kind of odd to me, though I don't doubt that it makes sense in the wider context of the code. > * The changes outside dependency.c just amount to applying the rules > stated above about how to use partition dependencies. I reverted the > places where v11 had decided that partition dependencies could be > substituted for auto dependencies, and made sure they are always > created in pairs. Makes sense. It's inherently necessary for code outside of dependency.c to get it right. > The main argument against changing these would be the risk that > client code already knows about 'I'. But neither pg_dump nor psql > do, so I judge that probably there's little if anything out there > that is special-casing that dependency type. I lean towards changing these on HEAD, now that it's clear that something very different will be needed for v11. I agree that the "internal_auto" terminology is very unhelpful, and it seems like a good idea to fully acknowledge that certain dependency graphs have qualities that are best thought of as peculiar to partitioning. In the unlikely event that this did end up breaking external client code that relies on the old constants, then the client code was probably subtly wrong to begin with. This may be one of those cases where breaking client code in a noticeable way is desirable. That said, I still don't think that partition_dependency_matches() is all that bad, since the state is still right there in the pg_depend entry. The main drawback of that overall approach is that it obscures a legitimate distinction about dependencies that could be made more apparent to somebody looking through raw pg_depend entries. -- Peter Geoghegan
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-08, Tom Lane wrote:
>> Also, I came across some coding in CloneFkReferencing() that looks fishy
>> as hell: that function imagines that it can delete an existing trigger
>> with nothing more than a summary CatalogTupleDelete(). I didn't do
>> anything about that here, but if it's not broken, I'd like to see an
>> explanation why not. I added a comment complaining about the lack of
>> pg_depend cleanup, and there's also the question of whether we don't
>> need to broadcast a relcache inval for the trigger's table.
> Oops, this is new code in 0464fdf07f69 (Jan 21st). Unless you object,
> I'll study a fix for this now, to avoid letting it appear in the minor
> next week.
+1. The best solution would presumably be to go through the normal
object deletion mechanism; though possibly there's a reason that
won't work given you're already inside some other DDL.
regards, tom lane
On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Feb-08, Tom Lane wrote: > >> Also, I came across some coding in CloneFkReferencing() that looks fishy > >> as hell: that function imagines that it can delete an existing trigger > >> with nothing more than a summary CatalogTupleDelete(). I didn't do > >> anything about that here, but if it's not broken, I'd like to see an > >> explanation why not. I added a comment complaining about the lack of > >> pg_depend cleanup, and there's also the question of whether we don't > >> need to broadcast a relcache inval for the trigger's table. > > > Oops, this is new code in 0464fdf07f69 (Jan 21st). Unless you object, > > I'll study a fix for this now, to avoid letting it appear in the minor > > next week. > > +1. The best solution would presumably be to go through the normal > object deletion mechanism; though possibly there's a reason that > won't work given you're already inside some other DDL. Maybe: - CatalogTupleDelete(trigrel, &trigtup->t_self); + RemoveTriggerById(trgform->oid)? Thanks, Amit
Amit Langote <amitlangote09@gmail.com> writes:
> On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1. The best solution would presumably be to go through the normal
>> object deletion mechanism; though possibly there's a reason that
>> won't work given you're already inside some other DDL.
> Maybe:
> - CatalogTupleDelete(trigrel, &trigtup->t_self);
> + RemoveTriggerById(trgform->oid)?
No, that's still the back end of the deletion machinery, and in particular
it would fail to clean pg_depend entries for the trigger. Going in by the
front door would use performDeletion(). (See deleteOneObject() to get
an idea of what's being possibly missed out here.)
regards, tom lane
On 2019-Feb-09, Tom Lane wrote: > Amit Langote <amitlangote09@gmail.com> writes: > > On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> +1. The best solution would presumably be to go through the normal > >> object deletion mechanism; though possibly there's a reason that > >> won't work given you're already inside some other DDL. > > > Maybe: > > - CatalogTupleDelete(trigrel, &trigtup->t_self); > > + RemoveTriggerById(trgform->oid)? > > No, that's still the back end of the deletion machinery, and in particular > it would fail to clean pg_depend entries for the trigger. Going in by the > front door would use performDeletion(). (See deleteOneObject() to get > an idea of what's being possibly missed out here.) This patch I think does the right thing. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> No, that's still the back end of the deletion machinery, and in particular
>> it would fail to clean pg_depend entries for the trigger. Going in by the
>> front door would use performDeletion(). (See deleteOneObject() to get
>> an idea of what's being possibly missed out here.)
> This patch I think does the right thing.
(squint ...) Don't much like the undocumented deleteDependencyRecordsFor
call; that looks like it's redundant with what deleteOneObject will do.
I think you're doing it to get rid of the INTERNAL dependency so that
deletion won't recurse across that, but why is that a good idea? Needs
a comment at least.
Also, I suspect you might need a second CCI after the performDeletion
call, in case the loop iterates?
regards, tom lane
On 2019-Feb-09, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> No, that's still the back end of the deletion machinery, and in particular
> >> it would fail to clean pg_depend entries for the trigger. Going in by the
> >> front door would use performDeletion(). (See deleteOneObject() to get
> >> an idea of what's being possibly missed out here.)
>
> > This patch I think does the right thing.
>
> (squint ...) Don't much like the undocumented deleteDependencyRecordsFor
> call; that looks like it's redundant with what deleteOneObject will do.
> I think you're doing it to get rid of the INTERNAL dependency so that
> deletion won't recurse across that, but why is that a good idea? Needs
> a comment at least.
Yeah, it's deleting the INTERNAL dependency, because otherwise the
trigger deletion is (correctly) forbidden, since the constraint depends
on it. Perhaps it'd be good to have it be more targetted: make sure it
only deletes that dependency row and not any others that the trigger
might have (though I don't have it shouldn't have any. How could it?) I'd do
that by adding a new function
long
deleteExactDependencyRecords(Oid classId, Oid objectId,
Oid refclassId, Oid refobjectId)
and calling that instead of deleteDependencyRecordsFor.
> Also, I suspect you might need a second CCI after the performDeletion
> call, in case the loop iterates?
Can do.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Feb 10, 2019 at 1:50 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Feb-09, Tom Lane wrote: > > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > > On 2019-Feb-09, Tom Lane wrote: > > >> No, that's still the back end of the deletion machinery, and in particular > > >> it would fail to clean pg_depend entries for the trigger. Going in by the > > >> front door would use performDeletion(). (See deleteOneObject() to get > > >> an idea of what's being possibly missed out here.) > > > > > This patch I think does the right thing. > > > > (squint ...) Don't much like the undocumented deleteDependencyRecordsFor > > call; that looks like it's redundant with what deleteOneObject will do. > > I think you're doing it to get rid of the INTERNAL dependency so that > > deletion won't recurse across that, but why is that a good idea? Needs > > a comment at least. > > Yeah, it's deleting the INTERNAL dependency, because otherwise the > trigger deletion is (correctly) forbidden, since the constraint depends > on it. Perhaps it'd be good to have it be more targetted: make sure it > only deletes that dependency row and not any others that the trigger > might have (though I don't have it shouldn't have any. How could it?) Reading Tom's reply to my email, I wondered if performDeletion won't do more than what the code is already doing (except calling the right trigger deletion function which the current code doesn't), because the trigger in question is an internal trigger without any dependencies (the function it invokes are pinned by the system)? Thanks, Amit
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> I think you're doing it to get rid of the INTERNAL dependency so that
>> deletion won't recurse across that, but why is that a good idea? Needs
>> a comment at least.
> Yeah, it's deleting the INTERNAL dependency, because otherwise the
> trigger deletion is (correctly) forbidden, since the constraint depends
> on it.
Well, the question that's begged here is exactly why it's okay to remove
the trigger and dependency link despite the fact that the constraint needs
it. I suppose the answer is that we'll subsequently insert a new trigger
implementing the same constraint (and internally-linked to it)? That
information is what I'd like to have in the comment.
> Perhaps it'd be good to have it be more targetted: make sure it
> only deletes that dependency row and not any others that the trigger
> might have (though I don't have it shouldn't have any. How could it?) I'd do
> that by adding a new function
I'm not sure that'd be an improvement, especially in light of the
hazard that the trigger might somehow have acquired extension and/or
partition dependencies that'd also cause issues.
regards, tom lane
Amit Langote <amitlangote09@gmail.com> writes:
> Reading Tom's reply to my email, I wondered if performDeletion won't
> do more than what the code is already doing (except calling the right
> trigger deletion function which the current code doesn't), because the
> trigger in question is an internal trigger without any dependencies
> (the function it invokes are pinned by the system)?
A big part of the point here is to not have to have such assumptions
wired into the fk-cloning code. But even if that internal dependency is
the only one the trigger is involved in, there are other steps in
deleteOneObject that shouldn't be ignored. For example, somebody
could've attached a comment to it.
regards, tom lane
On 2019-Feb-09, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Feb-09, Tom Lane wrote: > >> I think you're doing it to get rid of the INTERNAL dependency so that > >> deletion won't recurse across that, but why is that a good idea? Needs > >> a comment at least. > > > Yeah, it's deleting the INTERNAL dependency, because otherwise the > > trigger deletion is (correctly) forbidden, since the constraint depends > > on it. > > Well, the question that's begged here is exactly why it's okay to remove > the trigger and dependency link despite the fact that the constraint needs > it. I suppose the answer is that we'll subsequently insert a new trigger > implementing the same constraint (and internally-linked to it)? That > information is what I'd like to have in the comment. Well, the answer is that the trigger is no longer needed. This is an action trigger, i.e. it's attached to the referenced relation; and the action is making an independent table become a partition. Since the partition is reachable by the action trigger that goes through the parent table, we no longer need the action trigger that goes directly to the partition. Conversely, when we detach a partition, we create an action trigger that wasn't there before. I'll put this in the comment. > > Perhaps it'd be good to have it be more targetted: make sure it > > only deletes that dependency row and not any others that the trigger > > might have (though I don't have it shouldn't have any. How could it?) I'd do > > that by adding a new function > > I'm not sure that'd be an improvement, especially in light of the > hazard that the trigger might somehow have acquired extension and/or > partition dependencies that'd also cause issues. Good point. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Feb 10, 2019 at 2:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Langote <amitlangote09@gmail.com> writes: > > Reading Tom's reply to my email, I wondered if performDeletion won't > > do more than what the code is already doing (except calling the right > > trigger deletion function which the current code doesn't), because the > > trigger in question is an internal trigger without any dependencies > > (the function it invokes are pinned by the system)? > > A big part of the point here is to not have to have such assumptions > wired into the fk-cloning code. But even if that internal dependency is > the only one the trigger is involved in, there are other steps in > deleteOneObject that shouldn't be ignored. For example, somebody > could've attached a comment to it. Okay, I hadn't considered that far. Thanks for explaining. Regards, Amit
On 2019-Feb-09, Alvaro Herrera wrote: > I'll put this in the comment. Attached. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Well, the question that's begged here is exactly why it's okay to remove
>> the trigger and dependency link despite the fact that the constraint needs
>> it. I suppose the answer is that we'll subsequently insert a new trigger
>> implementing the same constraint (and internally-linked to it)? That
>> information is what I'd like to have in the comment.
> Well, the answer is that the trigger is no longer needed. This is an
> action trigger, i.e. it's attached to the referenced relation; and the
> action is making an independent table become a partition. Since the
> partition is reachable by the action trigger that goes through the
> parent table, we no longer need the action trigger that goes directly to
> the partition.
Oh ... then why don't we go ahead and get rid of the constraint entry,
too?
regards, tom lane
On 2019-Feb-09, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Feb-09, Tom Lane wrote: > >> Well, the question that's begged here is exactly why it's okay to > >> remove the trigger and dependency link despite the fact that the > >> constraint needs it. I suppose the answer is that we'll > >> subsequently insert a new trigger implementing the same constraint > >> (and internally-linked to it)? That information is what I'd like > >> to have in the comment. > > > Well, the answer is that the trigger is no longer needed. This is > > an action trigger, i.e. it's attached to the referenced relation; > > and the action is making an independent table become a partition. > > Since the partition is reachable by the action trigger that goes > > through the parent table, we no longer need the action trigger that > > goes directly to the partition. > > Oh ... then why don't we go ahead and get rid of the constraint entry, > too? Because each partition has its own pg_constraint entry. (Otherwise there's no place to put the column numbers into -- they can differ from partition to partition, remember.) The only thing we do is mark it as child of the parent's one. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Oh ... then why don't we go ahead and get rid of the constraint entry,
>> too?
> Because each partition has its own pg_constraint entry. (Otherwise
> there's no place to put the column numbers into -- they can differ from
> partition to partition, remember.) The only thing we do is mark it as
> child of the parent's one.
Uh-huh. And what happens after DETACH PARTITION ... are you going to run
around and recreate these triggers?
regards, tom lane
On 2019-Feb-09, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Feb-09, Tom Lane wrote: > >> Oh ... then why don't we go ahead and get rid of the constraint entry, > >> too? > > > Because each partition has its own pg_constraint entry. (Otherwise > > there's no place to put the column numbers into -- they can differ from > > partition to partition, remember.) The only thing we do is mark it as > > child of the parent's one. > > Uh-huh. And what happens after DETACH PARTITION ... are you going to run > around and recreate these triggers? Yep, that's there too. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Uh-huh. And what happens after DETACH PARTITION ... are you going to run
>> around and recreate these triggers?
> Yep, that's there too.
OK, then I guess it's fine.
regards, tom lane
On 2019-Feb-09, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2019-Feb-09, Tom Lane wrote: > >> Uh-huh. And what happens after DETACH PARTITION ... are you going to run > >> around and recreate these triggers? > > > Yep, that's there too. > > OK, then I guess it's fine. Thanks for verifying; pushed now. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Feb 8, 2019 at 8:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * The other such issue is a pre-existing bug, which maybe we ought to
>> back-patch, though I can't recall seeing any field reports that seem
>> to match it: after recursing to an internal/extension dependency,
>> we need to ensure that whatever objflags were passed down to our level
>> get applied to the targetObjects entry for the current object.
> Hmm. This seems very subtle to me. Perhaps the comment you've added
> above the new object_address_present_add_flags() call in
> findDependentObjects() ought to explain the "current object gets
> marked with objflags" issue first, while only then mentioning the
> cross-check. The interface that object_address_present_add_flags()
> presents seems kind of odd to me, though I don't doubt that it makes
> sense in the wider context of the code.
How about this comment text?
/*
* The current target object should have been added to
* targetObjects while processing the owning object; but it
* probably got only the flag bits associated with the
* dependency we're looking at. We need to add the objflags
* that were passed to this recursion level, too, else we may
* get a bogus failure in reportDependentObjects (if, for
* example, we were called due to a partition dependency).
*
* If somehow the current object didn't get scheduled for
* deletion, bleat. (That would imply that somebody deleted
* this dependency record before the recursion got to it.)
* Another idea would be to reacquire lock on the current
* object and resume trying to delete it, but it seems not
* worth dealing with the race conditions inherent in that.
*/
>> [ invent separate primary and secondary partition dependencies? ]
> I lean towards changing these on HEAD, ...
Me too.
> ... now that it's clear that
> something very different will be needed for v11.
Just to be be clear, my inclination is to do nothing about this in v11.
It's not apparent to me that any fix is possible given the v11 dependency
data, at least not without downsides that'd likely outweigh the upsides.
We've not seen field complaints about these problems.
> That said, I still don't think that partition_dependency_matches() is
> all that bad, since the state is still right there in the pg_depend
> entry. The main drawback of that overall approach is that it obscures
> a legitimate distinction about dependencies that could be made more
> apparent to somebody looking through raw pg_depend entries.
The thing I don't like about it is that it's not very hard to foresee
cases where the approach will fail to uniquely resolve the primary
dependency. Making the original creator of the dependencies specify
which object is the primary owner seems a lot more future-proof.
regards, tom lane
I wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
>>> [ invent separate primary and secondary partition dependencies? ]
>> I lean towards changing these on HEAD, ...
> Me too.
Here's a version of the patch that does it that way.
regards, tom lane
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 6dd0700..ae69a9e 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
*************** SCRAM-SHA-256$<replaceable><iteration
*** 2970,2976 ****
referenced object, and should be automatically dropped
(regardless of <literal>RESTRICT</literal> or <literal>CASCADE</literal>
mode) if the referenced object is dropped. Example: a named
! constraint on a table is made autodependent on the table, so
that it will go away if the table is dropped.
</para>
</listitem>
--- 2970,2976 ----
referenced object, and should be automatically dropped
(regardless of <literal>RESTRICT</literal> or <literal>CASCADE</literal>
mode) if the referenced object is dropped. Example: a named
! constraint on a table is made auto-dependent on the table, so
that it will go away if the table is dropped.
</para>
</listitem>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 2982,3019 ****
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A <command>DROP</command> of the dependent object
! will be disallowed outright (we'll tell the user to issue a
! <command>DROP</command> against the referenced object, instead). A
! <command>DROP</command> of the referenced object will be propagated
! through to drop the dependent object whether
! <command>CASCADE</command> is specified or not. Example: a trigger
! that's created to enforce a foreign-key constraint is made
! internally dependent on the constraint's
! <structname>pg_constraint</structname> entry.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><symbol>DEPENDENCY_INTERNAL_AUTO</symbol> (<literal>I</literal>)</term>
<listitem>
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A <command>DROP</command> of the dependent object
! will be disallowed outright (we'll tell the user to issue a
! <command>DROP</command> against the referenced object, instead).
! While a regular internal dependency will prevent
! the dependent object from being dropped while any such dependencies
! remain, <literal>DEPENDENCY_INTERNAL_AUTO</literal> will allow such
! a drop as long as the object can be found by following any of such
! dependencies.
! Example: an index on a partition is made internal-auto-dependent on
! both the partition itself as well as on the index on the parent
! partitioned table; so the partition index is dropped together with
! either the partition it indexes, or with the parent index it is
! attached to.
</para>
</listitem>
</varlistentry>
--- 2982,3052 ----
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation. A direct <command>DROP</command> of the dependent
! object will be disallowed outright (we'll tell the user to issue
! a <command>DROP</command> against the referenced object, instead).
! A <command>DROP</command> of the referenced object will result in
! automatically dropping the dependent object
! whether <literal>CASCADE</literal> is specified or not. If the
! dependent object is reached due to a dependency on some other object,
! the drop is converted to a drop of the referenced object, so
! that <literal>NORMAL</literal> and <literal>AUTO</literal>
! dependencies of the dependent object behave much like they were
! dependencies of the referenced object.
! Example: a view's <literal>ON SELECT</literal> rule is made
! internally dependent on the view, preventing it from being dropped
! while the view remains. Dependencies of the rule (such as tables it
! refers to) act as if they were dependencies of the view.
</para>
</listitem>
</varlistentry>
<varlistentry>
! <term><symbol>DEPENDENCY_PARTITION_PRI</symbol> (<literal>P</literal>)</term>
<listitem>
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
! implementation; however, unlike <literal>INTERNAL</literal>,
! there is more than one such referenced object. The dependent object
! must not be dropped unless at least one of these referenced objects
! is dropped; if any one is, the dependent object should be dropped
! whether or not <literal>CASCADE</literal> is specified. Also
! unlike <literal>INTERNAL</literal>, a drop of some other object
! that the dependent object depends on does not result in automatic
! deletion of any partition-referenced object. Hence, if the drop
! does not cascade to at least one of these objects via some other
! path, it will be refused. (In most cases, the dependent object
! shares all its non-partition dependencies with at least one
! partition-referenced object, so that this restriction does not
! result in blocking any cascaded delete.)
! Note that partition dependencies are made in addition to, not
! instead of, any dependencies the object would normally have. This
! simplifies <command>ATTACH/DETACH PARTITION</command> operations:
! the partition dependencies need only be added or removed.
! Example: a child partitioned index is made partition-dependent
! on both the partition table it is on and the parent partitioned
! index, so that it goes away if either of those is dropped, but
! not otherwise.
! </para>
! </listitem>
! </varlistentry>
!
! <varlistentry>
! <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term>
! <listitem>
! <para>
! A <quote>secondary</quote> partition dependency acts identically to
! a primary one, except that the primary dependency is preferentially
! referenced in error messages. An object should have at most one
! primary partition dependency, but there could perhaps be multiple
! secondary dependencies.
! Example: actually, we'll set up a child partitioned index with the
! parent partitioned index as primary partition dependency and the
! partition table as secondary partition dependency. In this way,
! if the user tries to drop the child partitioned index, the error
! message will suggest dropping the parent partitioned index instead
! (not the table).
</para>
</listitem>
</varlistentry>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 3026,3034 ****
the referenced object (see
<link linkend="catalog-pg-extension"><structname>pg_extension</structname></link>).
The dependent object can be dropped only via
! <command>DROP EXTENSION</command> on the referenced object. Functionally
! this dependency type acts the same as an internal dependency, but
! it's kept separate for clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
--- 3059,3068 ----
the referenced object (see
<link linkend="catalog-pg-extension"><structname>pg_extension</structname></link>).
The dependent object can be dropped only via
! <command>DROP EXTENSION</command> on the referenced object.
! Functionally this dependency type acts the same as
! an <literal>INTERNAL</literal> dependency, but it's kept separate for
! clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
*************** SCRAM-SHA-256$<replaceable><iteration
*** 3038,3047 ****
<listitem>
<para>
The dependent object is not a member of the extension that is the
! referenced object (and so should not be ignored by pg_dump), but
! cannot function without it and should be dropped when the
! extension itself is. The dependent object may be dropped on its
! own as well.
</para>
</listitem>
</varlistentry>
--- 3072,3084 ----
<listitem>
<para>
The dependent object is not a member of the extension that is the
! referenced object (and so it should not be ignored
! by <application>pg_dump</application>), but it cannot function
! without the extension and should be auto-dropped if the extension is.
! The dependent object may be dropped on its own as well.
! Functionally this dependency type acts the same as
! an <literal>AUTO</literal> dependency, but it's kept separate for
! clarity and to simplify <application>pg_dump</application>.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2c54895..e001334 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** typedef struct
*** 99,107 ****
#define DEPFLAG_NORMAL 0x0002 /* reached via normal dependency */
#define DEPFLAG_AUTO 0x0004 /* reached via auto dependency */
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
! #define DEPFLAG_EXTENSION 0x0010 /* reached via extension dependency */
! #define DEPFLAG_REVERSE 0x0020 /* reverse internal/extension link */
! #define DEPFLAG_SUBOBJECT 0x0040 /* subobject of another deletable object */
/* expansible list of ObjectAddresses */
--- 99,109 ----
#define DEPFLAG_NORMAL 0x0002 /* reached via normal dependency */
#define DEPFLAG_AUTO 0x0004 /* reached via auto dependency */
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
! #define DEPFLAG_PARTITION 0x0010 /* reached via partition dependency */
! #define DEPFLAG_EXTENSION 0x0020 /* reached via extension dependency */
! #define DEPFLAG_REVERSE 0x0040 /* reverse internal/extension link */
! #define DEPFLAG_IS_PART 0x0080 /* has a partition dependency */
! #define DEPFLAG_SUBOBJECT 0x0100 /* subobject of another deletable object */
/* expansible list of ObjectAddresses */
*************** findDependentObjects(const ObjectAddress
*** 478,483 ****
--- 480,487 ----
SysScanDesc scan;
HeapTuple tup;
ObjectAddress otherObject;
+ ObjectAddress owningObject;
+ ObjectAddress partitionObject;
ObjectAddressAndFlags *dependentObjects;
int numDependentObjects;
int maxDependentObjects;
*************** findDependentObjects(const ObjectAddress
*** 547,552 ****
--- 551,560 ----
scan = systable_beginscan(*depRel, DependDependerIndexId, true,
NULL, nkeys, key);
+ /* initialize variables that loop may fill */
+ memset(&owningObject, 0, sizeof(owningObject));
+ memset(&partitionObject, 0, sizeof(partitionObject));
+
while (HeapTupleIsValid(tup = systable_getnext(scan)))
{
Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup);
*************** findDependentObjects(const ObjectAddress
*** 591,614 ****
/* FALL THRU */
case DEPENDENCY_INTERNAL:
- case DEPENDENCY_INTERNAL_AUTO:
/*
* This object is part of the internal implementation of
* another object, or is part of the extension that is the
* other object. We have three cases:
*
! * 1. At the outermost recursion level, disallow the DROP. (We
! * just ereport here, rather than proceeding, since no other
! * dependencies are likely to be interesting.) However, if
! * the owning object is listed in pendingObjects, just release
! * the caller's lock and return; we'll eventually complete the
! * DROP when we reach that entry in the pending list.
*/
if (stack == NULL)
{
- char *otherObjDesc;
-
if (pendingObjects &&
object_address_present(&otherObject, pendingObjects))
{
--- 599,624 ----
/* FALL THRU */
case DEPENDENCY_INTERNAL:
/*
* This object is part of the internal implementation of
* another object, or is part of the extension that is the
* other object. We have three cases:
*
! * 1. At the outermost recursion level, we must disallow the
! * DROP. However, if the owning object is listed in
! * pendingObjects, just release the caller's lock and return;
! * we'll eventually complete the DROP when we reach that entry
! * in the pending list.
! *
! * Note: the above statement is true only if this pg_depend
! * entry still exists by then; in principle, therefore, we
! * could miss deleting an item the user told us to delete.
! * However, no inconsistency can result: since we're at outer
! * level, there is no object depending on this one.
*/
if (stack == NULL)
{
if (pendingObjects &&
object_address_present(&otherObject, pendingObjects))
{
*************** findDependentObjects(const ObjectAddress
*** 617,630 ****
ReleaseDeletionLock(object);
return;
}
! otherObjDesc = getObjectDescription(&otherObject);
! ereport(ERROR,
! (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
! errmsg("cannot drop %s because %s requires it",
! getObjectDescription(object),
! otherObjDesc),
! errhint("You can drop %s instead.",
! otherObjDesc)));
}
/*
--- 627,643 ----
ReleaseDeletionLock(object);
return;
}
!
! /*
! * We postpone actually issuing the error message until
! * after this loop, so that we can make the behavior
! * independent of the ordering of pg_depend entries, at
! * least so long as there's just one INTERNAL + EXTENSION
! * dependency. (If there's more, we'll complain about a
! * random one of them.)
! */
! owningObject = otherObject;
! break;
}
/*
*************** findDependentObjects(const ObjectAddress
*** 643,656 ****
* transform this deletion request into a delete of this
* owning object.
*
- * For INTERNAL_AUTO dependencies, we don't enforce this; in
- * other words, we don't follow the links back to the owning
- * object.
- */
- if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO)
- break;
-
- /*
* First, release caller's lock on this object and get
* deletion lock on the owning object. (We must release
* caller's lock to avoid deadlock against a concurrent
--- 656,661 ----
*************** findDependentObjects(const ObjectAddress
*** 673,678 ****
--- 678,690 ----
}
/*
+ * One way or the other, we're done with the scan; might as
+ * well close it down before recursing, to reduce peak
+ * resource consumption.
+ */
+ systable_endscan(scan);
+
+ /*
* Okay, recurse to the owning object instead of proceeding.
*
* We do not need to stack the current object; we want the
*************** findDependentObjects(const ObjectAddress
*** 690,699 ****
targetObjects,
pendingObjects,
depRel);
/* And we're done here. */
- systable_endscan(scan);
return;
case DEPENDENCY_PIN:
/*
--- 702,767 ----
targetObjects,
pendingObjects,
depRel);
+
+ /*
+ * The current target object should have been added to
+ * targetObjects while processing the owning object; but it
+ * probably got only the flag bits associated with the
+ * dependency we're looking at. We need to add the objflags
+ * that were passed to this recursion level, too, else we may
+ * get a bogus failure in reportDependentObjects (if, for
+ * example, we were called due to a partition dependency).
+ *
+ * If somehow the current object didn't get scheduled for
+ * deletion, bleat. (That would imply that somebody deleted
+ * this dependency record before the recursion got to it.)
+ * Another idea would be to reacquire lock on the current
+ * object and resume trying to delete it, but it seems not
+ * worth dealing with the race conditions inherent in that.
+ */
+ if (!object_address_present_add_flags(object, objflags,
+ targetObjects))
+ elog(ERROR, "deletion of owning object %s failed to delete %s",
+ getObjectDescription(&otherObject),
+ getObjectDescription(object));
+
/* And we're done here. */
return;
+ case DEPENDENCY_PARTITION_PRI:
+
+ /*
+ * Remember that this object has a partition-type dependency.
+ * After the dependency scan, we'll complain if we didn't find
+ * a reason to delete one of its partition dependencies.
+ */
+ objflags |= DEPFLAG_IS_PART;
+
+ /*
+ * Also remember the primary partition owner, for error
+ * messages. If there are multiple primary owners (which
+ * there should not be), we'll report a random one of them.
+ */
+ partitionObject = otherObject;
+ break;
+
+ case DEPENDENCY_PARTITION_SEC:
+
+ /*
+ * Only use secondary partition owners in error messages if we
+ * find no primary owner (which probably shouldn't happen).
+ */
+ if (!(objflags & DEPFLAG_IS_PART))
+ partitionObject = otherObject;
+
+ /*
+ * Remember that this object has a partition-type dependency.
+ * After the dependency scan, we'll complain if we didn't find
+ * a reason to delete one of its partition dependencies.
+ */
+ objflags |= DEPFLAG_IS_PART;
+ break;
+
case DEPENDENCY_PIN:
/*
*************** findDependentObjects(const ObjectAddress
*** 713,718 ****
--- 781,808 ----
systable_endscan(scan);
/*
+ * If we found an INTERNAL or EXTENSION dependency when we're at outer
+ * level, complain about it now. If we also found a PARTITION dependency,
+ * we prefer to report the PARTITION dependency. This is arbitrary but
+ * seems to be more useful in practice.
+ */
+ if (OidIsValid(owningObject.classId))
+ {
+ char *otherObjDesc;
+
+ if (OidIsValid(partitionObject.classId))
+ otherObjDesc = getObjectDescription(&partitionObject);
+ else
+ otherObjDesc = getObjectDescription(&owningObject);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s requires it",
+ getObjectDescription(object), otherObjDesc),
+ errhint("You can drop %s instead.", otherObjDesc)));
+ }
+
+ /*
* Next, identify all objects that directly depend on the current object.
* To ensure predictable deletion order, we collect them up in
* dependentObjects and sort the list before actually recursing. (The
*************** findDependentObjects(const ObjectAddress
*** 789,798 ****
case DEPENDENCY_AUTO_EXTENSION:
subflags = DEPFLAG_AUTO;
break;
- case DEPENDENCY_INTERNAL_AUTO:
case DEPENDENCY_INTERNAL:
subflags = DEPFLAG_INTERNAL;
break;
case DEPENDENCY_EXTENSION:
subflags = DEPFLAG_EXTENSION;
break;
--- 879,891 ----
case DEPENDENCY_AUTO_EXTENSION:
subflags = DEPFLAG_AUTO;
break;
case DEPENDENCY_INTERNAL:
subflags = DEPFLAG_INTERNAL;
break;
+ case DEPENDENCY_PARTITION_PRI:
+ case DEPENDENCY_PARTITION_SEC:
+ subflags = DEPFLAG_PARTITION;
+ break;
case DEPENDENCY_EXTENSION:
subflags = DEPFLAG_EXTENSION;
break;
*************** findDependentObjects(const ObjectAddress
*** 868,877 ****
/*
* Finally, we can add the target object to targetObjects. Be careful to
* include any flags that were passed back down to us from inner recursion
! * levels.
*/
extra.flags = mystack.flags;
! if (stack)
extra.dependee = *stack->object;
else
memset(&extra.dependee, 0, sizeof(extra.dependee));
--- 961,975 ----
/*
* Finally, we can add the target object to targetObjects. Be careful to
* include any flags that were passed back down to us from inner recursion
! * levels. Record the "dependee" as being either the most important
! * partition owner if there is one, else the object we recursed from, if
! * any. (The logic in reportDependentObjects() is such that it can only
! * need one of those objects.)
*/
extra.flags = mystack.flags;
! if (extra.flags & DEPFLAG_IS_PART)
! extra.dependee = partitionObject;
! else if (stack)
extra.dependee = *stack->object;
else
memset(&extra.dependee, 0, sizeof(extra.dependee));
*************** reportDependentObjects(const ObjectAddre
*** 906,913 ****
int i;
/*
* If no error is to be thrown, and the msglevel is too low to be shown to
! * either client or server log, there's no need to do any of the work.
*
* Note: this code doesn't know all there is to be known about elog
* levels, but it works for NOTICE and DEBUG2, which are the only values
--- 1004,1040 ----
int i;
/*
+ * If we need to delete any partition-dependent objects, make sure that
+ * we're deleting at least one of their partition dependencies, too. That
+ * can be detected by checking that we reached them by a PARTITION
+ * dependency at some point.
+ *
+ * We just report the first such object, as in most cases the only way to
+ * trigger this complaint is to explicitly try to delete one partition of
+ * a partitioned object.
+ */
+ for (i = 0; i < targetObjects->numrefs; i++)
+ {
+ const ObjectAddressExtra *extra = &targetObjects->extras[i];
+
+ if ((extra->flags & DEPFLAG_IS_PART) &&
+ !(extra->flags & DEPFLAG_PARTITION))
+ {
+ const ObjectAddress *object = &targetObjects->refs[i];
+ char *otherObjDesc = getObjectDescription(&extra->dependee);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s requires it",
+ getObjectDescription(object), otherObjDesc),
+ errhint("You can drop %s instead.", otherObjDesc)));
+ }
+ }
+
+ /*
* If no error is to be thrown, and the msglevel is too low to be shown to
! * either client or server log, there's no need to do any of the rest of
! * the work.
*
* Note: this code doesn't know all there is to be known about elog
* levels, but it works for NOTICE and DEBUG2, which are the only values
*************** reportDependentObjects(const ObjectAddre
*** 951,961 ****
/*
* If, at any stage of the recursive search, we reached the object via
! * an AUTO, INTERNAL, or EXTENSION dependency, then it's okay to
! * delete it even in RESTRICT mode.
*/
if (extra->flags & (DEPFLAG_AUTO |
DEPFLAG_INTERNAL |
DEPFLAG_EXTENSION))
{
/*
--- 1078,1089 ----
/*
* If, at any stage of the recursive search, we reached the object via
! * an AUTO, INTERNAL, PARTITION, or EXTENSION dependency, then it's
! * okay to delete it even in RESTRICT mode.
*/
if (extra->flags & (DEPFLAG_AUTO |
DEPFLAG_INTERNAL |
+ DEPFLAG_PARTITION |
DEPFLAG_EXTENSION))
{
/*
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index faf6956..d16c3d0 100644
*** a/src/backend/catalog/index.c
--- b/src/backend/catalog/index.c
*************** index_create(Relation heapRelation,
*** 1041,1049 ****
else
{
bool have_simple_col = false;
- DependencyType deptype;
-
- deptype = OidIsValid(parentIndexRelid) ? DEPENDENCY_INTERNAL_AUTO : DEPENDENCY_AUTO;
/* Create auto dependencies on simply-referenced columns */
for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
--- 1041,1046 ----
*************** index_create(Relation heapRelation,
*** 1054,1060 ****
referenced.objectId = heapRelationId;
referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
! recordDependencyOn(&myself, &referenced, deptype);
have_simple_col = true;
}
--- 1051,1057 ----
referenced.objectId = heapRelationId;
referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
have_simple_col = true;
}
*************** index_create(Relation heapRelation,
*** 1072,1089 ****
referenced.objectId = heapRelationId;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, deptype);
}
}
! /* Store dependency on parent index, if any */
if (OidIsValid(parentIndexRelid))
{
referenced.classId = RelationRelationId;
referenced.objectId = parentIndexRelid;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
/* Store dependency on collations */
--- 1069,1097 ----
referenced.objectId = heapRelationId;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
}
}
! /*
! * If this is an index partition, create partition dependencies on
! * both the parent index and the table. (Note: these must be *in
! * addition to*, not instead of, all other dependencies. Otherwise
! * we'll be short some dependencies after DETACH PARTITION.)
! */
if (OidIsValid(parentIndexRelid))
{
referenced.classId = RelationRelationId;
referenced.objectId = parentIndexRelid;
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_PRI);
!
! referenced.classId = RelationRelationId;
! referenced.objectId = heapRelationId;
! referenced.objectSubId = 0;
!
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_SEC);
}
/* Store dependency on collations */
*************** index_constraint_create(Relation heapRel
*** 1342,1356 ****
recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
/*
! * Also, if this is a constraint on a partition, mark it as depending on
! * the constraint in the parent.
*/
if (OidIsValid(parentConstraintId))
{
! ObjectAddress parentConstr;
!
! ObjectAddressSet(parentConstr, ConstraintRelationId, parentConstraintId);
! recordDependencyOn(&referenced, &parentConstr, DEPENDENCY_INTERNAL_AUTO);
}
/*
--- 1350,1366 ----
recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL);
/*
! * Also, if this is a constraint on a partition, give it partition-type
! * dependencies on the parent constraint as well as the table.
*/
if (OidIsValid(parentConstraintId))
{
! ObjectAddressSet(myself, ConstraintRelationId, conOid);
! ObjectAddressSet(referenced, ConstraintRelationId, parentConstraintId);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_PRI);
! ObjectAddressSet(referenced, RelationRelationId,
! RelationGetRelid(heapRelation));
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_SEC);
}
/*
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 698b493..ad836e0 100644
*** a/src/backend/catalog/pg_constraint.c
--- b/src/backend/catalog/pg_constraint.c
*************** AlterConstraintNamespaces(Oid ownerId, O
*** 760,772 ****
/*
* ConstraintSetParentConstraint
! * Set a partition's constraint as child of its parent table's
*
* This updates the constraint's pg_constraint row to show it as inherited, and
! * add a dependency to the parent so that it cannot be removed on its own.
*/
void
! ConstraintSetParentConstraint(Oid childConstrId, Oid parentConstrId)
{
Relation constrRel;
Form_pg_constraint constrForm;
--- 760,776 ----
/*
* ConstraintSetParentConstraint
! * Set a partition's constraint as child of its parent constraint,
! * or remove the linkage if parentConstrId is InvalidOid.
*
* This updates the constraint's pg_constraint row to show it as inherited, and
! * adds PARTITION dependencies to prevent the constraint from being deleted
! * on its own. Alternatively, reverse that.
*/
void
! ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId,
! Oid childTableId)
{
Relation constrRel;
Form_pg_constraint constrForm;
*************** ConstraintSetParentConstraint(Oid childC
*** 795,804 ****
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
- ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId);
ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
else
{
--- 799,811 ----
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
ObjectAddressSet(depender, ConstraintRelationId, childConstrId);
! ObjectAddressSet(referenced, ConstraintRelationId, parentConstrId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_PRI);
!
! ObjectAddressSet(referenced, RelationRelationId, childTableId);
! recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_SEC);
}
else
{
*************** ConstraintSetParentConstraint(Oid childC
*** 809,818 ****
/* Make sure there's no further inheritance. */
Assert(constrForm->coninhcount == 0);
deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
ConstraintRelationId,
! DEPENDENCY_INTERNAL_AUTO);
! CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
}
ReleaseSysCache(tuple);
--- 816,829 ----
/* Make sure there's no further inheritance. */
Assert(constrForm->coninhcount == 0);
+ CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
+
deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
ConstraintRelationId,
! DEPENDENCY_PARTITION_PRI);
! deleteDependencyRecordsForClass(ConstraintRelationId, childConstrId,
! RelationRelationId,
! DEPENDENCY_PARTITION_SEC);
}
ReleaseSysCache(tuple);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index bd85099..7352b9e 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
*************** DefineIndex(Oid relationId,
*** 971,977 ****
IndexSetParentIndex(cldidx, indexRelationId);
if (createdConstraintId != InvalidOid)
ConstraintSetParentConstraint(cldConstrOid,
! createdConstraintId);
if (!cldidx->rd_index->indisvalid)
invalidate_parent = true;
--- 971,978 ----
IndexSetParentIndex(cldidx, indexRelationId);
if (createdConstraintId != InvalidOid)
ConstraintSetParentConstraint(cldConstrOid,
! createdConstraintId,
! childRelid);
if (!cldidx->rd_index->indisvalid)
invalidate_parent = true;
*************** IndexSetParentIndex(Relation partitionId
*** 2622,2656 ****
if (fix_dependencies)
{
- ObjectAddress partIdx;
-
/*
! * Insert/delete pg_depend rows. If setting a parent, add an
! * INTERNAL_AUTO dependency to the parent index; if making standalone,
! * remove all existing rows and put back the regular dependency on the
! * table.
*/
- ObjectAddressSet(partIdx, RelationRelationId, partRelid);
-
if (OidIsValid(parentOid))
{
ObjectAddress parentIdx;
ObjectAddressSet(parentIdx, RelationRelationId, parentOid);
! recordDependencyOn(&partIdx, &parentIdx, DEPENDENCY_INTERNAL_AUTO);
}
else
{
- ObjectAddress partitionTbl;
-
- ObjectAddressSet(partitionTbl, RelationRelationId,
- partitionIdx->rd_index->indrelid);
-
deleteDependencyRecordsForClass(RelationRelationId, partRelid,
RelationRelationId,
! DEPENDENCY_INTERNAL_AUTO);
!
! recordDependencyOn(&partIdx, &partitionTbl, DEPENDENCY_AUTO);
}
/* make our updates visible */
--- 2623,2656 ----
if (fix_dependencies)
{
/*
! * Insert/delete pg_depend rows. If setting a parent, add PARTITION
! * dependencies on the parent index and the table; if removing a
! * parent, delete PARTITION dependencies.
*/
if (OidIsValid(parentOid))
{
+ ObjectAddress partIdx;
ObjectAddress parentIdx;
+ ObjectAddress partitionTbl;
+ ObjectAddressSet(partIdx, RelationRelationId, partRelid);
ObjectAddressSet(parentIdx, RelationRelationId, parentOid);
! ObjectAddressSet(partitionTbl, RelationRelationId,
! partitionIdx->rd_index->indrelid);
! recordDependencyOn(&partIdx, &parentIdx,
! DEPENDENCY_PARTITION_PRI);
! recordDependencyOn(&partIdx, &partitionTbl,
! DEPENDENCY_PARTITION_SEC);
}
else
{
deleteDependencyRecordsForClass(RelationRelationId, partRelid,
RelationRelationId,
! DEPENDENCY_PARTITION_PRI);
! deleteDependencyRecordsForClass(RelationRelationId, partRelid,
! RelationRelationId,
! DEPENDENCY_PARTITION_SEC);
}
/* make our updates visible */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b66d194..715c6a2 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** CloneFkReferencing(Relation pg_constrain
*** 7825,7831 ****
bool attach_it;
Oid constrOid;
ObjectAddress parentAddr,
! childAddr;
ListCell *cell;
int i;
--- 7825,7832 ----
bool attach_it;
Oid constrOid;
ObjectAddress parentAddr,
! childAddr,
! childTableAddr;
ListCell *cell;
int i;
*************** CloneFkReferencing(Relation pg_constrain
*** 7966,7972 ****
systable_endscan(scan);
table_close(trigrel, RowExclusiveLock);
! ConstraintSetParentConstraint(fk->conoid, parentConstrOid);
CommandCounterIncrement();
attach_it = true;
break;
--- 7967,7974 ----
systable_endscan(scan);
table_close(trigrel, RowExclusiveLock);
! ConstraintSetParentConstraint(fk->conoid, parentConstrOid,
! RelationGetRelid(partRel));
CommandCounterIncrement();
attach_it = true;
break;
*************** CloneFkReferencing(Relation pg_constrain
*** 8013,8020 ****
1, false, true);
subclone = lappend_oid(subclone, constrOid);
ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
! recordDependencyOn(&childAddr, &parentAddr, DEPENDENCY_INTERNAL_AUTO);
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
--- 8015,8028 ----
1, false, true);
subclone = lappend_oid(subclone, constrOid);
+ /* Set up partition dependencies for the new constraint */
ObjectAddressSet(childAddr, ConstraintRelationId, constrOid);
! recordDependencyOn(&childAddr, &parentAddr,
! DEPENDENCY_PARTITION_PRI);
! ObjectAddressSet(childTableAddr, RelationRelationId,
! RelationGetRelid(partRel));
! recordDependencyOn(&childAddr, &childTableAddr,
! DEPENDENCY_PARTITION_SEC);
fkconstraint = makeNode(Constraint);
/* for now this is all we need */
*************** AttachPartitionEnsureIndexes(Relation re
*** 14893,14899 ****
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrOid, constraintOid);
update_relispartition(NULL, cldIdxId, true);
found = true;
break;
--- 14901,14908 ----
/* bingo. */
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrOid, constraintOid,
! RelationGetRelid(attachrel));
update_relispartition(NULL, cldIdxId, true);
found = true;
break;
*************** ATExecDetachPartition(Relation rel, Rang
*** 15151,15157 ****
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);
if (OidIsValid(constrOid))
! ConstraintSetParentConstraint(constrOid, InvalidOid);
index_close(idx, NoLock);
}
--- 15160,15166 ----
constrOid = get_relation_idx_constraint_oid(RelationGetRelid(partRel),
idxid);
if (OidIsValid(constrOid))
! ConstraintSetParentConstraint(constrOid, InvalidOid, InvalidOid);
index_close(idx, NoLock);
}
*************** ATExecDetachPartition(Relation rel, Rang
*** 15183,15189 ****
}
/* unset conparentid and adjust conislocal, coninhcount, etc. */
! ConstraintSetParentConstraint(fk->conoid, InvalidOid);
/*
* Make the action triggers on the referenced relation. When this was
--- 15192,15198 ----
}
/* unset conparentid and adjust conislocal, coninhcount, etc. */
! ConstraintSetParentConstraint(fk->conoid, InvalidOid, InvalidOid);
/*
* Make the action triggers on the referenced relation. When this was
*************** ATExecAttachPartitionIdx(List **wqueue,
*** 15419,15425 ****
/* All good -- do it */
IndexSetParentIndex(partIdx, RelationGetRelid(parentIdx));
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrId, constraintOid);
update_relispartition(NULL, partIdxId, true);
pfree(attmap);
--- 15428,15435 ----
/* All good -- do it */
IndexSetParentIndex(partIdx, RelationGetRelid(parentIdx));
if (OidIsValid(constraintOid))
! ConstraintSetParentConstraint(cldConstrId, constraintOid,
! RelationGetRelid(partTbl));
update_relispartition(NULL, partIdxId, true);
pfree(attmap);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 0b245a6..409bee2 100644
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 1012,1028 ****
* User CREATE TRIGGER, so place dependencies. We make trigger be
* auto-dropped if its relation is dropped or if the FK relation is
* dropped. (Auto drop is compatible with our pre-7.3 behavior.)
- *
- * Exception: if this trigger comes from a parent partitioned table,
- * then it's not separately drop-able, but goes away if the partition
- * does.
*/
referenced.classId = RelationRelationId;
referenced.objectId = RelationGetRelid(rel);
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, OidIsValid(parentTriggerOid) ?
! DEPENDENCY_INTERNAL_AUTO :
! DEPENDENCY_AUTO);
if (OidIsValid(constrrelid))
{
--- 1012,1022 ----
* User CREATE TRIGGER, so place dependencies. We make trigger be
* auto-dropped if its relation is dropped or if the FK relation is
* dropped. (Auto drop is compatible with our pre-7.3 behavior.)
*/
referenced.classId = RelationRelationId;
referenced.objectId = RelationGetRelid(rel);
referenced.objectSubId = 0;
! recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
if (OidIsValid(constrrelid))
{
*************** CreateTrigger(CreateTrigStmt *stmt, cons
*** 1046,1056 ****
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
}
! /* Depends on the parent trigger, if there is one. */
if (OidIsValid(parentTriggerOid))
{
ObjectAddressSet(referenced, TriggerRelationId, parentTriggerOid);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_INTERNAL_AUTO);
}
}
--- 1040,1054 ----
recordDependencyOn(&referenced, &myself, DEPENDENCY_INTERNAL);
}
! /*
! * If it's a partition trigger, create the partition dependencies.
! */
if (OidIsValid(parentTriggerOid))
{
ObjectAddressSet(referenced, TriggerRelationId, parentTriggerOid);
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_PRI);
! ObjectAddressSet(referenced, RelationRelationId, RelationGetRelid(rel));
! recordDependencyOn(&myself, &referenced, DEPENDENCY_PARTITION_SEC);
}
}
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 5dea270..b235a23 100644
*** a/src/include/catalog/dependency.h
--- b/src/include/catalog/dependency.h
***************
*** 24,87 ****
*
* In all cases, a dependency relationship indicates that the referenced
* object may not be dropped without also dropping the dependent object.
! * However, there are several subflavors:
! *
! * DEPENDENCY_NORMAL ('n'): normal relationship between separately-created
! * objects. The dependent object may be dropped without affecting the
! * referenced object. The referenced object may only be dropped by
! * specifying CASCADE, in which case the dependent object is dropped too.
! * Example: a table column has a normal dependency on its datatype.
! *
! * DEPENDENCY_AUTO ('a'): the dependent object can be dropped separately
! * from the referenced object, and should be automatically dropped
! * (regardless of RESTRICT or CASCADE mode) if the referenced object
! * is dropped.
! * Example: a named constraint on a table is made auto-dependent on
! * the table, so that it will go away if the table is dropped.
! *
! * DEPENDENCY_INTERNAL ('i'): the dependent object was created as part
! * of creation of the referenced object, and is really just a part of
! * its internal implementation. A DROP of the dependent object will be
! * disallowed outright (we'll tell the user to issue a DROP against the
! * referenced object, instead). A DROP of the referenced object will be
! * propagated through to drop the dependent object whether CASCADE is
! * specified or not.
! * Example: a trigger that's created to enforce a foreign-key constraint
! * is made internally dependent on the constraint's pg_constraint entry.
! *
! * DEPENDENCY_INTERNAL_AUTO ('I'): the dependent object was created as
! * part of creation of the referenced object, and is really just a part
! * of its internal implementation. A DROP of the dependent object will
! * be disallowed outright (we'll tell the user to issue a DROP against the
! * referenced object, instead). While a regular internal dependency will
! * prevent the dependent object from being dropped while any such
! * dependencies remain, DEPENDENCY_INTERNAL_AUTO will allow such a drop as
! * long as the object can be found by following any of such dependencies.
! * Example: an index on a partition is made internal-auto-dependent on
! * both the partition itself as well as on the index on the parent
! * partitioned table; so the partition index is dropped together with
! * either the partition it indexes, or with the parent index it is attached
! * to.
!
! * DEPENDENCY_EXTENSION ('e'): the dependent object is a member of the
! * extension that is the referenced object. The dependent object can be
! * dropped only via DROP EXTENSION on the referenced object. Functionally
! * this dependency type acts the same as an internal dependency, but it's
! * kept separate for clarity and to simplify pg_dump.
! *
! * DEPENDENCY_AUTO_EXTENSION ('x'): the dependent object is not a member
! * of the extension that is the referenced object (and so should not be
! * ignored by pg_dump), but cannot function without the extension and
! * should be dropped when the extension itself is. The dependent object
! * may be dropped on its own as well.
! *
! * DEPENDENCY_PIN ('p'): there is no dependent object; this type of entry
! * is a signal that the system itself depends on the referenced object,
! * and so that object must never be deleted. Entries of this type are
! * created only during initdb. The fields for the dependent object
! * contain zeroes.
! *
! * Other dependency flavors may be needed in future.
*/
typedef enum DependencyType
--- 24,31 ----
*
* In all cases, a dependency relationship indicates that the referenced
* object may not be dropped without also dropping the dependent object.
! * However, there are several subflavors; see the description of pg_depend
! * in catalogs.sgml for details.
*/
typedef enum DependencyType
*************** typedef enum DependencyType
*** 89,95 ****
DEPENDENCY_NORMAL = 'n',
DEPENDENCY_AUTO = 'a',
DEPENDENCY_INTERNAL = 'i',
! DEPENDENCY_INTERNAL_AUTO = 'I',
DEPENDENCY_EXTENSION = 'e',
DEPENDENCY_AUTO_EXTENSION = 'x',
DEPENDENCY_PIN = 'p'
--- 33,40 ----
DEPENDENCY_NORMAL = 'n',
DEPENDENCY_AUTO = 'a',
DEPENDENCY_INTERNAL = 'i',
! DEPENDENCY_PARTITION_PRI = 'P',
! DEPENDENCY_PARTITION_SEC = 'S',
DEPENDENCY_EXTENSION = 'e',
DEPENDENCY_AUTO_EXTENSION = 'x',
DEPENDENCY_PIN = 'p'
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 55a2694..c87bded 100644
*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** extern char *ChooseConstraintName(const
*** 239,245 ****
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
extern void ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId);
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
bool missing_ok, Oid *constraintOid);
--- 239,246 ----
extern void AlterConstraintNamespaces(Oid ownerId, Oid oldNspId,
Oid newNspId, bool isType, ObjectAddresses *objsMoved);
extern void ConstraintSetParentConstraint(Oid childConstrId,
! Oid parentConstrId,
! Oid childTableId);
extern Oid get_relation_constraint_oid(Oid relid, const char *conname, bool missing_ok);
extern Bitmapset *get_relation_constraint_attnos(Oid relid, const char *conname,
bool missing_ok, Oid *constraintOid);
On Sun, Feb 10, 2019 at 8:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > How about this comment text? > > /* > * The current target object should have been added to > * targetObjects while processing the owning object; but it > * probably got only the flag bits associated with the > * dependency we're looking at. We need to add the objflags > * that were passed to this recursion level, too, else we may > * get a bogus failure in reportDependentObjects (if, for > * example, we were called due to a partition dependency). > * > * If somehow the current object didn't get scheduled for > * deletion, bleat. (That would imply that somebody deleted > * this dependency record before the recursion got to it.) > * Another idea would be to reacquire lock on the current > * object and resume trying to delete it, but it seems not > * worth dealing with the race conditions inherent in that. > */ LGTM. I agree that referencing a counterfactual design that reacquires the lock instead adds something. > Just to be be clear, my inclination is to do nothing about this in v11. > It's not apparent to me that any fix is possible given the v11 dependency > data, at least not without downsides that'd likely outweigh the upsides. > We've not seen field complaints about these problems. I thought that you might have had a trick up your sleeve for v11, although I had no idea how that would be possible without making sure that partition dependencies came in pairs to begin with. :-) I'll reply to your new revision of the patch separately. -- Peter Geoghegan
On Sun, Feb 10, 2019 at 8:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> [ invent separate primary and secondary partition dependencies? ] > Here's a version of the patch that does it that way. Now that I see separate DEPENDENCY_PARTITION_PRI and DEPENDENCY_PARTITION_SEC dependency types, I agree that it's clearer that way. It certainly clarifies what external dependency.c callers are up to. Minor issue here: > ! <varlistentry> > ! <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term> > ! <listitem> > ! <para> > ! A <quote>secondary</quote> partition dependency acts identically to > ! a primary one, except that the primary dependency is preferentially > ! referenced in error messages. An object should have at most one > ! primary partition dependency, but there could perhaps be multiple > ! secondary dependencies. > ! Example: actually, we'll set up a child partitioned index with the > ! parent partitioned index as primary partition dependency and the > ! partition table as secondary partition dependency. In this way, > ! if the user tries to drop the child partitioned index, the error > ! message will suggest dropping the parent partitioned index instead > ! (not the table). > </para> I think that the wording for this example needs to be tweaked. Other than that, looks good to me. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> I think that the wording for this example needs to be tweaked.
> Other than that, looks good to me.
After looking closer, I find that it's valid SGML to collapse the two
items into one entry, so how about:
<varlistentry>
<term><symbol>DEPENDENCY_PARTITION_PRI</symbol> (<literal>P</literal>)</term>
<term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term>
<listitem>
<para>
The dependent object was created as part of creation of the
referenced object, and is really just a part of its internal
implementation; however, unlike <literal>INTERNAL</literal>,
there is more than one such referenced object. The dependent object
must not be dropped unless at least one of these referenced objects
is dropped; if any one is, the dependent object should be dropped
whether or not <literal>CASCADE</literal> is specified. Also
unlike <literal>INTERNAL</literal>, a drop of some other object
that the dependent object depends on does not result in automatic
deletion of any partition-referenced object. Hence, if the drop
does not cascade to at least one of these objects via some other
path, it will be refused. (In most cases, the dependent object
shares all its non-partition dependencies with at least one
partition-referenced object, so that this restriction does not
result in blocking any cascaded delete.)
Primary and secondary partition dependencies behave identically
except that the primary dependency is preferred for use in error
messages; hence, a partition-dependent object should have one
primary partition dependency and one or more secondary partition
dependencies.
Note that partition dependencies are made in addition to, not
instead of, any dependencies the object would normally have. This
simplifies <command>ATTACH/DETACH PARTITION</command> operations:
the partition dependencies need only be added or removed.
Example: a child partitioned index is made partition-dependent
on both the partition table it is on and the parent partitioned
index, so that it goes away if either of those is dropped, but
not otherwise. The dependency on the parent index is primary,
so that if the user tries to drop the child partitioned index,
the error message will suggest dropping the parent index instead
(not the table).
</para>
</listitem>
</varlistentry>
regards, tom lane
On Sun, Feb 10, 2019 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > After looking closer, I find that it's valid SGML to collapse the two > items into one entry I'll have to remember that detail -- seems like it'll come in handy again. > <varlistentry> > <term><symbol>DEPENDENCY_PARTITION_PRI</symbol> (<literal>P</literal>)</term> > <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term> > <listitem> > <para> > Primary and secondary partition dependencies behave identically > except that the primary dependency is preferred for use in error > messages; hence, a partition-dependent object should have one > primary partition dependency and one or more secondary partition > dependencies. > Note that partition dependencies are made in addition to, not > instead of, any dependencies the object would normally have. This > simplifies <command>ATTACH/DETACH PARTITION</command> operations: > the partition dependencies need only be added or removed. > Example: a child partitioned index is made partition-dependent > on both the partition table it is on and the parent partitioned > index, so that it goes away if either of those is dropped, but > not otherwise. The dependency on the parent index is primary, > so that if the user tries to drop the child partitioned index, > the error message will suggest dropping the parent index instead > (not the table). That seems perfect. It gets to the root of the matter. -- Peter Geoghegan
On 2019-Feb-10, Tom Lane wrote: > Primary and secondary partition dependencies behave identically > except that the primary dependency is preferred for use in error > messages; hence, a partition-dependent object should have one > primary partition dependency and one or more secondary partition > dependencies. Hmm, zero or more secondary partition dependencies? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-10, Tom Lane wrote:
>> Primary and secondary partition dependencies behave identically
>> except that the primary dependency is preferred for use in error
>> messages; hence, a partition-dependent object should have one
>> primary partition dependency and one or more secondary partition
>> dependencies.
> Hmm, zero or more secondary partition dependencies?
If there's only one partition dependency, why use the mechanism
at all?
regards, tom lane
On 2019-Feb-10, Peter Geoghegan wrote: > On Sun, Feb 10, 2019 at 8:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Just to be be clear, my inclination is to do nothing about this in v11. > > It's not apparent to me that any fix is possible given the v11 dependency > > data, at least not without downsides that'd likely outweigh the upsides. > > We've not seen field complaints about these problems. > > I thought that you might have had a trick up your sleeve for v11, > although I had no idea how that would be possible without making sure > that partition dependencies came in pairs to begin with. :-) If we disregard the scenario were people downgrade across minor versions, it's likely possible to produce SQL queries to transform from the old arrangement to the new one, and include those in release notes or a wiki page; not for this week's minors (ENOTIME) but maybe for the next one. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-10, Peter Geoghegan wrote:
>> On Sun, Feb 10, 2019 at 8:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Just to be be clear, my inclination is to do nothing about this in v11.
>>> It's not apparent to me that any fix is possible given the v11 dependency
>>> data, at least not without downsides that'd likely outweigh the upsides.
>>> We've not seen field complaints about these problems.
>> I thought that you might have had a trick up your sleeve for v11,
>> although I had no idea how that would be possible without making sure
>> that partition dependencies came in pairs to begin with. :-)
> If we disregard the scenario were people downgrade across minor
> versions, it's likely possible to produce SQL queries to transform from
> the old arrangement to the new one, and include those in release notes
> or a wiki page; not for this week's minors (ENOTIME) but maybe for the
> next one.
Dunno ... we couldn't force people to do that, so the server would have to
be prepared to cope with either arrangement, which seems like an
impossible mess.
regards, tom lane
On 2019-Feb-10, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > If we disregard the scenario were people downgrade across minor > > versions, it's likely possible to produce SQL queries to transform from > > the old arrangement to the new one, and include those in release notes > > or a wiki page; not for this week's minors (ENOTIME) but maybe for the > > next one. > > Dunno ... we couldn't force people to do that, so the server would have to > be prepared to cope with either arrangement, which seems like an > impossible mess. True. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I've pushed this now. I made one additional change, which was to fix
things so that if both an INTERNAL and an EXTENSION dependency exist,
the first loop will reliably complain about the EXTENSION dependency.
It only takes one more if-test to do that now that we're postponing
the error report till after the loop, and this way we don't need to
split hairs about how likely it is for both to exist.
I think we're done with this thread, though I still need to look at
the problem I complained of in <26527.1549572789@sss.pgh.pa.us>.
regards, tom lane
On Mon, Feb 11, 2019 at 11:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think we're done with this thread, though I still need to look at > the problem I complained of in <26527.1549572789@sss.pgh.pa.us>. Right, we're done with this thread now. Thanks again! -- Peter Geoghegan
On 2019-Feb-11, Tom Lane wrote: > I've pushed this now. I made one additional change, which was to fix > things so that if both an INTERNAL and an EXTENSION dependency exist, > the first loop will reliably complain about the EXTENSION dependency. > It only takes one more if-test to do that now that we're postponing > the error report till after the loop, and this way we don't need to > split hairs about how likely it is for both to exist. > > I think we're done with this thread, though I still need to look at > the problem I complained of in <26527.1549572789@sss.pgh.pa.us>. Thanks for taking care of this! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services