Re: partitioned tables referenced by FKs

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/