On Fri, Feb 19, 2021 at 5:04 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Mon, Feb 15, 2021 at 10:37 PM Amit Langote <amitlangot...@gmail.com> wrote: > > Regarding the patch, I would have liked if it only prevented the > > update when the foreign key that would be violated by the component > > delete references the update query's main target relation. If the > > foreign key references the source partition directly, then the delete > > would be harmless. However there's not enough infrastructure in v12, > > v13 branches to determine that, which makes back-patching this a bit > > hard. For example, there's no way to tell in the back-branches if the > > foreign-key-enforcing triggers of a leaf partition have descended from > > the parent table. IOW, I am not so sure anymore if we should try to > > do anything in the back-branches. > > Hmm I'm not sure the necessity of figuring out foreign-key-enforcing > triggers of a leaf partition have descended from the parent table. I > might be missing something but even if the foreign key references the > leaf partition directly, the same problem could happen if doing a > cross-partition update, is that right?
Actually not, because in that case the referencing relations only care about the contents of that leaf partition, so it's okay that the ON DELETE actions are performed in that case, or IOW, no need to abort the update. Contrast that with when the foreign key references the parent table being updated, where both the old and the new row logically belong to the table being referenced, so performing ON DELETE actions using the source leaf partition's trigger is wrong. Does that make sense? > Also, the updated patch prevents a cross-partition update even when > the foreign key references another column of itself. This kind of > cross-update works as expected for now so it seems not to need to > disallow that. What do you think? The scenario is: > > create table p (a int primary key, b int references p(a) on delete > cascade) partition by list (a); > create table p1 partition of p for values in (1); > create table p2 partition of p for values in (2); > insert into p values (1, 1); > update p set a = 2, b = 2 where a = 1; > ERROR: cannot move row being updated to another partition > DETAIL: Moving the row may cause a foreign key involving the source > partition to be violated. Hmm yes, it would be nice to not cause an error in this case. But we don't have enough details about the actual foreign key in this part of the code (ExecUpdate). Given the current ResultRelInfo infrastructure, this code can only look at the trigger objects to get some details about the foreign key, such as whether the relation being operated on is the FK relation or the PK relation. More detailed properties of those foreign keys are only checked inside the trigger's functions (ri_trigger.c) and perhaps also in the dispatch code in trigger.c: AfterTriggerSaveEvent(). However, it would be hard to detect in those modules that a delete trigger event indeed comes from a delete performed as part of a cross-partition update, that is, without significantly changing their interfaces. Anyway, I am no longer sure if we should do anything in the back branches, which the patch you have been looking at is for. There have not been many field complaints about this other than the one that started this thread. Also, I suspect that aborting the cross-partition updates for any partitioned table that is referenced in a foreign key will annoy users, especially those who simply don't use ON DELETE/UPDATE clauses. So, it may be better to focus on the patches for master that, instead of giving an error, make the cross-partition updates just work. -- Amit Langote EDB: http://www.enterprisedb.com
