Hi Rahila,
Thanks for the review.
On Thu, Feb 18, 2021 at 7:08 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
>> Here is an updated version of the patch with some cosmetic changes
>> from the previous version. I moved the code being added to
>> AfterTriggerSaveEvent() and ExecUpdate() into separate subroutines to
>> improve readability, hopefully.
>>
> I tested these patches.
>
> It works as expected in case of cross partition updates, by correctly updating the
> referencing table. It works fine for ON UPDATE SET NULL and SET DEFAULT options as well.
> Also, tested by having a table reference only a partition and not the parent. In this case, the delete
> trigger is correctly called when the row is moved out of referenced partition.
I assume these are comments for the v3-0001 & v3-0002 patches...
> The partition-key-update-1.spec test fails with the following error message appearing in the diffs.
>
> step s1u3pc: UPDATE foo_range_parted SET a=11 WHERE a=7;
> +ERROR: cannot move row being updated to another partition
...whereas, this error happens with the patch I posted in my last
email (prevent-row-movement-on-pk-table.patch) that is not meant to be
considered for HEAD, but for back-branches (if at all). I also see
that cfbot got triggered by it and shows the same failure. I am not
going to try to take care of these failures unless we want to do
something in the back-branches.
To be clear, patches for HEAD do pass make check-world.
> I think the documentation update is missing from the patches.
Hmm, I don't think we document the behavior that is improved by the v3
patches as a limitation of any existing feature, neither of foreign
keys referencing partitioned tables nor of the update row movement
feature. So maybe there's nothing in the existing documentation that
is to be updated.
However, the patch does add a new error message for a case that the
patch doesn't handle, so maybe we could document that as a limitation.
Not sure if in the Notes section of the UPDATE reference page which
has some notes on row movement or somewhere else. Do you have
suggestions?
Attaching rebased version of the patches for HEAD to appease the cfbot.
--
Amit Langote
EDB: http://www.enterprisedb.com
