Hi Alvaro, On 2019/04/02 5:03, Alvaro Herrera wrote: > On 2019-Mar-29, Jesper Pedersen wrote: > >> I ran my test cases for this feature, and havn't seen any issues. >> >> Therefore I'm marking 1877 as Ready for Committer. If others have additional >> feedback feel free to switch it back. > > Thanks! > > I found two issues today. One, server side, is that during cloning for > partition attach we were not checking for concurrent deletion of > referenced tuples in partitions. I added an isolation spec test for > this. To fix the bug, added a find_all_inheritors() to lock all > partitions with ShareRowExclusiveLock. > > Another is that psql's \d failed for versions < 12, because we were > inconditionally adding an "AND conparentid = 0" clause. > > I also reworked CloneForeignKeyConstraints. The previous style was > being forced by the old recursing method; now we can make it a lot > simpler -- it's now just two subroutine calls. > > I'm satisfied with this patch now, so I intend to push early tomorrow.
I tried the latest patch and found no problems as far as I tried. I guess it's OK for the time being that users will not be able to drop a partition while a foreign key is pointing to it, given the troubles with that pre-DROP check. (Of course, someone may want to improve dependency.c in the future so that we can allow the DROP where possible.) Thanks for working on this. Off-topic: I did some performance testing, which has no relation to the code being changed by this patch, but thought the numbers hint at a need for improving other areas that affect the scalability (in terms of number of partitions) of the foreign key checks. * Table setup: Create a hash partitioned table with N partitions, followed by a regular table referencing the aforementioned partitioned table. Insert keys 1-1000 into ht, the primary key table. N: 2..8192 drop table ht cascade; create table ht (a int primary key, b int, c int) partition by hash (a); select 'create table ht' || x::text || ' partition of ht for values with (MODULUS N, REMAINDER ' || (x)::text || ');' from generate_series(0, N-1) x; \gexec drop table foo cascade; create table foo (a int references ht); truncate ht cascade; insert into ht select generate_series(1, 1000); * pgbench script (insert-foo.sql) \set a random(1, 1000) insert into foo values (:a); * Run pgbench pgbench -n -T 120 -f insert-foo.sql * TPS with plan_cache_mode = auto (default) or force_custom_plan 2 2382.779742 <= 8 2392.514952 32 2392.682178 128 2387.828361 512 2358.325865 1024 2234.699889 4096 2030.610062 8192 1740.633117 * TPS with plan_cache_mode = force_generic_plan 2 2924.030727 <= 8 2854.460937 32 2378.630989 128 1550.235166 512 642.980148 1024 330.142430 4096 71.739272 8192 32.620403 It'd be good to work on improving the scalability of generic plan execution in the future, because not having to re-plan the query used by RI_FKey_check would always be better, as seen by comparing execution times for 2 partitions (2382 tps with custom plan vs. 2924 tps with generic plan.). Having run-time pruning already helps quite a lot, but some other ideas are needed, such as one proposed by David recently, but withdrawn for PG 12 [1]. Anyway, nothing here that prevents this patch from being committed. :) Thanks for reading. Regards, Amit [1] https://commitfest.postgresql.org/22/1897/
