Thread: DELETE CASCADE
Hi -hackers,
Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.
That said, I'm happy to quibble about the specific approach to be taken; I've written this based on the most straightforward way I could come up with to accomplish this, but if there are better directions to take to get the equivalent functionality I'm happy to discuss.
From the commit message:
Proof of concept of allowing a DELETE statement to override formal FK's handling from RESTRICT/NO
ACTION and treat as CASCADE instead.
Syntax is "DELETE CASCADE ..." instead of "DELETE ... CASCADE" due to unresolvable bison conflicts.
Sample session:
postgres=# create table foo (id serial primary key, val text);
CREATE TABLE
postgres=# create table bar (id serial primary key, foo_id int references foo(id), val text);
CREATE TABLE
postgres=# insert into foo (val) values ('a'),('b'),('c');
INSERT 0 3
postgres=# insert into bar (foo_id, val) values (1,'d'),(1,'e'),(2,'f'),(2,'g');
INSERT 0 4
postgres=# select * from foo;
id | val
----+-----
1 | a
2 | b
3 | c
(3 rows)
postgres=# select * from bar;
id | foo_id | val
----+--------+-----
1 | 1 | d
2 | 1 | e
3 | 2 | f
4 | 2 | g
(4 rows)
postgres=# delete from foo where id = 1;
ERROR: update or delete on table "foo" violates foreign key constraint "bar_foo_id_fkey" on table "bar"
DETAIL: Key (id)=(1) is still referenced from table "bar".
postgres=# delete cascade from foo where id = 1;
DELETE 1
postgres=# select * from foo;
id | val
----+-----
2 | b
3 | c
(2 rows)
postgres=# select * from bar;
id | foo_id | val
----+--------+-----
3 | 2 | f
4 | 2 | g
(2 rows)
ACTION and treat as CASCADE instead.
Syntax is "DELETE CASCADE ..." instead of "DELETE ... CASCADE" due to unresolvable bison conflicts.
Sample session:
postgres=# create table foo (id serial primary key, val text);
CREATE TABLE
postgres=# create table bar (id serial primary key, foo_id int references foo(id), val text);
CREATE TABLE
postgres=# insert into foo (val) values ('a'),('b'),('c');
INSERT 0 3
postgres=# insert into bar (foo_id, val) values (1,'d'),(1,'e'),(2,'f'),(2,'g');
INSERT 0 4
postgres=# select * from foo;
id | val
----+-----
1 | a
2 | b
3 | c
(3 rows)
postgres=# select * from bar;
id | foo_id | val
----+--------+-----
1 | 1 | d
2 | 1 | e
3 | 2 | f
4 | 2 | g
(4 rows)
postgres=# delete from foo where id = 1;
ERROR: update or delete on table "foo" violates foreign key constraint "bar_foo_id_fkey" on table "bar"
DETAIL: Key (id)=(1) is still referenced from table "bar".
postgres=# delete cascade from foo where id = 1;
DELETE 1
postgres=# select * from foo;
id | val
----+-----
2 | b
3 | c
(2 rows)
postgres=# select * from bar;
id | foo_id | val
----+--------+-----
3 | 2 | f
4 | 2 | g
(2 rows)
Best,
David
Attachment
On Thu, 3 Jun 2021 at 16:49, David Christensen <david.christensen@crunchydata.com> wrote:
Hi -hackers,Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.
I would sometimes find this convenient. There are circumstances where I don't want every DELETE to blunder all over the database deleting stuff, but certain specific DELETEs should take care of the referencing tables.
An additional syntax to say "CASCADE TO table1, table2" would be safer and sometimes useful in the case where I know I want to cascade to specific other tables but not all (and in particular not to ones I didn't think of when I wrote the query); I might almost suggest omitting the cascade to all syntax (or maybe have a separate syntax, literally "CASCADE TO ALL TABLES" or some such).
What happens if I don't have delete permission on the referencing table? When a foreign key reference delete cascades, I can cause records to disappear from a referencing table even if I don't have delete permission on that table. This feels like it's just supposed to be a convenience that replaces multiple DELETE invocations but one way or the other we need to be clear on the behaviour.
Sidebar: isn't this inconsistent with trigger behaviour in general? When I say "ON DELETE CASCADE" what I mean and what I get are the same: whenever the referenced row is deleted, the referencing row also disappears, regardless of the identity or permissions of the role running the actual DELETE. But any manually implemented trigger runs as the caller; I cannot make the database do something when a table update occurs; I can only make the role doing the table update perform some additional actions.
On Thu, Jun 3, 2021 at 1:49 PM David Christensen <david.christensen@crunchydata.com> wrote:
Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints.
ON DELETE NO ACTION constraints become ON DELETE CASCADE constraints - ON DELETE SET NULL constraints are ignored, and not possible to emulate via this feature.
I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.
Once...but I agreed.
That said, I'm happy to quibble about the specific approach to be taken; I've written this based on the most straightforward way I could come up with to accomplish this, but if there are better directions to take to get the equivalent functionality I'm happy to discuss.
This behavior should require the same permissions as actually creating an ON DELETE CASCADE FK on the cascaded-to tables. i.e., Table Owner role membership (the requirement for FK permissions can be assumed by the presence of the existing FK constraint and being the table's owner).
Having the defined FK behaviors be more readily changeable, while not mitigating this need, is IMO a more important feature to implement. If there is a reason that cannot be implemented (besides no one has bothered to take the time) then I would consider that reason to also apply to prevent implementing this work-around.
David J.
On Thu, Jun 3, 2021 at 4:48 PM David G. Johnston <david.g.johnston@gmail.com> wrote:
On Thu, Jun 3, 2021 at 1:49 PM David Christensen <david.christensen@crunchydata.com> wrote:Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints.ON DELETE NO ACTION constraints become ON DELETE CASCADE constraints - ON DELETE SET NULL constraints are ignored, and not possible to emulate via this feature.
I have not tested this part per se (which clearly I need to expand the existing test suite), but my reasoning here was that ON DELETE SET NULL/DEFAULT would still be applied with their defined behaviors (being that we're still calling the underlying RI triggers using SPI) with the same results; the intent of this feature is just to suppress the RESTRICT action and cascade the DELETE to all tables (on down the chain) which would normally block this, without having to manually figure all the dependencies which can be inferred by the database itself.
I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.Once...but I agreed.
Heh.
That said, I'm happy to quibble about the specific approach to be taken; I've written this based on the most straightforward way I could come up with to accomplish this, but if there are better directions to take to get the equivalent functionality I'm happy to discuss.This behavior should require the same permissions as actually creating an ON DELETE CASCADE FK on the cascaded-to tables. i.e., Table Owner role membership (the requirement for FK permissions can be assumed by the presence of the existing FK constraint and being the table's owner).
I'm not sure if this would be overly prohibitive or not, but if you're the table owner this should just work, like you point out. I think this restriction could be fine for the common case, and if there was a way to hint if/when this failed to cascade as to the actual reason for the failure I'm fine with that part too. (I was assuming that DELETE permission on the underlying tables + existence of FK would be enough in practice, but we could definitely tighten that up.)
Having the defined FK behaviors be more readily changeable, while not mitigating this need, is IMO a more important feature to implement. If there is a reason that cannot be implemented (besides no one has bothered to take the time) then I would consider that reason to also apply to prevent implementing this work-around.
Agreed that this would be a nice feature to have too; noone wants to break FK consistency to change things or require a rescan of a valid constraint.
On Thu, Jun 3, 2021 at 4:15 PM Isaac Morland <isaac.morland@gmail.com> wrote:
On Thu, 3 Jun 2021 at 16:49, David Christensen <david.christensen@crunchydata.com> wrote:Hi -hackers,Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.I would sometimes find this convenient. There are circumstances where I don't want every DELETE to blunder all over the database deleting stuff, but certain specific DELETEs should take care of the referencing tables.An additional syntax to say "CASCADE TO table1, table2" would be safer and sometimes useful in the case where I know I want to cascade to specific other tables but not all (and in particular not to ones I didn't think of when I wrote the query); I might almost suggest omitting the cascade to all syntax (or maybe have a separate syntax, literally "CASCADE TO ALL TABLES" or some such).
I'm not fond of the syntax requirements for the explicitness here, plus it seems like it would complicate the functionality of the patch (which currently is able to just slightly refactor the RI triggers to account for a single state variable, rather than do anything smarter than that). I do understand the desire/need for visibility into what would be affected with an offhand statement.
What happens if I don't have delete permission on the referencing table? When a foreign key reference delete cascades, I can cause records to disappear from a referencing table even if I don't have delete permission on that table. This feels like it's just supposed to be a convenience that replaces multiple DELETE invocations but one way or the other we need to be clear on the behaviour.
Did you test this and find a failure? Because it is literally using all of the same RI proc code/permissions as defined I would expect that it would just abort the transaction. (I am working on expanding the test suite for this feature to allow for test cases like this, so keep 'em coming... :-))
Sidebar: isn't this inconsistent with trigger behaviour in general? When I say "ON DELETE CASCADE" what I mean and what I get are the same: whenever the referenced row is deleted, the referencing row also disappears, regardless of the identity or permissions of the role running the actual DELETE. But any manually implemented trigger runs as the caller; I cannot make the database do something when a table update occurs; I can only make the role doing the table update perform some additional actions.
Have you found a failure? Because all this is doing is effectively calling the guts of the cascade RI routines, so no differences should occur. If not, I'm not quite clear on your objection; can you clarify?
David
What happens if I don't have delete permission on the referencing table? When a foreign key reference delete cascades, I can cause records to disappear from a referencing table even if I don't have delete permission on that table. This feels like it's just supposed to be a convenience that replaces multiple DELETE invocations but one way or the other we need to be clear on the behaviour.Did you test this and find a failure? Because it is literally using all of the same RI proc code/permissions as defined I would expect that it would just abort the transaction. (I am working on expanding the test suite for this feature to allow for test cases like this, so keep 'em coming... :-))
Enclosed is a basic test script and the corresponding output run through `psql -e` (will adapt into part of the regression test, but wanted to get this out there). TL;DR; DELETE CASCADE behaves exactly as if said constraint were defined as a ON DELETE CASCADE FK constraint wrt DELETE permission behavior. I do agree in this case, that it makes sense to throw an error if we're trying to bypass the RESTRICT behavior and we are not part of the table owner role (and since this would be called/checked recursively for each table involved in the graph I think we can count on it reporting the appropriate error message in this case).
Attachment
On Thu, 3 Jun 2021 at 18:08, David Christensen <david.christensen@crunchydata.com> wrote:
On Thu, Jun 3, 2021 at 4:15 PM Isaac Morland <isaac.morland@gmail.com> wrote:What happens if I don't have delete permission on the referencing table? When a foreign key reference delete cascades, I can cause records to disappear from a referencing table even if I don't have delete permission on that table. This feels like it's just supposed to be a convenience that replaces multiple DELETE invocations but one way or the other we need to be clear on the behaviour.Did you test this and find a failure? Because it is literally using all of the same RI proc code/permissions as defined I would expect that it would just abort the transaction. (I am working on expanding the test suite for this feature to allow for test cases like this, so keep 'em coming... :-))
I haven't run your patch. I'm just asking because it's a question about exactly how the behaviour works that needs to be clearly and intentionally decided (and documented). I think aborting the transaction with a permission denied error on the referencing table is probably the right behaviour: it's what you would get if you issued an equivalent delete on the referencing table explicitly. I think of your patch as being a convenience to avoid having to write a separate DELETE for each referencing table. So based on what you say, it sounds like you've already covered this issue.
Sidebar: isn't this inconsistent with trigger behaviour in general? When I say "ON DELETE CASCADE" what I mean and what I get are the same: whenever the referenced row is deleted, the referencing row also disappears, regardless of the identity or permissions of the role running the actual DELETE. But any manually implemented trigger runs as the caller; I cannot make the database do something when a table update occurs; I can only make the role doing the table update perform some additional actions.Have you found a failure? Because all this is doing is effectively calling the guts of the cascade RI routines, so no differences should occur. If not, I'm not quite clear on your objection; can you clarify?
Sorry, my sidebar is only tangentially related. In another thread we had a discussion about triggers, which it turns out execute as the role running the command, not as the owner of the table. For many triggers it doesn't matter, but for many things I can think of that I would want to do with triggers it will only work if the trigger executes as the owner of the table (or trigger, hypothetically…); and there are several common cases where it makes way more sense to execute as the owner (e.g., triggers to maintain a log table; it doesn't make sense to have to grant permissions on the log table to roles with permissions on the main table, and also allows spurious log entries to be made). But here it seems that cascaded actions do execute as a role that is not dependent on who is running the command.
In short, I probably should have left off the sidebar. It's not an issue with your patch.
On Thu, 3 Jun 2021 at 18:25, David Christensen <david.christensen@crunchydata.com> wrote:
What happens if I don't have delete permission on the referencing table? When a foreign key reference delete cascades, I can cause records to disappear from a referencing table even if I don't have delete permission on that table. This feels like it's just supposed to be a convenience that replaces multiple DELETE invocations but one way or the other we need to be clear on the behaviour.Did you test this and find a failure? Because it is literally using all of the same RI proc code/permissions as defined I would expect that it would just abort the transaction. (I am working on expanding the test suite for this feature to allow for test cases like this, so keep 'em coming... :-))Enclosed is a basic test script and the corresponding output run through `psql -e` (will adapt into part of the regression test, but wanted to get this out there). TL;DR; DELETE CASCADE behaves exactly as if said constraint were defined as a ON DELETE CASCADE FK constraint wrt DELETE permission behavior. I do agree in this case, that it makes sense to throw an error if we're trying to bypass the RESTRICT behavior and we are not part of the table owner role (and since this would be called/checked recursively for each table involved in the graph I think we can count on it reporting the appropriate error message in this case).
Surely you mean if we don't have DELETE permission on the referencing table? I don't see why we need to be a member of the table owner role.
On Thu, Jun 3, 2021 at 3:29 PM Isaac Morland <isaac.morland@gmail.com> wrote:
Surely you mean if we don't have DELETE permission on the referencing table? I don't see why we need to be a member of the table owner role.
I would reverse the question - why does this feature need to allow the more broad DELETE permission instead of just limiting it to the table owner? The latter matches the required permission for the existing cascade feature that this is extending.
David J.
On 03.06.21 23:47, David G. Johnston wrote: > This behavior should require the same permissions as actually creating > an ON DELETE CASCADE FK on the cascaded-to tables. i.e., Table Owner > role membership (the requirement for FK permissions can be assumed by > the presence of the existing FK constraint and being the table's owner). You can create foreign keys if you have the REFERENCES privilege on the primary key table. That's something this patch doesn't observe correctly: Normally, the owner of the foreign key table decides the cascade action, but with this patch, it's the primary key table owner.
On Fri, Jun 4, 2021 at 2:53 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 03.06.21 23:47, David G. Johnston wrote:
> This behavior should require the same permissions as actually creating
> an ON DELETE CASCADE FK on the cascaded-to tables. i.e., Table Owner
> role membership (the requirement for FK permissions can be assumed by
> the presence of the existing FK constraint and being the table's owner).
You can create foreign keys if you have the REFERENCES privilege on the
primary key table. That's something this patch doesn't observe
correctly: Normally, the owner of the foreign key table decides the
cascade action, but with this patch, it's the primary key table owner.
So what are the necessary and sufficient conditions to check at this point? The constraint already exists, so what permissions would we need to check against which table(s) in order to grant this action?
On Fri, 4 Jun 2021 at 16:24, David Christensen <david.christensen@crunchydata.com> wrote:
On Fri, Jun 4, 2021 at 2:53 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:On 03.06.21 23:47, David G. Johnston wrote:
> This behavior should require the same permissions as actually creating
> an ON DELETE CASCADE FK on the cascaded-to tables. i.e., Table Owner
> role membership (the requirement for FK permissions can be assumed by
> the presence of the existing FK constraint and being the table's owner).
You can create foreign keys if you have the REFERENCES privilege on the
primary key table. That's something this patch doesn't observe
correctly: Normally, the owner of the foreign key table decides the
cascade action, but with this patch, it's the primary key table owner.So what are the necessary and sufficient conditions to check at this point? The constraint already exists, so what permissions would we need to check against which table(s) in order to grant this action?
I apologize if I am deeply confused, but say I have this:
CREATE TABLE parent (
pid int primary key,
parent_data text
);
CREATE TABLE child (
pid int REFERENCES parent,
cid int,
PRIMARY KEY (pid, cid),
child_data text
);
It's easy to imagine needing to write:
DELETE FROM child WHERE ...
DELETE FROM parent WHERE ...
... where the WHERE clauses both work out to the same pid values. It would be nice to be able to say:
DELETE CASCADE FROM parent WHERE ...
... and just skip writing the first DELETE entirely. And what do I mean by "DELETE CASCADE" if not "delete the referencing rows from child"? So to me I think I should require DELETE permission on child (and parent) in order to execute this DELETE CASCADE. I definitely shouldn't require any DDL-related permissions (table owner, REFERENCES, …) because I'm not doing DDL - just data changes. Sure, it may be implemented by temporarily treating the foreign key references differently, but conceptually I'm just deleting from multiple tables in one command.
I will say I would prefer this syntax:
DELETE FROM parent WHERE ... CASCADE TO child;
(or "CASCADE TO ALL TABLES" or some such if I want that)
I don't like the idea of saying "CASCADE" and getting a bunch of tables I didn't intend (or which didn't exist when the query was written).
On Fri, Jun 4, 2021 at 3:40 PM Isaac Morland <isaac.morland@gmail.com> wrote:
I apologize if I am deeply confused, but say I have this:CREATE TABLE parent (pid int primary key,parent_data text);CREATE TABLE child (pid int REFERENCES parent,cid int,PRIMARY KEY (pid, cid),child_data text);It's easy to imagine needing to write:DELETE FROM child WHERE ...DELETE FROM parent WHERE ...... where the WHERE clauses both work out to the same pid values. It would be nice to be able to say:DELETE CASCADE FROM parent WHERE ...
This is entirely the use case and the motivation.
... and just skip writing the first DELETE entirely. And what do I mean by "DELETE CASCADE" if not "delete the referencing rows from child"? So to me I think I should require DELETE permission on child (and parent) in order to execute this DELETE CASCADE. I definitely shouldn't require any DDL-related permissions (table owner, REFERENCES, …) because I'm not doing DDL - just data changes. Sure, it may be implemented by temporarily treating the foreign key references differently, but conceptually I'm just deleting from multiple tables in one command.
This is the part where I'm also running into some conceptual roadblocks between what is an implementation issue based on the current behavior of CASCADE triggers, and what makes sense in terms of a POLA perspective. In part, I am having this discussion to flesh out this part of the problem.
I will say I would prefer this syntax:DELETE FROM parent WHERE ... CASCADE TO child;(or "CASCADE TO ALL TABLES" or some such if I want that)I don't like the idea of saying "CASCADE" and getting a bunch of tables I didn't intend (or which didn't exist when the query was written).
A soft -1 from me here, though I understand the rationale here; you would be unable to manually delete these records with the existing constraints if there were a `grandchild` table without first removing those records too. (Maybe some method of previewing which relations/FKs would be involved here would be a suitable compromise, but I have no idea what that would look like or how it would work.) (Maybe just NOTICE: DELETE CASCADES to ... for each table, and people should know to wrap in a transaction if they don't know what will happen.)
David
On 04.06.21 22:24, David Christensen wrote: > So what are the necessary and sufficient conditions to check at this > point? The constraint already exists, so what permissions would we need > to check against which table(s) in order to grant this action? I think you would need DELETE privilege on all affected tables.
On 03.06.21 22:49, David Christensen wrote: > Presented for discussion is a POC for a DELETE CASCADE functionality, > which will allow you one-shot usage of treating existing NO ACTION and > RESTRICT FK constraints as if they were originally defined as CASCADE > constraints. I can't tell you how many times this functionality would > have been useful in the field, and despite the expected answer of > "define your constraints right in the first place", this is not always > an option, nor is the ability to change that easily (or create new > constraints that need to revalidate against big tables) always the best > option. I think, if we think this is useful, the other way around would also be useful: Override a foreign key defined as ON DELETE CASCADE to behave as RESTRICT for a particular command.
> On Jun 5, 2021, at 2:30 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 03.06.21 22:49, David Christensen wrote: >> Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treatingexisting NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can'ttell you how many times this functionality would have been useful in the field, and despite the expected answer of "defineyour constraints right in the first place", this is not always an option, nor is the ability to change that easily(or create new constraints that need to revalidate against big tables) always the best option. > > I think, if we think this is useful, the other way around would also be useful: Override a foreign key defined as ON DELETECASCADE to behave as RESTRICT for a particular command. I am not opposed to this, but I am struggling to come up with a use case. Where would this be useful? David
On Sat, 5 Jun 2021 at 03:30, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 03.06.21 22:49, David Christensen wrote:
> Presented for discussion is a POC for a DELETE CASCADE functionality,
> which will allow you one-shot usage of treating existing NO ACTION and
> RESTRICT FK constraints as if they were originally defined as CASCADE
> constraints. I can't tell you how many times this functionality would
> have been useful in the field, and despite the expected answer of
> "define your constraints right in the first place", this is not always
> an option, nor is the ability to change that easily (or create new
> constraints that need to revalidate against big tables) always the best
> option.
I think, if we think this is useful, the other way around would also be
useful: Override a foreign key defined as ON DELETE CASCADE to behave as
RESTRICT for a particular command.
This is not as obviously useful as the other, but might conceivably still have applications.
We would need to be very careful about permissions. This is a substitute for checking whether there are any matching rows in the referring tables and throwing an error manually in that case. My immediate reaction is that this should require SELECT permission on the referring tables. Or to be more precise, SELECT permission on the foreign key columns in the referring tables.
> On Jun 5, 2021, at 2:29 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 04.06.21 22:24, David Christensen wrote: >> So what are the necessary and sufficient conditions to check at this point? The constraint already exists, so what permissionswould we need to check against which table(s) in order to grant this action? > > I think you would need DELETE privilege on all affected tables. So basically where we are dispatching to the CASCADE guts, first check session user’s DELETE permission and throw the normalpermissions error if they can’t delete?
On 05.06.21 14:21, David Christensen wrote: > >> On Jun 5, 2021, at 2:30 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: >> >> On 03.06.21 22:49, David Christensen wrote: >>> Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treatingexisting NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can'ttell you how many times this functionality would have been useful in the field, and despite the expected answer of "defineyour constraints right in the first place", this is not always an option, nor is the ability to change that easily(or create new constraints that need to revalidate against big tables) always the best option. >> >> I think, if we think this is useful, the other way around would also be useful: Override a foreign key defined as ON DELETECASCADE to behave as RESTRICT for a particular command. > > I am not opposed to this, but I am struggling to come up with a use case. Where would this be useful? If you suspect a primary key row is no longer used, you want to delete it, but don't want to accidentally delete it if it's still used. I sense more complicated concurrency and permission issues, however.
On 05.06.21 14:25, David Christensen wrote: > >> On Jun 5, 2021, at 2:29 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: >> >> On 04.06.21 22:24, David Christensen wrote: >>> So what are the necessary and sufficient conditions to check at this point? The constraint already exists, so what permissionswould we need to check against which table(s) in order to grant this action? >> >> I think you would need DELETE privilege on all affected tables. > > So basically where we are dispatching to the CASCADE guts, first check session user’s DELETE permission and throw the normalpermissions error if they can’t delete? Actually, you also need appropriate SELECT permissions that correspond to the WHERE clause of the DELETE statement.
On Mon, Jun 7, 2021 at 2:54 AM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
On 05.06.21 14:21, David Christensen wrote:
>
>> On Jun 5, 2021, at 2:30 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> On 03.06.21 22:49, David Christensen wrote:
>>> Presented for discussion is a POC for a DELETE CASCADE functionality, which will allow you one-shot usage of treating existing NO ACTION and RESTRICT FK constraints as if they were originally defined as CASCADE constraints. I can't tell you how many times this functionality would have been useful in the field, and despite the expected answer of "define your constraints right in the first place", this is not always an option, nor is the ability to change that easily (or create new constraints that need to revalidate against big tables) always the best option.
>>
>> I think, if we think this is useful, the other way around would also be useful: Override a foreign key defined as ON DELETE CASCADE to behave as RESTRICT for a particular command.
>
> I am not opposed to this, but I am struggling to come up with a use case. Where would this be useful?
If you suspect a primary key row is no longer used, you want to delete
it, but don't want to accidentally delete it if it's still used.
Okay, I can see that.
I sense more complicated concurrency and permission issues, however.
Assuming this happens in the same transaction, wouldn't this just work? Or are you thinking there needs to be some sort of predicate lock to prevent a concurrent add of the referencing record in the FK table?
David
> So basically where we are dispatching to the CASCADE guts, first check session user’s DELETE permission and throw the normal permissions error if they can’t delete?
Actually, you also need appropriate SELECT permissions that correspond
to the WHERE clause of the DELETE statement.
So this would be both a table-level and column level check? (It seems odd that we could configure a policy whereby we could DELETE an arbitrary row in the table, but not SELECT which one, but I can see that there could be information leakage implications here.)
Other permissions-level things to consider, like RLS, or is this part automatic? Do you happen to know offhand another instance in the code which takes these granular permissions into consideration? Might help bootstrap both the understanding and the implementation of this.
Thanks,
David
On 08.06.21 21:29, David Christensen wrote: > > So basically where we are dispatching to the CASCADE guts, first > check session user’s DELETE permission and throw the normal > permissions error if they can’t delete? > > Actually, you also need appropriate SELECT permissions that correspond > to the WHERE clause of the DELETE statement. > > > So this would be both a table-level and column level check? (It seems > odd that we could configure a policy whereby we could DELETE an > arbitrary row in the table, but not SELECT which one, but I can see that > there could be information leakage implications here.) > > Other permissions-level things to consider, like RLS, or is this part > automatic? Do you happen to know offhand another instance in the code > which takes these granular permissions into consideration? Might help > bootstrap both the understanding and the implementation of this. All of this permissions checking code already exists in the executor, so the question perhaps isn't so much which details to consider but how to make use of that code. If you convince the trigger actions to run as the invoker of the original DELETE rather than whatever they are doing now, that should all work correctly. How to do that, I don't know right now.
On 08.06.21 21:25, David Christensen wrote: > I sense more complicated concurrency and permission issues, however. > > Assuming this happens in the same transaction, wouldn't this just work? > Or are you thinking there needs to be some sort of predicate lock to > prevent a concurrent add of the referencing record in the FK table? It might work, I'm just saying it needs to be thought about carefully. If you have functionality like, delete this if there is no matching record over there, you need to have the permission to check that and need to make sure it stays that way.
On Wednesday, June 9, 2021, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
It might work, I'm just saying it needs to be thought about carefully. If you have functionality like, delete this if there is no matching record over there, you need to have the permission to check that and need to make sure it stays that way.
Which I believe the presence of an existing foreign key does quite nicely. Thus if the executing user is the table owner (group membership usually) and a FK already exists, the conditions for the cascade are fulfilled, including locking I would think, because that FK could have been defined to just do it without all this. We are effectively just temporarily changing that aspect of the foreign key - the behavior should be identical to on cascade delete.
I require convincing that there is a use case that requires laxer permissions. Especially if we can solve the whole changing of the cascade option without having to drop the foreign key.
David J.
On Wed, Jun 9, 2021 at 8:48 AM David G. Johnston <david.g.johnston@gmail.com> wrote:
On Wednesday, June 9, 2021, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
It might work, I'm just saying it needs to be thought about carefully. If you have functionality like, delete this if there is no matching record over there, you need to have the permission to check that and need to make sure it stays that way.Which I believe the presence of an existing foreign key does quite nicely. Thus if the executing user is the table owner (group membership usually) and a FK already exists, the conditions for the cascade are fulfilled, including locking I would think, because that FK could have been defined to just do it without all this. We are effectively just temporarily changing that aspect of the foreign key - the behavior should be identical to on cascade delete.
I think Peter is referring to the DELETE RESTRICT proposed mirror behavior in this specific case, not DELETE CASCADE specifically.
I require convincing that there is a use case that requires laxer permissions. Especially if we can solve the whole changing of the cascade option without having to drop the foreign key.
This was my original feeling as well, though really if I was going to run this operation it would likely already be the database owner or superuser, so my desire to make this work in all situations is tempered with my desire to just have the basic functionality available at *some* level. :-)
David
David G. Johnston writes: > Having the defined FK behaviors be more readily changeable, while not > mitigating this need, is IMO a more important feature to implement. If > there is a reason that cannot be implemented (besides no one has bothered > to take the time) then I would consider that reason to also apply to > prevent implementing this work-around. > > David J. I assume this would look something like: ALTER TABLE foo ALTER CONSTRAINT my_fkey ON UPDATE CASCADE ON DELETE RESTRICT with omitted referential_action implying preserving the existing one. Seems if we were going to tackle this particular problem, there would be two possible approaches here: 1) Change the definitions of the RI_FKey_* constraints for (at least) RI_FKey_*_del() to instead share a single function definition RI_FKey_del() and then pass in the constraint type operation (restrict, cascade, no action, etc) in as a trigger argument instead of having separate functions for each constraint type here. This would then ensure that the dispatch function could both change the constriant just by modifying the trigger arguments, as well as allowing for potential different behavior depending on how the underlying function is called. 2) Keep the existing RI trigger functions, but allow an ALTER CONSTRAINT variant to replace the trigger function to the new desired value, preserving (or transforming, as necessary) the original arguments. A few things off-hand: - pg_trigger locking will be necessary as we change the underlying args for the tables in question. This seems unavoidable. - partitions; do we need to lock them all in both solutions, or can we get away without it in the first approach? - with the first solution you would lose the self-describing name of the trigger functions themselves (moving to the trigger args instead); while it is a change in a very long-standing behavior/design, it *should* be an implementation detail, and allows things like the explicit DELETE [ RESTRICT | CASCADE ] the original patch was pushing for. - probably more I haven't thought of. Best, David
[ a couple of random thoughts after quickly scanning this thread ... ]
David Christensen <david.christensen@crunchydata.com> writes:
> I assume this would look something like:
> ALTER TABLE foo ALTER CONSTRAINT my_fkey ON UPDATE CASCADE ON DELETE RESTRICT
> with omitted referential_action implying preserving the existing one.
I seem to remember somebody working on exactly that previously, though
it's evidently not gotten committed. In any case, we already have
ALTER TABLE ... ALTER CONSTRAINT constraint_name
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
which has to modify pg_trigger rows, so it's hard to see why it'd
be at all difficult to implement this using code similar to that
(maybe even sharing much of the code).
Returning to the original thought of a DML statement option to temporarily
override the referential_action, I wonder why only temporarily-set-CASCADE
was considered. It seems to me like there might also be use-cases for
temporarily selecting the SET NULL or SET DEFAULT actions.
Another angle is that if we consider the deferrability properties as
precedent, there already is a way to override an FK constraint's
deferrability for the duration of a transaction: see SET CONSTRAINTS.
So I wonder if maybe the way to treat this is to invent something like
SET CONSTRAINTS my_fk_constraint [,...] ON DELETE referential_action
which would override the constraints' action for the remainder of the
transaction. (Permission needs TBD, but probably the same as you
would need to create a new FK constraint on the relevant table.)
In comparison to the original proposal, this'd force you to be explicit
about which constraint(s) you intend to override, but TBH I think that's
a good thing.
One big practical problem, which we've never addressed in the context of
SET CONSTRAINTS but maybe it's time to tackle, is that the SQL spec
defines the syntax like that because it thinks constraint names are
unique per-schema; thus a possibly-schema-qualified name is sufficient
ID. Of course we say that constraint names are only unique per-table,
so SET CONSTRAINTS has always had this issue of not being very carefully
targeted. I think we could do something like extending the syntax
to be
SET CONSTRAINTS conname [ON tablename] [,...] new_properties
Anyway, just food for thought --- I'm not necessarily set on any
of this.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > [ a couple of random thoughts after quickly scanning this thread ... ] > > David Christensen <david.christensen@crunchydata.com> writes: >> I assume this would look something like: >> ALTER TABLE foo ALTER CONSTRAINT my_fkey ON UPDATE CASCADE ON DELETE RESTRICT >> with omitted referential_action implying preserving the existing one. > > I seem to remember somebody working on exactly that previously, though > it's evidently not gotten committed. In any case, we already have > > ALTER TABLE ... ALTER CONSTRAINT constraint_name > [ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ] > > which has to modify pg_trigger rows, so it's hard to see why it'd > be at all difficult to implement this using code similar to that > (maybe even sharing much of the code). Sure; this was my assumption as well. > Returning to the original thought of a DML statement option to temporarily > override the referential_action, I wonder why only temporarily-set-CASCADE > was considered. It seems to me like there might also be use-cases for > temporarily selecting the SET NULL or SET DEFAULT actions. Agreed; DELETE CASCADE had been the originating use case, but no reason to limit it to just that action. (I'd later expanded it to add RESTRICT as well, but barring implementation issues, probably no reason to limit any of referential action.) > Another angle is that if we consider the deferrability properties as > precedent, there already is a way to override an FK constraint's > deferrability for the duration of a transaction: see SET CONSTRAINTS. > So I wonder if maybe the way to treat this is to invent something like > > SET CONSTRAINTS my_fk_constraint [,...] ON DELETE referential_action > > which would override the constraints' action for the remainder of the > transaction. (Permission needs TBD, but probably the same as you > would need to create a new FK constraint on the relevant table.) > > In comparison to the original proposal, this'd force you to be explicit > about which constraint(s) you intend to override, but TBH I think that's > a good thing. I can see the argument for this in terms of being cautious/explicit about what gets removed, however the utility in this particular form was related to being able to *avoid* having to manually figure out the relationship chains and the specific constraints involved. Might there be some sort of middle ground here? > One big practical problem, which we've never addressed in the context of > SET CONSTRAINTS but maybe it's time to tackle, is that the SQL spec > defines the syntax like that because it thinks constraint names are > unique per-schema; thus a possibly-schema-qualified name is sufficient > ID. Of course we say that constraint names are only unique per-table, > so SET CONSTRAINTS has always had this issue of not being very carefully > targeted. I think we could do something like extending the syntax > to be > > SET CONSTRAINTS conname [ON tablename] [,...] new_properties This part seems reasonable. I need to look at how the existing SET CONSTRAINTS is implemented; would be interesting to see how the settings per-table/session are managed, as that would be illustrative to additional transient state like this. > Anyway, just food for thought --- I'm not necessarily set on any > of this. Sure thing; I appreciate even a little bit of your attention here. Best, David --
Hi, On Wed, Sep 29, 2021 at 03:55:22PM -0500, David Christensen wrote: > > I can see the argument for this in terms of being cautious/explicit about what gets removed, however > the utility in this particular form was related to being able to *avoid* having to manually figure out > the relationship chains and the specific constraints involved. Might there be some sort of middle > ground here? > [...] > > I think we could do something like extending the syntax to be > > > > SET CONSTRAINTS conname [ON tablename] [,...] new_properties > > This part seems reasonable. I need to look at how the existing SET CONSTRAINTS is implemented; > would be interesting to see how the settings per-table/session are managed, as that would be > illustrative to additional transient state like this. The cfbot reports that this patch doesn't apply anymore: http://cfbot.cputube.org/patch_36_3195.log > patching file src/backend/utils/adt/ri_triggers.c > Hunk #1 succeeded at 93 (offset 3 lines). > Hunk #2 FAILED at 181. > Hunk #3 succeeded at 556 (offset 5 lines). > Hunk #4 succeeded at 581 (offset 5 lines). > Hunk #5 succeeded at 755 (offset 5 lines). > Hunk #6 succeeded at 776 (offset 5 lines). > 1 out of 6 hunks FAILED -- saving rejects to file src/backend/utils/adt/ri_triggers.c.rej Are you currently working on a possibly different approach and/or grammar? If not, could you send a rebased patch? In the meantime I will switch the cf entry to Waiting on Author.
Hi, On Wed, Jan 12, 2022 at 04:57:27PM +0800, Julien Rouhaud wrote: > > The cfbot reports that this patch doesn't apply anymore: > http://cfbot.cputube.org/patch_36_3195.log > > > patching file src/backend/utils/adt/ri_triggers.c > > Hunk #1 succeeded at 93 (offset 3 lines). > > Hunk #2 FAILED at 181. > > Hunk #3 succeeded at 556 (offset 5 lines). > > Hunk #4 succeeded at 581 (offset 5 lines). > > Hunk #5 succeeded at 755 (offset 5 lines). > > Hunk #6 succeeded at 776 (offset 5 lines). > > 1 out of 6 hunks FAILED -- saving rejects to file src/backend/utils/adt/ri_triggers.c.rej > > Are you currently working on a possibly different approach and/or grammar? If > not, could you send a rebased patch? In the meantime I will switch the cf > entry to Waiting on Author. It's been almost 4 months since your last email, and almost 2 weeks since the notice that this patch doesn't apply anymore. Without update in the next couple of days this patch will be closed as Returned with Feedback per the commitfest rules.
Hi, On Tue, Jan 25, 2022 at 10:26:53PM +0800, Julien Rouhaud wrote: > > It's been almost 4 months since your last email, and almost 2 weeks since the > notice that this patch doesn't apply anymore. Without update in the next > couple of days this patch will be closed as Returned with Feedback per the > commitfest rules. Closed.
On Sun, Jan 30, 2022 at 9:47 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
Hi,
On Tue, Jan 25, 2022 at 10:26:53PM +0800, Julien Rouhaud wrote:
>
> It's been almost 4 months since your last email, and almost 2 weeks since the
> notice that this patch doesn't apply anymore. Without update in the next
> couple of days this patch will be closed as Returned with Feedback per the
> commitfest rules.
Closed.
David
Hi, On Mon, Jan 31, 2022 at 09:14:21AM -0600, David Christensen wrote: > > Sounds good; when I get time to look at this again I will resubmit (if > people think the base functionality is worth it, which is still a topic of > discussion). Yes, please do! Sorry I should have mentioned it, if a patch is closed as Return with Feedback it means that the feature is wanted, which was my understanding backlogging this thread, so submitting in some later commit fest is expected.