Re: Foreign keys and partitioned tables

While adding some more tests for the "action" part (i.e. updates and
deletes on the referenced table) I came across a bug that was causing
the server to crash ... but it's actually a preexisting bug in an
assert. The fix is in 0001.

0002 I already posted: don't clone row triggers that are declared
internal. As you noted, there is no test that changes because of this.
I haven't tried yet; the only case that comes to mind is doing something
with a deferred unique constraint. Anyway, it was clear to me from the
beginning that cloning internal triggers was not going to work for the
FK constraints.

0003 is the main patch, which is a bit changed from v4, notably to cover
your review comments:

Peter Eisentraut wrote:

> > - tables and permanent tables.
> > + tables and permanent tables. Also note that while it is permitted to
> > + define a foreign key on a partitioned table, declaring a foreign key
> > + that references a partitioned table is not allowed.
> > <para>
>
> Maybe use "possible" or "supported" instead of "allowed" and "permitted"
> in this and similar cases.

Fixed.

> @@ -7226,12 +7235,23 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab,
> Relation rel,
> * numbers)
> */
> if (pkrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + {
> + /* fix recursion in ATExecValidateConstraint to enable this case */
> + if (fkconstraint->skip_validation && !fkconstraint->initially_valid)
> + ereport(ERROR,
> + (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("cannot add NOT VALID foreign key to
> relation \"%s\"",
> + RelationGetRelationName(pkrel))));
> + }
>
> Maybe this error message should be more along the lines of "is not
> supported yet".

I added errdetail("This feature is not yet supported on partitioned tables.")))

> Also, this restriction should perhaps be mentioned in
> the documentation additions that we have been discussing.

Added a note in ALTER TABLE.

>
> The first few hunks in ri_triggers.c (the ones that refer to the
> pktable) are apparently for the postponed part of the feature, foreign
> keys referencing partitioned tables. So I think those hunks should be
> dropped from this patch.

Yeah, reverted.

> The removal of the ONLY for the foreign key queries also affects
> inherited tables, in undesirable ways. For example, consider this
> script: [...]

> With the patch, this will have deleted the (111, 1) record in test2a,
> which it did not do before.
>
> I think the trigger functions need to look up whether the table is a
> partitioned table and decide whether the use ONLY based on that.
> (This will probably also apply to the PK side, when that is
> implemented.)

Updated this. I added you test script to inherit.sql.

>
> In pg_dump, maybe this can be refined:
>
> + /*
> + * For partitioned tables, it doesn't work to emit constraints
> as not
> + * inherited.
> + */
> + if (tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
> + only = "";
> + else
> + only = "ONLY ";
>
> We use ONLY for foreign keys on inherited tables, but foreign keys are
> not inherited anyway, so this is at best future-proofing. We could
> just drop the ONLY unconditionally. Or at least explain this more.

Yeah, I admit this is a bit weird. I expanded the comment but didn't
change the code:

/*
* Foreign keys on partitioned tables are always declared as inheriting
* to partitions; for all other cases, emit them as applying ONLY
* directly to the named table, because that's how they work for
* regular inherited tables.
*/

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services