Re: partitioned tables referenced by FKs

Hi Alvaro,

I looked at the latest patch and most of the issues/bugs that I was going
to report based on the late January version of the patch seem to have been
taken care of; sorry that I couldn't post sooner which would've saved you
some time. The patch needs to be rebased on top of ff11e7f4b9 which
touched ri_triggers.c.

In the following case:

create table pk (a int primary key) partition by list (a);
create table pk1 (a int primary key);
create table fk (a int references pk1);

-- adds FK referencing pk1 via ATAddForeignKeyConstraint recursion
alter table pk attach partition pk1 for values in (1);
alter table fk add foreign key (a) references pk;

or:

-- adds FK referencing pk1 via CloneForeignKeyConstraints
alter table fk add foreign key (a) references pk;
alter table pk attach partition pk1 for values in (1);

\d fk
Table "public.fk"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
Foreign-key constraints:
"fk_a_fkey" FOREIGN KEY (a) REFERENCES pk1(a)
"fk_a_fkey1" FOREIGN KEY (a) REFERENCES pk(a)
"fk_a_fkey2" FOREIGN KEY (a) REFERENCES pk1(a)

Could we have avoided creating fk_a_fkey2 which must be same as fk_a_fkey
as far as the user-visible behavior is concerned?

* Regarding 0003

I'd like to hear your thoughts on some suggestions to alter the structure
of the reorganized code around foreign key addition/cloning. With this
patch adding support for foreign keys to reference partitioned tables, the
code now has to consider various cases due to the possibility of having
partitioned tables on both sides of a foreign key, which is reflected in
the complexity of the new code. Following functions have been introduced
in the foreign key DDL code in the course of adding support for partitioning:

addFkRecurseReferenced
addFkRecurseReferencing
CloneForeignKeyConstraints
CloneFkReferencing
CloneFkReferenced
tryAttachPartitionForeignKey

We have addFkRecurseReferencing and CloneForeignKeyConstraints calling
CloneFkReferencing to recursively add a foreign key constraint being
defined on (or being cloned to) a partitioned table to its partitions.
The latter invokes tryAttachPartitionForeignKey to see if an existing
foreign key constraint is functionally same as one being cloned to avoid
creating a duplicate constraint.

However, we have CloneFkReferenced() calling addFkRecurseReferenced, not
the other way around. Neither of these functions attempts to reuse an
existing, functionally equivalent, foreign key constraint referencing a
given partition that's being recursed to. So we get the behavior in the
above example, which again, I'm not sure is intentional.

Also, it seems a bit confusing that there is a CreateConstraintEntry call
in addFkRecurseReferenced() which is creating a constraint on the
*referencing* relation, which I think should be in
ATAddForeignKeyConstraint, the caller. I know we need to create a copy of
the constraint to reference the partitions of the referenced table, but we
might as well put it in CloneFkReferenced and reverse who calls who --
make addFkRecurseReferenced() call CloneFkReferenced and have the code to
create the cloned constraint and action triggers in the latter. That will
make the code to handle different sides of foreign key look similar, and
imho, easier to follow.

+/*
+ * addFkRecurseReferenced
+ * recursive subroutine for ATAddForeignKeyConstraint, referenced side

How about:

Subroutine for ATAddForeignKeyConstraint to add action trigger on
referenced relation, recursing if partitioned, in which case, both the
constraint referencing a given partition and the action trigger are cloned
in all partitions

+/*
+ * addFkRecurseReferenced
+ * recursive subroutine for ATAddForeignKeyConstraint, referenced side

How about:

Subroutine for ATAddForeignKeyConstraint to add check trigger on
referencing relation, recursing if partitioned, in which case, both the
constraint and the check trigger are cloned in all partitions

I noticed however that this function itself is not recursively called;
CloneFkReferencing handles the recursion.

/*
* CloneForeignKeyConstraints
* Clone foreign keys from a partitioned table to a newly acquired
* partition.

Maybe:

Clone foreign key of (or referencing) a partitioned table to a newly
acquired partition

* In 0002,

/*
+ * Return the OID of the index, in the given partition, that is a child
of the
+ * given index or InvalidOid if there isn't one.
+ */
+Oid
+index_get_partition(Relation partition, Oid indexId)

Maybe add a comma between "...given index" and "or InvalidOid", or maybe
rewrite the sentence as:

Return the OID of index of the given partition that is a child of the
given index, or InvalidOid if there isn't one.

* Unrelated to this thread, but while reviewing, I noticed this code in
ATExecAttachPartitionIdx:

/* no deadlock risk: RangeVarGetRelidExtended already acquired the lock */
partIdx = relation_open(partIdxId, AccessExclusiveLock);

/* we already hold locks on both tables, so this is safe: */
parentTbl = relation_open(parentIdx->rd_index->indrelid, AccessShareLock);
partTbl = relation_open(partIdx->rd_index->indrelid, NoLock);

I wonder why not NoLock in *all* of these relation_open calls? As
the comment says, RangeVarGetRelidExtended already got the lock for
'partIdxId'. Then there must already be a lock on 'parentTbl' as it's the
target relation of the ALTER INDEX command (actually the target index's
table).

Thanks,
Amit