[PATCH] Support for foreign keys with arrays

Started by Gabriele Bartoliniover 14 years ago62 messageshackers

gabriele.bartolini@2ndQuadrant.it

over 14 years ago

This patch adds basic support of arrays in foreign keys, by allowing to
define a referencing column as an array of elements having the same type
as the referenced column in the referenced table.
Every NOT NULL element in the referencing array is matched against the
referenced table.

Example:

CREATE TABLE pt (
id INTEGER PRIMARY KEY,
...
);

CREATE TABLE ft (
id SERIAL PRIMARY KEY,
pids INTEGER[] REFERENCES pt,
...
);

This patch is for discussion and has been built against HEAD.
It compiles and passes all regressions tests (including specific ones -
see the src/test/regress/sql/foreign_key.sql file).
Empty arrays, multi-dimensional arrays, duplicate elements and NULL
values are allowed.

We had to enforce some limitations, due to the lack (yet) of a clear and
universally accepted behaviour and strategy.
For example, consider the ON DELETE action on the above tables: in case
of delete of a record in the 'pt' table, should we remove the whole row
or just the values from the array?
We hope we can start a discussion from here.

Current limitations:

* Only arrays of the same type as the primary key in the referenced
table are supported
* multi-column foreign keys are not supported (only single column)
* Only RESTRICT and NO ACTION methods for referential integrity
enforcement are currently supported

TODO:
* Improve check for empty arrays, which might interfere with SSI (see below)
* Verify interaction with serializable transactions

AUTHORS:
* Gabriele Bartolini <gabriele.bartolini@2ndQuadrant.it>
* Marco Nenciarini <marco.nenciarini@2ndQuadrant.it>

Cheers,
Gabriele (and Marco)

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele.bartolini@2ndQuadrant.it | www.2ndQuadrant.it

Noah Misch

noah@leadboat.com

over 14 years ago

In reply to: Gabriele Bartolini (#1)

Re: [PATCH] Support for foreign keys with arrays

Hi Gabriele,

On Fri, Nov 04, 2011 at 01:48:02PM +0100, Gabriele Bartolini wrote:

CREATE TABLE pt (
id INTEGER PRIMARY KEY,
...
);

CREATE TABLE ft (
id SERIAL PRIMARY KEY,
pids INTEGER[] REFERENCES pt,
...
);

This seems useful.

I'm assuming the SQL spec says nothing about a feature like this?

This patch is for discussion and has been built against HEAD.
It compiles and passes all regressions tests (including specific ones -
see the src/test/regress/sql/foreign_key.sql file).
Empty arrays, multi-dimensional arrays, duplicate elements and NULL
values are allowed.

With this patch, RI_Initial_Check does not detect a violation in an array that
contains at least one conforming element:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[]);
INSERT INTO parent VALUES (1);
INSERT INTO child VALUES ('{3,1,2}');
ALTER TABLE child ADD FOREIGN KEY (c) REFERENCES parent; -- should error
INSERT INTO child VALUES ('{3,1,2}'); -- does error, as expected
ROLLBACK;

The error message DETAIL on constraint violation would benefit from
array-FK-specific language. Example of current message:

ERROR: insert or update on table "child" violates foreign key constraint "child_c_fkey"
DETAIL: Key (c)=({3,1,2}) is not present in table "parent".

The patch is missing a change to the code that does FK=FK checks when a user
updates the FK side:

\set VERBOSITY verbose
BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[] REFERENCES parent);
INSERT INTO parent VALUES (1);
INSERT INTO child VALUES ('{1,1}');
COMMIT;
-- ERROR: XX000: no conversion function from integer[] to integer
-- LOCATION: ri_HashCompareOp, ri_triggers.c:4097
UPDATE child SET c = '{1,1}';
DROP TABLE parent, child;
COMMIT;

Please audit each ri_triggers.c entry point for further problems like this.

We had to enforce some limitations, due to the lack (yet) of a clear and
universally accepted behaviour and strategy.
For example, consider the ON DELETE action on the above tables: in case
of delete of a record in the 'pt' table, should we remove the whole row
or just the values from the array?
We hope we can start a discussion from here.

Removing values from the array seems best to me. There's no doubt about what
ON UPDATE CASCADE should do, and having ON DELETE CASCADE excise individual
array elements is consistent with that. It's less clear for SET NULL, but I'd
continue with a per-element treatment. I'd continue to forbid SET DEFAULT.

However, Jeff Davis did expect ON DELETE CASCADE to remove entire rows:
http://archives.postgresql.org/message-id/1288119207.15279.24.camel@jdavis-ux.asterdata.local
So, perhaps the behavior needs to be user-selectable.

Current limitations:

* Only arrays of the same type as the primary key in the referenced
table are supported

This is understandable for a WIP, but the final patch will need to use our
existing, looser foreign key type match requirement.

* multi-column foreign keys are not supported (only single column)

Any particular reason for this?

*** a/doc/src/sgml/ddl.sgml
--- b/doc/src/sgml/ddl.sgml
***************
*** 764,769 **** CREATE TABLE order_items (
--- 764,796 ----
the last table.
</para>

+    <para>
+     Another option you have with foreign keys is to use a referencing column
+     which is an array of elements with the same type as the referenced column
+     in the related table. This feature, also known as <firstterm>foreign key arrays</firstterm>,
+     is described in the following example:

Please wrap your documentation paragraphs.

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 5705,5710 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5705,5735 ----
Oid			ffeqop;
int16		eqstrategy;

+ 		/* Check if foreign key is an array of primary key types */
+ 		const bool		is_foreign_key_array = (fktype == get_array_type (pktype));

We don't declare non-pointer, local variables "const". Also, [not positive on
this one] when an initial assignment requires a comment, declare the variable
with no assignment and no comment. Then, assign it later with the comment.
This keeps the per-block declarations packed together.

This test wrongly rejects FK types that are domains over the array type:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE DOMAIN intarrdom AS int[];
CREATE TABLE child (c intarrdom REFERENCES parent);
ROLLBACK;

+ 
+ 		/* Enforce foreign key array restrictions */
+ 		if (is_foreign_key_array)
+ 		{
+ 			/*
+ 			 * Foreign key array must not be part of a multi-column foreign key
+ 			 */
+ 			if (is_foreign_key_array && numpks > 1)
+ 				ereport(ERROR,
+ 					(errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ 					errmsg("foreign key arrays must not be part of a multi-column foreign key")));
+ 
+ 			/*
+ 			 * We have to restrict foreign key array to NO ACTION and RESTRICT mode
+ 			 * until the behaviour triggered by the other actions is clearer and well defined
+ 			 */
+ 			if ((fkconstraint->fk_upd_action != FKCONSTR_ACTION_NOACTION && fkconstraint->fk_upd_action != FKCONSTR_ACTION_RESTRICT)
+ 				|| (fkconstraint->fk_del_action != FKCONSTR_ACTION_NOACTION && fkconstraint->fk_del_action != FKCONSTR_ACTION_RESTRICT))

Break these lines to keep things within 78 columns. Audit the remainder of
your changes for long lines, and break when in doubt.

+ 				ereport(ERROR,
+ 					(errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ 					errmsg("NO ACTION and RESTRICT are the only supported actions for foreign key arrays")));

Error message constants can remain unbroken, though.

+ }
+
/* We need several fields out of the pg_opclass entry */
cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i]));
if (!HeapTupleIsValid(cla_ht))
***************
*** 5766,5772 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
Oid target_typeids[2];
input_typeids[0] = pktype;
! 			input_typeids[1] = fktype;
target_typeids[0] = opcintype;
target_typeids[1] = opcintype;
if (can_coerce_type(2, input_typeids, target_typeids,
--- 5791,5801 ----
Oid			target_typeids[2];
input_typeids[0] = pktype;
! /* When is FKA we must use for FK the same type of PK */
! if (is_foreign_key_array)
! input_typeids[1] = pktype;
! else
! input_typeids[1] = fktype;
target_typeids[0] = opcintype;
target_typeids[1] = opcintype;
if (can_coerce_type(2, input_typeids, target_typeids,

This is bogus; the can_coerce_type test will always pass (excluding bad cases
of catalog inconsistency).

ATAddForeignKeyConstraint should choose to make an array foreign key whenever
the PK side is a scalar and the FK side is an array. Then, grab the element
type of the FK side and feed that through the operator-identification logic.

*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
***************
*** 460,465 **** RI_FKey_check(PG_FUNCTION_ARGS)
--- 460,466 ----
char		paramname[16];
const char *querysep;
Oid			queryoids[RI_MAX_NUMKEYS];
+ 		bool		is_foreign_key_array = false;
/* ----------
* The query string built is
***************
*** 476,493 **** RI_FKey_check(PG_FUNCTION_ARGS)
{
Oid pk_type = RIAttType(pk_rel, riinfo.pk_attnums[i]);
Oid fk_type = RIAttType(fk_rel, riinfo.fk_attnums[i]);

quoteOneName(attname,
RIAttName(pk_rel, riinfo.pk_attnums[i]));
sprintf(paramname, "$%d", i + 1);
! ri_GenerateQual(&querybuf, querysep,
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
querysep = "AND";
queryoids[i] = fk_type;
}
! appendStringInfo(&querybuf, " FOR SHARE OF x");
/* Prepare and save the plan */
qplan = ri_PlanCheck(querybuf.data, riinfo.nkeys, queryoids,
--- 477,524 ----
{
Oid			pk_type = RIAttType(pk_rel, riinfo.pk_attnums[i]);
Oid			fk_type = RIAttType(fk_rel, riinfo.fk_attnums[i]);
+ 			is_foreign_key_array = (fk_type == get_array_type (pk_type));

Drop the extra whitespace before the function argument list.

quoteOneName(attname,
RIAttName(pk_rel, riinfo.pk_attnums[i]));
sprintf(paramname, "$%d", i + 1);
! /*
! * In case of an array foreign key, we check that every
! * DISTINCT NOT NULL value in the array is present in the PK table.
! * XXX: This works because the query is executed with LIMIT 1,

I found this comment confusing, since the SQL syntax "LIMIT 1" is never used
here. I suppose you're referring to the fact that we call into SPI with
tcount = 1?

! * but may not work properly with SSI (a better approach would be
! * to inspect the array and skip the check in case of empty arrays).

Why might serializable transactions be specially affected?

! */
! if (is_foreign_key_array)
! {
! appendStringInfo(&querybuf, " %s (SELECT count(*) FROM (SELECT DISTINCT UNNEST(%s)) y WHERE y IS NOT NULL)", querysep, paramname);
! appendStringInfo(&querybuf, " = (SELECT count(*) FROM (SELECT 1 FROM ONLY %s y", pkrelname);
! ri_GenerateQual(&querybuf, "WHERE",
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
! /*
! * We lock for share every row in the pkreltable that is
! * referenced by the array elements
! */
! appendStringInfo(&querybuf, " FOR SHARE OF y) z)");

The resulting query performs an irrelevant sequential scan on the PK table:

SELECT 1 FROM ONLY "public"."parent" x WHERE (SELECT count(*) FROM (SELECT DISTINCT UNNEST($1)) y WHERE y IS NOT NULL) = (SELECT count(*) FROM (SELECT 1 FROM ONLY "public"."parent" y WHERE "c" OPERATOR(pg_catalog.=) ANY ($1) FOR SHARE OF y) z)

As you suggested with that comment above, this scan always ends after one row.
That places a bound on the actual performance hit. However, we still read the
one row, which may mean loading a page for nothing. At a minimum, simplify
this query to:

SELECT 1 WHERE (SELECT count(*) FROM (SELECT DISTINCT UNNEST($1)) y WHERE y IS NOT NULL) = (SELECT count(*) FROM (SELECT 1 FROM ONLY "public"."parent" y WHERE "c" OPERATOR(pg_catalog.=) ANY ($1) FOR SHARE OF y) z)

That also naturally handles empty arrays against empty PK tables, which
currently fail for me even at READ COMMITTED:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[] REFERENCES parent);
INSERT INTO child VALUES ('{}'); -- fails wrongly
ROLLBACK;

! }
! else
! {
! ri_GenerateQual(&querybuf, querysep,
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
! }
querysep = "AND";
queryoids[i] = fk_type;
}
! /*
! * We skip locking for share in case of foreign key arrays
! * as it has been done in the inner subquery
! */
! if (! is_foreign_key_array)

Drop the whitespace after the "!".

! appendStringInfo(&querybuf, " FOR SHARE OF x");

/* Prepare and save the plan */
qplan = ri_PlanCheck(querybuf.data, riinfo.nkeys, queryoids,

*** a/src/test/regress/expected/foreign_key.out
--- b/src/test/regress/expected/foreign_key.out
***************
*** 968,978 **** drop table pktable;
drop table pktable_base;
-- 2 columns (1 table), mismatched types
create table pktable_base(base1 int not null, base2 int);
- create table pktable(ptest1 inet, ptest2 inet[], primary key(base1, ptest1), foreign key(base2, ptest2) references
-                                              pktable(base1, ptest1)) inherits (pktable_base);
- NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
- ERROR:  foreign key constraint "pktable_base2_fkey" cannot be implemented
- DETAIL:  Key columns "ptest2" and "ptest1" are of incompatible types: inet[] and inet.

Instead of deleting this test, change the type from inet[] to something
unrelated, like float8.

Thanks,
nm

Pavel Stehule

pavel.stehule@gmail.com

over 14 years ago

In reply to: Noah Misch (#2)

Re: [PATCH] Support for foreign keys with arrays

Hello

2011/11/17 Noah Misch <noah@leadboat.com>:

Hi Gabriele,

On Fri, Nov 04, 2011 at 01:48:02PM +0100, Gabriele Bartolini wrote:

CREATE TABLE pt (
id INTEGER PRIMARY KEY,
...
);

CREATE TABLE ft (
id SERIAL PRIMARY KEY,
pids INTEGER[] REFERENCES pt,
...
);

This seems useful.

will be supported situation

CREATE TABLE main(
id int[] PRIMARY KEY,
...
);

CREATE TABLE child(
main_id int[] REFERENCES main(id),

Regards

Pavel Stehule

Show quoted text

I'm assuming the SQL spec says nothing about a feature like this?

This patch is for discussion and has been built against HEAD.
It compiles and passes all regressions tests (including specific ones -
see the src/test/regress/sql/foreign_key.sql file).
Empty arrays, multi-dimensional arrays, duplicate elements and NULL
values are allowed.

With this patch, RI_Initial_Check does not detect a violation in an array that
contains at least one conforming element:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[]);
INSERT INTO parent VALUES (1);
INSERT INTO child VALUES ('{3,1,2}');
ALTER TABLE child ADD FOREIGN KEY (c) REFERENCES parent; -- should error
INSERT INTO child VALUES ('{3,1,2}'); -- does error, as expected
ROLLBACK;

The error message DETAIL on constraint violation would benefit from
array-FK-specific language. Example of current message:

ERROR: insert or update on table "child" violates foreign key constraint "child_c_fkey"
DETAIL: Key (c)=({3,1,2}) is not present in table "parent".

The patch is missing a change to the code that does FK=FK checks when a user
updates the FK side:

\set VERBOSITY verbose
BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[] REFERENCES parent);
INSERT INTO parent VALUES (1);
INSERT INTO child VALUES ('{1,1}');
COMMIT;
-- ERROR: XX000: no conversion function from integer[] to integer
-- LOCATION: ri_HashCompareOp, ri_triggers.c:4097
UPDATE child SET c = '{1,1}';
DROP TABLE parent, child;
COMMIT;

Please audit each ri_triggers.c entry point for further problems like this.

We had to enforce some limitations, due to the lack (yet) of a clear and
universally accepted behaviour and strategy.
For example, consider the ON DELETE action on the above tables: in case
of delete of a record in the 'pt' table, should we remove the whole row
or just the values from the array?
We hope we can start a discussion from here.

Removing values from the array seems best to me. There's no doubt about what
ON UPDATE CASCADE should do, and having ON DELETE CASCADE excise individual
array elements is consistent with that. It's less clear for SET NULL, but I'd
continue with a per-element treatment. I'd continue to forbid SET DEFAULT.

However, Jeff Davis did expect ON DELETE CASCADE to remove entire rows:
http://archives.postgresql.org/message-id/1288119207.15279.24.camel@jdavis-ux.asterdata.local
So, perhaps the behavior needs to be user-selectable.

Current limitations:

* Only arrays of the same type as the primary key in the referenced
table are supported

This is understandable for a WIP, but the final patch will need to use our
existing, looser foreign key type match requirement.

* multi-column foreign keys are not supported (only single column)

Any particular reason for this?
*** a/doc/src/sgml/ddl.sgml
--- b/doc/src/sgml/ddl.sgml
***************
*** 764,769 **** CREATE TABLE order_items (
--- 764,796 ----
      the last table.
     </para>
+    <para>
+     Another option you have with foreign keys is to use a referencing column
+     which is an array of elements with the same type as the referenced column
+     in the related table. This feature, also known as <firstterm>foreign key arrays</firstterm>,
+     is described in the following example:
Please wrap your documentation paragraphs.
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 5705,5710 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
--- 5705,5735 ----
              Oid                     ffeqop;
              int16           eqstrategy;
+             /* Check if foreign key is an array of primary key types */
+             const bool              is_foreign_key_array = (fktype == get_array_type (pktype));
We don't declare non-pointer, local variables "const". Also, [not positive on
this one] when an initial assignment requires a comment, declare the variable
with no assignment and no comment. Then, assign it later with the comment.
This keeps the per-block declarations packed together.

This test wrongly rejects FK types that are domains over the array type:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE DOMAIN intarrdom AS int[];
CREATE TABLE child (c intarrdom REFERENCES parent);
ROLLBACK;
+
+             /* Enforce foreign key array restrictions */
+             if (is_foreign_key_array)
+             {
+                     /*
+                      * Foreign key array must not be part of a multi-column foreign key
+                      */
+                     if (is_foreign_key_array && numpks > 1)
+                             ereport(ERROR,
+                                     (errcode(ERRCODE_INVALID_FOREIGN_KEY),
+                                     errmsg("foreign key arrays must not be part of a multi-column foreign key")));
+
+                     /*
+                      * We have to restrict foreign key array to NO ACTION and RESTRICT mode
+                      * until the behaviour triggered by the other actions is clearer and well defined
+                      */
+                     if ((fkconstraint->fk_upd_action != FKCONSTR_ACTION_NOACTION && fkconstraint->fk_upd_action != FKCONSTR_ACTION_RESTRICT)
+                             || (fkconstraint->fk_del_action != FKCONSTR_ACTION_NOACTION && fkconstraint->fk_del_action != FKCONSTR_ACTION_RESTRICT))
Break these lines to keep things within 78 columns. Audit the remainder of
your changes for long lines, and break when in doubt.
+                             ereport(ERROR,
+                                     (errcode(ERRCODE_INVALID_FOREIGN_KEY),
+                                     errmsg("NO ACTION and RESTRICT are the only supported actions for foreign key arrays")));
Error message constants can remain unbroken, though.
+ }
+
/* We need several fields out of the pg_opclass entry */
cla_ht = SearchSysCache1(CLAOID, ObjectIdGetDatum(opclasses[i]));
if (!HeapTupleIsValid(cla_ht))
***************
*** 5766,5772 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
Oid target_typeids[2];
                      input_typeids[0] = pktype;
!                     input_typeids[1] = fktype;
                      target_typeids[0] = opcintype;
                      target_typeids[1] = opcintype;
                      if (can_coerce_type(2, input_typeids, target_typeids,
--- 5791,5801 ----
                      Oid                     target_typeids[2];
input_typeids[0] = pktype;
! /* When is FKA we must use for FK the same type of PK */
! if (is_foreign_key_array)
! input_typeids[1] = pktype;
! else
! input_typeids[1] = fktype;
target_typeids[0] = opcintype;
target_typeids[1] = opcintype;
if (can_coerce_type(2, input_typeids, target_typeids,
This is bogus; the can_coerce_type test will always pass (excluding bad cases
of catalog inconsistency).

ATAddForeignKeyConstraint should choose to make an array foreign key whenever
the PK side is a scalar and the FK side is an array. Then, grab the element
type of the FK side and feed that through the operator-identification logic.
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
***************
*** 460,465 **** RI_FKey_check(PG_FUNCTION_ARGS)
--- 460,466 ----
              char            paramname[16];
              const char *querysep;
              Oid                     queryoids[RI_MAX_NUMKEYS];
+             bool            is_foreign_key_array = false;
/* ----------
* The query string built is
***************
*** 476,493 **** RI_FKey_check(PG_FUNCTION_ARGS)
{
Oid pk_type = RIAttType(pk_rel, riinfo.pk_attnums[i]);
Oid fk_type = RIAttType(fk_rel, riinfo.fk_attnums[i]);

quoteOneName(attname,
RIAttName(pk_rel, riinfo.pk_attnums[i]));
sprintf(paramname, "$%d", i + 1);
! ri_GenerateQual(&querybuf, querysep,
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
querysep = "AND";
queryoids[i] = fk_type;
}
! appendStringInfo(&querybuf, " FOR SHARE OF x");
              /* Prepare and save the plan */
              qplan = ri_PlanCheck(querybuf.data, riinfo.nkeys, queryoids,
--- 477,524 ----
              {
                      Oid                     pk_type = RIAttType(pk_rel, riinfo.pk_attnums[i]);
                      Oid                     fk_type = RIAttType(fk_rel, riinfo.fk_attnums[i]);
+                     is_foreign_key_array = (fk_type == get_array_type (pk_type));
Drop the extra whitespace before the function argument list.

quoteOneName(attname,
RIAttName(pk_rel, riinfo.pk_attnums[i]));
sprintf(paramname, "$%d", i + 1);
! /*
! * In case of an array foreign key, we check that every
! * DISTINCT NOT NULL value in the array is present in the PK table.
! * XXX: This works because the query is executed with LIMIT 1,

I found this comment confusing, since the SQL syntax "LIMIT 1" is never used
here. I suppose you're referring to the fact that we call into SPI with
tcount = 1?

! * but may not work properly with SSI (a better approach would be
! * to inspect the array and skip the check in case of empty arrays).

Why might serializable transactions be specially affected?

! */
! if (is_foreign_key_array)
! {
! appendStringInfo(&querybuf, " %s (SELECT count(*) FROM (SELECT DISTINCT UNNEST(%s)) y WHERE y IS NOT NULL)", querysep, paramname);
! appendStringInfo(&querybuf, " = (SELECT count(*) FROM (SELECT 1 FROM ONLY %s y", pkrelname);
! ri_GenerateQual(&querybuf, "WHERE",
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
! /*
! * We lock for share every row in the pkreltable that is
! * referenced by the array elements
! */
! appendStringInfo(&querybuf, " FOR SHARE OF y) z)");

The resulting query performs an irrelevant sequential scan on the PK table:

SELECT 1 FROM ONLY "public"."parent" x WHERE (SELECT count(*) FROM (SELECT DISTINCT UNNEST($1)) y WHERE y IS NOT NULL) = (SELECT count(*) FROM (SELECT 1 FROM ONLY "public"."parent" y WHERE "c" OPERATOR(pg_catalog.=) ANY ($1) FOR SHARE OF y) z)

As you suggested with that comment above, this scan always ends after one row.
That places a bound on the actual performance hit. However, we still read the
one row, which may mean loading a page for nothing. At a minimum, simplify
this query to:

SELECT 1 WHERE (SELECT count(*) FROM (SELECT DISTINCT UNNEST($1)) y WHERE y IS NOT NULL) = (SELECT count(*) FROM (SELECT 1 FROM ONLY "public"."parent" y WHERE "c" OPERATOR(pg_catalog.=) ANY ($1) FOR SHARE OF y) z)

That also naturally handles empty arrays against empty PK tables, which
currently fail for me even at READ COMMITTED:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[] REFERENCES parent);
INSERT INTO child VALUES ('{}'); -- fails wrongly
ROLLBACK;

! }
! else
! {
! ri_GenerateQual(&querybuf, querysep,
! attname, pk_type,
! riinfo.pf_eq_oprs[i],
! paramname, fk_type);
! }
querysep = "AND";
queryoids[i] = fk_type;
}
! /*
! * We skip locking for share in case of foreign key arrays
! * as it has been done in the inner subquery
! */
! if (! is_foreign_key_array)

Drop the whitespace after the "!".

! appendStringInfo(&querybuf, " FOR SHARE OF x");

/* Prepare and save the plan */
qplan = ri_PlanCheck(querybuf.data, riinfo.nkeys, queryoids,
*** a/src/test/regress/expected/foreign_key.out
--- b/src/test/regress/expected/foreign_key.out
***************
*** 968,978 **** drop table pktable;
  drop table pktable_base;
  -- 2 columns (1 table), mismatched types
  create table pktable_base(base1 int not null, base2 int);
- create table pktable(ptest1 inet, ptest2 inet[], primary key(base1, ptest1), foreign key(base2, ptest2) references
-                                              pktable(base1, ptest1)) inherits (pktable_base);
- NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
- ERROR:  foreign key constraint "pktable_base2_fkey" cannot be implemented
- DETAIL:  Key columns "ptest2" and "ptest1" are of incompatible types: inet[] and inet.
Instead of deleting this test, change the type from inet[] to something
unrelated, like float8.

Thanks,
nm

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Noah Misch (#2)

Re: [PATCH] Support for foreign keys with arrays

Noah Misch <noah@leadboat.com> writes:

On Fri, Nov 04, 2011 at 01:48:02PM +0100, Gabriele Bartolini wrote:

CREATE TABLE pt (
id INTEGER PRIMARY KEY,

CREATE TABLE ft (
id SERIAL PRIMARY KEY,
pids INTEGER[] REFERENCES pt,

I'm assuming the SQL spec says nothing about a feature like this?

I'm pretty certain that the SQL spec flat out forbids this.

The least we could do is invent some non-spec syntax that makes the
intention clear, rather than having the system assume that an error case
was intended to mean something else. Maybe

pids INTEGER[] ARRAY REFERENCES pt,

or something like that. (ARRAY is a fully reserved word already,
so I think this syntax should work, but I've not tried it.)

BTW, has anyone thought through whether this is a sane idea at all?
It seems to me to be full of cases that will require rather arbitrary
decisions, like whether ON DELETE CASCADE should involve deleting the
whole row or just one array element.

regards, tom lane

Josh Berkus

josh@agliodbs.com

over 14 years ago

In reply to: Tom Lane (#4)

Re: [PATCH] Support for foreign keys with arrays

BTW, has anyone thought through whether this is a sane idea at all?
It seems to me to be full of cases that will require rather arbitrary
decisions, like whether ON DELETE CASCADE should involve deleting the
whole row or just one array element.

One array element, presumably.

Does the patch implement CASCADE?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Pavel Stehule

pavel.stehule@gmail.com

over 14 years ago

In reply to: Tom Lane (#4)

Re: [PATCH] Support for foreign keys with arrays

2011/11/17 Tom Lane <tgl@sss.pgh.pa.us>:

Noah Misch <noah@leadboat.com> writes:

On Fri, Nov 04, 2011 at 01:48:02PM +0100, Gabriele Bartolini wrote:

CREATE TABLE pt (
id INTEGER PRIMARY KEY,

CREATE TABLE ft (
id SERIAL PRIMARY KEY,
pids INTEGER[] REFERENCES pt,

I'm assuming the SQL spec says nothing about a feature like this?

I'm pretty certain that the SQL spec flat out forbids this.

The least we could do is invent some non-spec syntax that makes the
intention clear, rather than having the system assume that an error case
was intended to mean something else. Maybe

pids INTEGER[] ARRAY REFERENCES pt,

or something like that. (ARRAY is a fully reserved word already,
so I think this syntax should work, but I've not tried it.)

Regards

Pavel Stehule

Show quoted text

BTW, has anyone thought through whether this is a sane idea at all?
It seems to me to be full of cases that will require rather arbitrary
decisions, like whether ON DELETE CASCADE should involve deleting the
whole row or just one array element.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Josh Berkus

josh@agliodbs.com

over 14 years ago

In reply to: Josh Berkus (#5)

Re: [PATCH] Support for foreign keys with arrays

Folks,

BTW, I don't want to block this patch. However, it occurs to me that a
more generalized FK based on non-equality conditions (i.e. expressions)
would be nice if it were possible. Then we could have FKs from all
kinds of complex structures.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Jaime Casanova

jcasanov@systemguards.com.ec

over 14 years ago

In reply to: Noah Misch (#2)

Re: [PATCH] Support for foreign keys with arrays

On Wed, Nov 16, 2011 at 11:28 PM, Noah Misch <noah@leadboat.com> wrote:

Removing values from the array seems best to me. There's no doubt about what
ON UPDATE CASCADE should do, and having ON DELETE CASCADE excise individual
array elements is consistent with that. It's less clear for SET NULL, but I'd
continue with a per-element treatment. I'd continue to forbid SET DEFAULT.

However, Jeff Davis did expect ON DELETE CASCADE to remove entire rows:
http://archives.postgresql.org/message-id/1288119207.15279.24.camel@jdavis-ux.asterdata.local
So, perhaps the behavior needs to be user-selectable.

i will agree with Jeff on this...

i mean, on the normal case it will delete the row. no?

the docs says about the CASCADE action
"""
CASCADE
Delete any rows referencing the deleted row, or update the value of
the referencing column to the new value of the referenced column,
respectively.
"""

so, that is what i will expect

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

Tom Lane

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Josh Berkus (#5)

Re: [PATCH] Support for foreign keys with arrays

Josh Berkus <josh@agliodbs.com> writes:

BTW, has anyone thought through whether this is a sane idea at all?
It seems to me to be full of cases that will require rather arbitrary
decisions, like whether ON DELETE CASCADE should involve deleting the
whole row or just one array element.

One array element, presumably.

Um, why? One reasonable interpretation of an array reference is that
the row depends on *all* of the referenced pkeys. Also, if you do
delete one array element at a time, what do you do when the array
becomes empty --- delete the row, or not, and in each case what's your
semantic justification for that choice?

In short, "presumably" doesn't cut it here.

regards, tom lane

#10

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

over 14 years ago

In reply to: Noah Misch (#2)

Re: [PATCH] Support for foreign keys with arrays

Hi Noah,

thanks for your unvaluable review, rich of useful and thorough comments
and notes. Marco and myself will add your proposed tests as soon as
possible (most likely after the Italian PGDay which is this week).
However, given the feedback received from other developers too
(including Tom), I would first concentrate on defining the syntax and
how referential integrity actions should work.

Il 17/11/11 05:28, Noah Misch ha scritto:

Removing values from the array seems best to me. There's no doubt
about what ON UPDATE CASCADE should do, and having ON DELETE CASCADE
excise individual array elements is consistent with that. It's less
clear for SET NULL, but I'd continue with a per-element treatment. I'd
continue to forbid SET DEFAULT. However, Jeff Davis did expect ON
DELETE CASCADE to remove entire rows:
http://archives.postgresql.org/message-id/1288119207.15279.24.camel@jdavis-ux.asterdata.local
So, perhaps the behavior needs to be user-selectable.

I would agree with what Tom is saying here, given that SQL specs do not
say anything about this feature. We could leave standard REFERENCES
keyword handling the array value as it is now. If a user wants to take
advantage of in-array referential integrity, we could implement the
special keyword "ARRAY REFERENCES" as Tom proposes (or a similar keyword).

Consequently, we need to agree on what the actions on delete and update
operations are. In case of ARRAY REFERENCES, I would be inclined to
leave the same meaning of ROW scope actions to CASCADE and SET NULL
actions, while disallowing the SET DEFAULT action (as Noah suggests
too). At the same time, I would add two actions for ARRAY REFERENCES
which will be processing array elements:

* ARRAY CASCADE
* ARRAY SET NULL

(Of course if you are welcome to propose a better naming convention).
This table summarises the scope of the actions.

For instance, with an "ARRAY REFERENCES ... ON DELETE CASCADE", I would
expect that the whole row is deleted (as Jeff et al. say). However, if I
specify "ARRAY REFERENCES ... ON DELETE ARRAY CASCADE", I would expect
that elements in the referencing array are removed.
Similary the "ARRAY REFERENCES ... ON DELETE SET NULL" will set the row
to NULL, whereas "ARRAY REFERENCES ... ON DELETE ARRAY SET NULL" will
set individual elements in the referencing array to NULL.

In case of updates, SET NULL and ARRAY SET NULL works the same (updating
the whole row or the single elements). CASCADE and ARRAY CASCADE are
synonyms, as they would work in individual elements (which is the action
that makes more sense anyway).

I believe that, before we proceed with one implementation or another, it
is important we discuss this sort of things and agree on a possible
long-term path (so that we can organise intermediate deliverables).

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele.bartolini@2ndQuadrant.it | www.2ndQuadrant.it

#11

Gianni Ciolli

gianni.ciolli@2ndquadrant.it

over 14 years ago

In reply to: Gabriele Bartolini (#10)

Re: [PATCH] Support for foreign keys with arrays

Hi Gabriele and Marco,

On Sun, Nov 20, 2011 at 10:36:15AM +0100, Gabriele Bartolini wrote:

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Element |
SET NULL | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
SET DEFAULT | Error | Error |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

thank you for this very clear and concise summary!

I agree with your appeal for a broad discussion on the proposed
syntax, and I will use the same language to express my proposal (for
clarity and to simplify the discussion):

------------------ --------- ---------------
| ON | ON |
Action | DELETE | UPDATE |
------------------ --------- ---------------
CASCADE | Element | Element |
SET NULL | Element | Element |
SET DEFAULT | Error | Error |
ARRAY CASCADE | Row | Element = Row |
ARRAY SET NULL | Row | Row |
ARRAY SET DEFAULT | Row | Row |
NO ACTION | - | - |
RESTRICT | - | - |
------------------ --------- ---------------

I have swapped your syntax in the following way which looks cleaner to
me: the ARRAY (CASCADE | SET NULL | SET DEFAULT) syntax denote
operations that happen on the whole array, and CASCADE | SET NULL |
SET DEFAULT denote instead operations that happen on the elements of
the array.

Associating the "Element" behaviour with the ON DELETE CASCADE syntax
is also consistent with the case where the referencing table A is
constructed as GROUP BY of another table B, and the array reference on
A is built by aggregating a non-array reference on B with ON DELETE
CASCADE syntax. In other words, the same syntax (ON DELETE CASCADE)
would denote the same behaviour in both the aggregated case ( = one
row per object, using array references) and the non-aggregated case
(multiple rows for one object, using equality references), which
represent two distinct implementations of the same abstraction.

The "Row" behaviour would instead be associated to a new syntax (ON
DELETE ARRAY CASCADE), which cannot be obtained via the existing
syntax in the non-aggregated implementation, on the grounds that it
might be useful for some semantics (for instance: if you remove a
vertex from a polygon, you can either destroy the polygon or transform
it into a polygon with less vertices).

The same principle of considering the two implementations as the same
abstraction would also confirm your choice to raise an exception on ON
(DELETE | UPDATE) SET DEFAULT.

It would also suggest to enable ON (DELETE | UPDATE) ARRAY SET
DEFAULT. The reasoning is that we can actually specify a default array
in the referencing column, but we can't specify a default element.
Before I briefly thought to use the referenced column default as a
default for the single element, but it seems a bad idea: a default is
an expression (possibly non-constant) which is evaluated only when a
new row is created in the referenced table, and using it outside of
that context looks inappropriate.

Regarding ON UPDATE ARRAY CASCADE, I agree to make it a synonym, since
updating the whole array to take into account the update on the
referenced field is equivalent to updating the single element to take
into account the same fact.

Finally, ON UPDATE ARRAY SET NULL would still have an use case as a
different behaviour than ON UPDATE SET NULL, which we make available
to the database designer: instead of replacing the updated element in
the array with a NULL, we replace the whole array with a NULL. This is
essentially the same difference that we have between ON DELETE ARRAY
CASCADE and ON DELETE CASCADE.

Thanks,
Dr. Gianni Ciolli - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gianni.ciolli@2ndquadrant.it | www.2ndquadrant.it

#12

Noah Misch

noah@leadboat.com

over 14 years ago

In reply to: Gabriele Bartolini (#10)

Re: [PATCH] Support for foreign keys with arrays

On Sun, Nov 20, 2011 at 10:36:15AM +0100, Gabriele Bartolini wrote:

I would agree with what Tom is saying here, given that SQL specs do not
say anything about this feature. We could leave standard REFERENCES
keyword handling the array value as it is now. If a user wants to take
advantage of in-array referential integrity, we could implement the
special keyword "ARRAY REFERENCES" as Tom proposes (or a similar
keyword).

No objection to that.

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Element |
SET NULL | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
SET DEFAULT | Error | Error |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

I like this.

CASCADE and ARRAY CASCADE are
synonyms, as they would work in individual elements (which is the action
that makes more sense anyway).

What about making ON UPDATE CASCADE an error? That way, we can say that ARRAY
<action> always applies to array elements, and plain <action> always applies to
entire rows.

SET DEFAULT should now be fine to allow. It's ARRAY SET DEFAULT, in your new
terminology, that wouldn't make sense.

Thanks,
nm

#13

Merlin Moncure

mmoncure@gmail.com

over 14 years ago

In reply to: Gabriele Bartolini (#1)

Re: [PATCH] Support for foreign keys with arrays

On Fri, Nov 4, 2011 at 7:48 AM, Gabriele Bartolini
<gabriele.bartolini@2ndquadrant.it> wrote:

This patch adds basic support of arrays in foreign keys, by allowing to
define a referencing column as an array of elements having the same type as
the referenced column in the referenced table.
Every NOT NULL element in the referencing array is matched against the
referenced table.

I like the idea of being able to define more flexible foreign keys,
but are we gilding the lily here? The proposed solution is really
quite specific to the nuances of arrays. Perhaps there is a more
general expression based syntax that leaves the door open for other
types conditions such as dealing fields dependent on other fields?

merlin

#14

Peter Eisentraut

peter_e@gmx.net

over 14 years ago

In reply to: Merlin Moncure (#13)

Re: [PATCH] Support for foreign keys with arrays

On mån, 2011-11-21 at 10:30 -0600, Merlin Moncure wrote:

I like the idea of being able to define more flexible foreign keys,
but are we gilding the lily here? The proposed solution is really
quite specific to the nuances of arrays. Perhaps there is a more
general expression based syntax that leaves the door open for other
types conditions such as dealing fields dependent on other fields?

Yeah, basically you'd just need a contains and/or is-contained-by
operator between the two types.

#15

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

over 14 years ago

In reply to: Noah Misch (#12)

Re: [PATCH] Support for foreign keys with arrays

Hi Noah,

thanks for your feedback.

Il 20/11/11 14:05, Noah Misch ha scritto:

What about making ON UPDATE CASCADE an error? That way, we can say that ARRAY
<action> always applies to array elements, and plain<action> always applies to
entire rows.

SET DEFAULT should now be fine to allow. It's ARRAY SET DEFAULT, in your new
terminology, that wouldn't make sense.

I have tried to gather your ideas with Gianni's and come to a
compromise, which I hope you can both agree on.

The reason why I would be inclined to leave CASCADE act on rows (rather
than array elements as Gianni suggests) is for backward compatibility
(people that are already using referential integrity based on array
values). For the same reason, I am not sure whether we should raise an
error on update, but will leave this for later.

So, here is a summary:

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Error |
SET NULL | Row | Row |
SET DEFAULT | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

If that's fine with you guys, Marco and I will refactor the development
based on these assumptions.

Thanks,
Gabriele

--
Gabriele Bartolini - 2ndQuadrant Italia
PostgreSQL Training, Services and Support
gabriele.bartolini@2ndQuadrant.it | www.2ndQuadrant.it

#16

Noah Misch

noah@leadboat.com

over 14 years ago

In reply to: Gabriele Bartolini (#15)

Re: [PATCH] Support for foreign keys with arrays

On Sat, Dec 10, 2011 at 09:47:53AM +0100, Gabriele Bartolini wrote:

Il 20/11/11 14:05, Noah Misch ha scritto:

What about making ON UPDATE CASCADE an error? That way, we can say that ARRAY
<action> always applies to array elements, and plain<action> always applies to
entire rows.

SET DEFAULT should now be fine to allow. It's ARRAY SET DEFAULT, in your new
terminology, that wouldn't make sense.

I have tried to gather your ideas with Gianni's and come to a
compromise, which I hope you can both agree on.

The reason why I would be inclined to leave CASCADE act on rows (rather
than array elements as Gianni suggests) is for backward compatibility
(people that are already using referential integrity based on array
values). For the same reason, I am not sure whether we should raise an
error on update, but will leave this for later.

Your conclusion is reasonable, but I don't understand this argument for it. The
patch does not change the meaning of any SQL that works today.

So, here is a summary:

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Error |
SET NULL | Row | Row |
SET DEFAULT | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

If that's fine with you guys, Marco and I will refactor the development
based on these assumptions.

Looks fine.

#17

Noah Misch

noah@leadboat.com

over 14 years ago

In reply to: Tom Lane (#4)

Re: [PATCH] Support for foreign keys with arrays

On Thu, Nov 17, 2011 at 12:08:32AM -0500, Tom Lane wrote:

The least we could do is invent some non-spec syntax that makes the
intention clear, rather than having the system assume that an error case
was intended to mean something else. Maybe

pids INTEGER[] ARRAY REFERENCES pt,

+1. Perhaps this for the table_constraint syntax:

FOREIGN KEY (ARRAY foo, bar, ARRAY pids) REFERENCES pt

#18

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Noah Misch (#16)

Re: [PATCH] Support for foreign keys with arrays

Hello,

Il giorno dom, 11/12/2011 alle 19.45 -0500, Noah Misch ha scritto:

On Sat, Dec 10, 2011 at 09:47:53AM +0100, Gabriele Bartolini wrote:

So, here is a summary:

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Error |
SET NULL | Row | Row |
SET DEFAULT | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

If that's fine with you guys, Marco and I will refactor the development
based on these assumptions.

Looks fine.

This is our latest version of the patch. Gabriele, Gianni and I have
discussed a lot and decided to send an initial patch which uses EACH
REFERENCES instead of ARRAY REFERENCES. The reason behind this is that
ARRAY REFERENCES generates an ambiguous grammar, and we all agreed that
EACH REFERENCES makes sense (and the same time does not introduce any
new keyword). This is however open for discussion.

The patch now includes the following clauses on the delete/update
actions - as per previous emails:

We will resubmit the patch for the 2012-01 commit fest.

Thank you,
Marco

--
Marco Nenciarini - System manager @ Devise.IT
marco.nenciarini@devise.it | http://www.devise.it

#19

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Noah Misch (#16)

Re: [PATCH] Support for foreign keys with arrays

Hello,

Il giorno dom, 11/12/2011 alle 19.45 -0500, Noah Misch ha scritto:

On Sat, Dec 10, 2011 at 09:47:53AM +0100, Gabriele Bartolini wrote:

So, here is a summary:

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Error |
SET NULL | Row | Row |
SET DEFAULT | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

If that's fine with you guys, Marco and I will refactor the development
based on these assumptions.

Looks fine.

The patch now includes the following clauses on the delete/update
actions - as per previous emails:

We will resubmit the patch for the 2012-01 commit fest.

Thank you,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

#20

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Marco Nenciarini (#19)

Re: [PATCH] Support for foreign keys with arrays

On Sat, Jan 14, 2012 at 08:18:48PM +0100, Marco Nenciarini wrote:

This is our latest version of the patch. Gabriele, Gianni and I have
discussed a lot and decided to send an initial patch which uses EACH
REFERENCES instead of ARRAY REFERENCES. The reason behind this is that
ARRAY REFERENCES generates an ambiguous grammar, and we all agreed that
EACH REFERENCES makes sense (and the same time does not introduce any
new keyword). This is however open for discussion.

I greatly like that name; it would still make sense for other aggregate types,
should we ever expand its use. Please complete the name change: the
documentation, catalog entries, etc should all call them something like "each
foreign key constraints" (I don't particularly like that exact wording).

You currently forbid multi-column EACH FKs. I agree that we should allow only
one array column per FK; with more, the set of required PK rows would be
something like the Cartesian product of the elements of array columns.
However, there are no definitional problems, at least for NO ACTION, around a
FK constraint having one array column and N scalar columns. Whether or not
you implement that now, let's choose a table_constraint syntax leaving that
opportunity open. How about:
FOREIGN KEY(col_a, EACH col_b, col_c) REFERENCES pktable (a, b, c)

You've identified that we cannot generally implement the ON DELETE ARRAY
CASCADE action on multidimensional arrays. This patch chooses to downgrade to
ON DELETE ARRAY SET NULL in that case. My initial reaction is that it would
be better to forbid multidimensional arrays in the column when the delete
action is ON DELETE ARRAY SET NULL. That's not satisfying, either, because it
makes the definition of conforming data depend on the ON DELETE action. Do we
have other options?

--------------- --------- ---------
| ON | ON |
Action | DELETE | UPDATE |
--------------- --------- ---------
CASCADE | Row | Error |
SET NULL | Row | Row |
SET DEFAULT | Row | Row |
ARRAY CASCADE | Element | Element |
ARRAY SET NULL | Element | Element |
NO ACTION | - | - |
RESTRICT | - | - |
--------------- --------- ---------

To complete the ARRAY -> EACH transition, I would suggest names like CASCADE
EACH/SET EACH NULL.

I like the extensive test cases you have included. There's one more thing
they should do: leave tables having EACH REFERENCES relationships in the
regression database. This way, pg_dump tests of the regression database will
exercise pg_dump with respect to this feature.

The patch emits several warnings:

heap.c: In function `StoreRelCheck':
heap.c:1947: warning: passing argument 17 of `CreateConstraintEntry' makes integer from pointer without a cast
index.c: In function `index_constraint_create':
index.c:1160: warning: passing argument 17 of `CreateConstraintEntry' makes integer from pointer without a cast
In file included from gram.y:13051:
scan.c: In function `yy_try_NUL_trans':
scan.c:16243: warning: unused variable `yyg'
trigger.c: In function `CreateTrigger':
trigger.c:454: warning: passing argument 17 of `CreateConstraintEntry' makes integer from pointer without a cast
typecmds.c: In function `domainAddConstraint':
typecmds.c:2960: warning: passing argument 17 of `CreateConstraintEntry' makes integer from pointer without a cast
arrayfuncs.c: In function `array_remove':
arrayfuncs.c:5197: warning: unused variable `dimresult'
ri_triggers.c: In function `RI_FKey_check':
ri_triggers.c:484: warning: too many arguments for format

This test case, copied from my previous review except for updating the syntax,
still fails:

BEGIN;
CREATE TABLE parent (c int PRIMARY KEY);
CREATE TABLE child (c int[]);
INSERT INTO parent VALUES (1);
INSERT INTO child VALUES ('{3,1,2}');
ALTER TABLE child ADD FOREIGN KEY (c) EACH REFERENCES parent; -- should error
INSERT INTO child VALUES ('{3,1,2}'); -- does error, as expected
ROLLBACK;

Most of my code comments concern minor matters:

*** a/doc/src/sgml/ddl.sgml
--- b/doc/src/sgml/ddl.sgml

*************** CREATE TABLE order_items (
*** 852,857 ****
--- 882,931 ----
</para>

<para>
+     When working with foreign key arrays, you have two more
+     options that can be used with your
+     <literal>EACH REFERENCES</literal> definition:
+     <literal>ARRAY CASCADE</literal> and
+     <literal>ARRAY SET NULL</literal>. Depending on
+     the triggering action (<command>DELETE</command> or
+     <command>UPDATE</command>) on the referenced table,
+     every element in the referencing array will be either
+     deleted/updated or set to NULL.
+     For more detailed information on foreign key arrays
+     options and special cases, please refer to the
+ 	documentation for <xref linkend="sql-createtable">.
+    </para>
+ 
+ <para>
+     For instance, in the example below, a <command>DELETE</command>
+     from the <literal>countries</literal> table will remove
+     the referencing elements in the <literal>citizenship_ids</literal>
+     array.
+ 
+ <programlisting>
+ CREATE TABLE countries (
+     country_id integer PRIMARY KEY,
+     name text,
+     ...
+ );
+ 
+ CREATE TABLE people (
+     person_id integer PRIMARY KEY,
+     first_name text,
+     last_name text,
+     ...
+     citizenship_ids integer[] <emphasis>EACH REFERENCES countries
+ 		ON DELETE ARRAY CASCADE ON UPDATE ARRAY CASCADE</emphasis>
+ );
+ </programlisting>    
+ 
+     Consequently, an <command>UPDATE</command> of
+     the <literal>country_id</literal> column will be propagated
+     to any element of the <literal>citizenship_ids</literal>
+     field in the <literal>people</literal> table.
+    </para>
+ 
+    <para>

I would leave off this second example.

*************** SELECT NULLIF(value, '(none)') ...
*** 10452,10457 ****
--- 10480,10490 ----
</note>

<para>
+     When using <function>array_remove</function> with multi-dimensional
+     arrays, elements will be set to NULL as fallback measure.
+    </para>

If we do keep this behavior, I would give this function a less-natural name
and not document it.

*** a/doc/src/sgml/ref/create_table.sgml
--- b/doc/src/sgml/ref/create_table.sgml
*************** CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY
*** 51,57 ****
DEFAULT <replaceable>default_expr</replaceable> |
UNIQUE <replaceable class="PARAMETER">index_parameters</replaceable> |
PRIMARY KEY <replaceable class="PARAMETER">index_parameters</replaceable> |
!   REFERENCES <replaceable class="PARAMETER">reftable</replaceable> [ ( <replaceable class="PARAMETER">refcolumn</replaceable> ) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ]
[ ON DELETE <replaceable class="parameter">action</replaceable> ] [ ON UPDATE <replaceable class="parameter">action</replaceable> ] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]

--- 51,57 ----
DEFAULT <replaceable>default_expr</replaceable> |
UNIQUE <replaceable class="PARAMETER">index_parameters</replaceable> |
PRIMARY KEY <replaceable class="PARAMETER">index_parameters</replaceable> |
!   {EACH} REFERENCES <replaceable class="PARAMETER">reftable</replaceable> [ ( <replaceable class="PARAMETER">refcolumn</replaceable> ) ] [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ]
[ ON DELETE <replaceable class="parameter">action</replaceable> ] [ ON UPDATE <replaceable class="parameter">action</replaceable> ] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]

Use square brackets, not curly brackets, around optional terms.

*************** CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY
*** 62,68 ****
UNIQUE ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) <replaceable class="PARAMETER">index_parameters</replaceable> |
PRIMARY KEY ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) <replaceable class="PARAMETER">index_parameters</replaceable> |
EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable> ] ( <replaceable class="parameter">exclude_element</replaceable> WITH <replaceable class="parameter">operator</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replaceable> [ WHERE ( <replaceable class="parameter">predicate</replaceable> ) ] |
! FOREIGN KEY ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) REFERENCES <replaceable class="PARAMETER">reftable</replaceable> [ ( <replaceable class="PARAMETER">refcolumn</replaceable> [, ... ] ) ]
[ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE <replaceable class="parameter">action</replaceable> ] [ ON UPDATE <replaceable class="parameter">action</replaceable> ] }
[ DEFERRABLE | NOT DEFERRABLE ] [ INITIALLY DEFERRED | INITIALLY IMMEDIATE ]
--- 62,68 ----
UNIQUE ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) <replaceable class="PARAMETER">index_parameters</replaceable> |
PRIMARY KEY ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) <replaceable class="PARAMETER">index_parameters</replaceable> |
EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable> ] ( <replaceable class="parameter">exclude_element</replaceable> WITH <replaceable class="parameter">operator</replaceable> [, ... ] ) <replaceable class="parameter">index_parameters</replaceable> [ WHERE ( <replaceable class="parameter">predicate</replaceable> ) ] |
!   FOREIGN KEY ( <replaceable class="PARAMETER">column_name</replaceable> [, ... ] ) {EACH} REFERENCES <replaceable class="PARAMETER">reftable</replaceable> [ ( <replaceable class="PARAMETER">refcolumn</replaceable> [, ... ] ) ]

Likewise.

*** a/src/backend/catalog/information_schema.sql
--- b/src/backend/catalog/information_schema.sql

*************** CREATE VIEW referential_constraints AS
*** 1173,1183 ****

CAST(
CASE con.confdeltype WHEN 'c' THEN 'CASCADE'
WHEN 'n' THEN 'SET NULL'
WHEN 'd' THEN 'SET DEFAULT'
WHEN 'r' THEN 'RESTRICT'
WHEN 'a' THEN 'NO ACTION' END
! AS character_data) AS delete_rule
FROM (pg_namespace ncon
INNER JOIN pg_constraint con ON ncon.oid = con.connamespace
--- 1175,1189 ----
CAST(
CASE con.confdeltype WHEN 'c' THEN 'CASCADE'
+                                   WHEN 'C' THEN 'ARRAY CASCADE'
WHEN 'n' THEN 'SET NULL'
+                                   WHEN 'N' THEN 'ARRAY SET NULL'
WHEN 'd' THEN 'SET DEFAULT'
WHEN 'r' THEN 'RESTRICT'
WHEN 'a' THEN 'NO ACTION' END
!              AS character_data) AS delete_rule,
! 
!            CAST(con.confisarray AS boolean) AS is_array

No need for that cast.

*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c

*************** ATAddForeignKeyConstraint(AlteredTableIn
*** 5688,5693 ****
--- 5689,5728 ----
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced columns for foreign key disagree")));

+ 	/* Enforce foreign key array restrictions */
+ 	if (fkconstraint->fk_array)
+ 	{
+ 		/*
+ 		 * Foreign key array must not be part of a multi-column foreign key
+ 		 */
+ 		if (numpks > 1)
+ 			ereport(ERROR,
+ 				(errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ 				errmsg("foreign key arrays must not be part of a multi-column foreign key")));
+ 
+ 		/*
+ 		 * ON UPDATE CASCADE action is not supported on FKA
+ 		 */
+ 		if (fkconstraint->fk_upd_action == FKCONSTR_ACTION_CASCADE)
+ 			ereport(ERROR,
+ 				(errcode(ERRCODE_INVALID_FOREIGN_KEY),
+ 				errmsg("ON UPDATE CASCADE action is not supported on foreign key arrays")));

Add a HINT about using ARRAY CASCADE.

*************** ATAddForeignKeyConstraint(AlteredTableIn
*** 5736,5775 ****
eqstrategy, opcintype, opcintype, opfamily);

/*
! * Are there equality operators that take exactly the FK type? Assume
! * we should look through any domain here.
*/
! fktyped = getBaseType(fktype);

! pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
! eqstrategy);
! if (OidIsValid(pfeqop))
! ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
eqstrategy);
- else
- ffeqop = InvalidOid; /* keep compiler quiet */

! if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
{
/*
! * Otherwise, look for an implicit cast from the FK type to the
! * opcintype, and if found, use the primary equality operator.
! * This is a bit tricky because opcintype might be a polymorphic
! * type such as ANYARRAY or ANYENUM; so what we have to test is
! * whether the two actual column types can be concurrently cast to
! * that type. (Otherwise, we'd fail to reject combinations such
! * as int[] and point[].)
*/
! Oid input_typeids[2];
! Oid target_typeids[2];

! input_typeids[0] = pktype;
! input_typeids[1] = fktype;
! target_typeids[0] = opcintype;
! target_typeids[1] = opcintype;
! if (can_coerce_type(2, input_typeids, target_typeids,
! COERCION_IMPLICIT))
! pfeqop = ffeqop = ppeqop;
}
if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
--- 5771,5861 ----
eqstrategy, opcintype, opcintype, opfamily);
/*
! * Discover the equality operators
*/
! if (fkconstraint->fk_array)
! {
! /*
! * Are there equality operators that take exactly the FK element type?
! * Assume we should look through any domain here.
! */
! Oid fk_element_type=get_base_element_type(fktype);
! if (!OidIsValid(fk_element_type))
! ereport(ERROR,
! (errcode(ERRCODE_DATATYPE_MISMATCH),
! errmsg("foreign key constraint \"%s\" "
! "cannot be implemented",
! fkconstraint->conname),
! errdetail("Key columns \"%s\" is not an array type: %s",
! strVal(list_nth(fkconstraint->fk_attrs, i)),
! format_type_be(fktype))));

Use a detail message like "Type of key column "%s" is not an array type: %s".

! ffeqop = ARRAY_EQ_OP;
!
! pfeqop = get_opfamily_member(opfamily, opcintype, fk_element_type,
eqstrategy);

! if (!(OidIsValid(pfeqop)))
! {
! /*
! * Otherwise, look for an implicit cast from the FK type to the
! * opcintype, and if found, use the primary equality operator.
! * This is a bit tricky because opcintype might be a polymorphic
! * type such as ANYARRAY or ANYENUM; so what we have to test is
! * whether the two actual column types can be concurrently cast to
! * that type. (Otherwise, we'd fail to reject combinations such
! * as int[] and point[].)
! */
! Oid input_typeids[2];
! Oid target_typeids[2];
!
! input_typeids[0] = pktype;
! input_typeids[1] = fk_element_type;
! target_typeids[0] = opcintype;
! target_typeids[1] = opcintype;
! if (can_coerce_type(2, input_typeids, target_typeids,
! COERCION_IMPLICIT))
! pfeqop = ppeqop;
! }
! }
! else
{
/*
! * Are there equality operators that take exactly the FK type? Assume
! * we should look through any domain here.
*/
! fktyped = getBaseType(fktype);

! pfeqop = get_opfamily_member(opfamily, opcintype, fktyped,
! eqstrategy);
! if (OidIsValid(pfeqop))
! ffeqop = get_opfamily_member(opfamily, fktyped, fktyped,
! eqstrategy);
! else
! ffeqop = InvalidOid; /* keep compiler quiet */
!
! if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))
! {
! /*
! * Otherwise, look for an implicit cast from the FK type to the
! * opcintype, and if found, use the primary equality operator.
! * This is a bit tricky because opcintype might be a polymorphic
! * type such as ANYARRAY or ANYENUM; so what we have to test is
! * whether the two actual column types can be concurrently cast to
! * that type. (Otherwise, we'd fail to reject combinations such
! * as int[] and point[].)
! */
! Oid input_typeids[2];
! Oid target_typeids[2];
!
! input_typeids[0] = pktype;
! input_typeids[1] = fktype;
! target_typeids[0] = opcintype;
! target_typeids[1] = opcintype;
! if (can_coerce_type(2, input_typeids, target_typeids,
! COERCION_IMPLICIT))
! pfeqop = ffeqop = ppeqop;
! }
}

Please reduce the level of textual code duplication here.

*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c

*************** ConvertTriggerToFK(CreateTrigStmt *stmt,
*** 861,876 ****
--- 862,881 ----
switch (funcoid)
{
case F_RI_FKEY_CASCADE_UPD:
+ 		case F_RI_FKEY_ARRCASCADE_UPD:
case F_RI_FKEY_RESTRICT_UPD:
case F_RI_FKEY_SETNULL_UPD:
+ 		case F_RI_FKEY_ARRSETNULL_UPD:
case F_RI_FKEY_SETDEFAULT_UPD:
case F_RI_FKEY_NOACTION_UPD:
funcnum = 0;
break;
case F_RI_FKEY_CASCADE_DEL:
+ case F_RI_FKEY_ARRCASCADE_DEL:
case F_RI_FKEY_RESTRICT_DEL:
case F_RI_FKEY_SETNULL_DEL:
+ case F_RI_FKEY_ARRSETNULL_DEL:
case F_RI_FKEY_SETDEFAULT_DEL:
case F_RI_FKEY_NOACTION_DEL:
funcnum = 1;

We don't need to support these clauses in ConvertTriggerToFK(); no ancient
dumps will bear them. A comment would be enough. On the other hand, your
changes here are simple enough. Maybe it's better to add the support as you
have than to explain why it's absent.

*** a/src/backend/utils/adt/arrayfuncs.c
--- b/src/backend/utils/adt/arrayfuncs.c
*************** array_unnest(PG_FUNCTION_ARGS)
*** 5174,5176 ****
--- 5174,5599 ----
SRF_RETURN_DONE(funcctx);
}
}
+ 
+ /*
+  * Remove any occurrence of an element from an array
+  *
+  * If used on a multi-dimensional array the matching elements will be replaced with NULLs as fallback.
+  *
+  */
+ Datum
+ array_remove(PG_FUNCTION_ARGS)
+ {
+ 	ArrayType  *v;
+ 	Datum		old_value = PG_GETARG_DATUM(1);
+ 	Oid			element_type;
+ 	ArrayType  *result;
+ 	Datum	   *values;
+ 	bool	   *nulls;
+ 	Datum		elt;
+ 	int			ndim;
+ 	int		   *dim;
+ 	int			nitems;
+ 	int		   *dimresult;
+ 	int			nresult;
+ 	int			i;
+ 	int32		nbytes = 0;
+ 	int32		dataoffset;
+ 	bool		hasnulls;
+ 	int			typlen;
+ 	bool		typbyval;
+ 	char		typalign;
+ 	char	   *s;
+ 	bits8	   *bitmap;
+ 	int			bitmask;
+ 	Oid			collation = PG_GET_COLLATION();
+ 	TypeCacheEntry *typentry;
+ 	FunctionCallInfoData locfcinfo;
+ 
+ 	/*
+ 	 * If the first argument is null
+ 	 * return NULL
+ 	 */
+ 	if (PG_ARGISNULL(0))
+ 		PG_RETURN_NULL();
+ 
+ 	v = PG_GETARG_ARRAYTYPE_P(0);
+ 
+ 	/*
+ 	 * If second argument is NULL, no match is possible
+ 	 * so return the first argument unchanged
+ 	 */
+ 	if (PG_ARGISNULL(1))
+ 		PG_RETURN_ARRAYTYPE_P(v);
+ 
+ 	ndim = ARR_NDIM(v);
+ 
+ 	/*
+ 	 * If used on a multi-dimensional array the matching elements
+ 	 * will be replaced with NULLs as fallback.
+ 	 */
+ 	if (ndim > 1) {
+ 		fcinfo->nargs = 3;
+ 		fcinfo->argnull[2]=true;
+ 		return array_replace(fcinfo);
+ 	}
+ 
+ 	dim = ARR_DIMS(v);
+ 	element_type = ARR_ELEMTYPE(v);
+ 	nitems = ArrayGetNItems(ndim, dim);
+ 
+ 	/* Check for empty array */
+ 	if (nitems <= 0)
+ 	{
+ 		/* Return empty array */
+ 		PG_RETURN_ARRAYTYPE_P(construct_empty_array(element_type));
+ 	}
+ 
+ 	/*
+ 	 * We arrange to look up the equality function only once per series of
+ 	 * calls, assuming the element type doesn't change underneath us.  The
+ 	 * typcache is used so that we have no memory leakage when being used
+ 	 * as an index support function.
+ 	 */

The second sentence of the comment does not apply in this function.

+ 	typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
+ 	if (typentry == NULL ||
+ 		typentry->type_id != element_type)
+ 	{
+ 		typentry = lookup_type_cache(element_type,
+ 									 TYPECACHE_EQ_OPR_FINFO);
+ 		if (!OidIsValid(typentry->eq_opr_finfo.fn_oid))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_UNDEFINED_FUNCTION),
+ 			errmsg("could not identify an equality operator for type %s",
+ 				   format_type_be(element_type))));
+ 		fcinfo->flinfo->fn_extra = (void *) typentry;
+ 	}
+ 	typlen = typentry->typlen;
+ 	typbyval = typentry->typbyval;
+ 	typalign = typentry->typalign;
+ 
+ 	/*
+ 	 * apply the operator to each pair of array elements.
+ 	 */
+ 	InitFunctionCallInfoData(locfcinfo, &typentry->eq_opr_finfo, 2,
+ 							 collation, NULL, NULL);
+ 
+ 	/* Allocate temporary arrays for new values */
+ 	values = (Datum *) palloc(nitems * sizeof(Datum));
+ 	nulls = (bool *) palloc(nitems * sizeof(bool));
+ 
+ 	/* Loop over source data */
+ 	s = ARR_DATA_PTR(v);
+ 	bitmap = ARR_NULLBITMAP(v);
+ 	bitmask = 1;
+ 	hasnulls = false;
+ 	nresult=0;
+ 
+ 	for (i = 0; i < nitems; i++)
+ 	{
+ 		bool		isNull;
+ 		bool		oprresult;
+ 		bool		skip;
+ 
+ 		/* Get source element, checking for NULL */
+ 		if (bitmap && (*bitmap & bitmask) == 0)
+ 		{
+ 			isNull = true;
+ 			skip = false;
+ 		}
+ 		else
+ 		{
+ 			elt = fetch_att(s, typbyval, typlen);
+ 			s = att_addlength_datum(s, typlen, elt);
+ 			s = (char *) att_align_nominal(s, typalign);
+ 			isNull = false;
+ 
+ 			/*
+ 			 * Apply the operator to the element pair
+ 			 */
+ 			locfcinfo.arg[0] = elt;
+ 			locfcinfo.arg[1] = old_value;
+ 			locfcinfo.argnull[0] = false;
+ 			locfcinfo.argnull[1] = false;
+ 			locfcinfo.isnull = false;
+ 			oprresult = DatumGetBool(FunctionCallInvoke(&locfcinfo));
+ 			if (!oprresult)
+ 			{
+ 				values[nresult] = elt;
+ 				skip = false;
+ 			}
+ 			else {
+ 				skip = true;
+ 			}

Remove braces.

+ 		}
+ 
+ 		if (!skip)
+ 		{
+ 			nulls[nresult] = isNull;
+ 			if (isNull)
+ 				hasnulls = true;
+ 			else
+ 			{
+ 				/* Ensure data is not toasted */
+ 				if (typlen == -1)
+ 					values[nresult] = PointerGetDatum(PG_DETOAST_DATUM(values[nresult]));

Shouldn't be needed; we just pulled the values from an array.

+ 				/* Update total result size */
+ 				nbytes = att_addlength_datum(nbytes, typlen, values[nresult]);
+ 				nbytes = att_align_nominal(nbytes, typalign);
+ 				/* check for overflow of total request */
+ 				if (!AllocSizeIsValid(nbytes))
+ 					ereport(ERROR,
+ 							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ 							 errmsg("array size exceeds the maximum allowed (%d)",
+ 									(int) MaxAllocSize)));

This test should never fail. Either convert it to an Assert or just add a
comment to that effect.

+ 			}
+ 			nresult++;
+ 		}
+ 
+ 		/* advance bitmap pointer if any */
+ 		if (bitmap)
+ 		{
+ 			bitmask <<= 1;
+ 			if (bitmask == 0x100)
+ 			{
+ 				bitmap++;
+ 				bitmask = 1;
+ 			}
+ 		}
+ 	}
+ 
+ 	/* Allocate and initialize the result array */

Is it worth tracking whether we didn't find anything to remove and just
returning the old array in that case?

+ 	if (hasnulls)
+ 	{
+ 		dataoffset = ARR_OVERHEAD_WITHNULLS(ndim, nresult);
+ 		nbytes += dataoffset;
+ 	}
+ 	else
+ 	{
+ 		dataoffset = 0;			/* marker for no null bitmap */
+ 		nbytes += ARR_OVERHEAD_NONULLS(ndim);
+ 	}
+ 	result = (ArrayType *) palloc0(nbytes);
+ 	SET_VARSIZE(result, nbytes);
+ 	result->ndim = ndim;
+ 	result->dataoffset = dataoffset;
+ 	result->elemtype = element_type;
+ 	memcpy(ARR_DIMS(result), ARR_DIMS(v), 2 * ndim * sizeof(int));
+ 
+ 	/* Adjust the final length */
+ 	ARR_DIMS(result)[0] = nresult;
+ 
+ 	/*
+ 	 * Note: do not risk trying to pfree the results of the called function
+ 	 */

The comment does not apply here; a comparison function has no result to free.

+ 	CopyArrayEls(result,
+ 				 values, nulls, nresult,
+ 				 typlen, typbyval, typalign,
+ 				 false);
+ 
+ 	pfree(values);
+ 	pfree(nulls);
+ 
+ 	PG_RETURN_ARRAYTYPE_P(result);
+ }
+ 
+ /*
+  * Replace any occurrence of an element in an array
+  */
+ Datum
+ array_replace(PG_FUNCTION_ARGS)
+ {
+ 	ArrayType  *v;
+ 	Datum		old_value = PG_GETARG_DATUM(1);
+ 	Datum		new_value = PG_GETARG_DATUM(2);
+ 	bool		new_value_isnull = PG_ARGISNULL(2);
+ 	Oid			element_type;
+ 	ArrayType  *result;
+ 	Datum	   *values;
+ 	bool	   *nulls;
+ 	Datum		elt;
+ 	int		   *dim;
+ 	int			ndim;
+ 	int			nitems;
+ 	int			i;
+ 	int32		nbytes = 0;
+ 	int32		dataoffset;
+ 	bool		hasnulls;
+ 	int			typlen;
+ 	bool		typbyval;
+ 	char		typalign;
+ 	char	   *s;
+ 	bits8	   *bitmap;
+ 	int			bitmask;
+ 	Oid			collation = PG_GET_COLLATION();
+ 	TypeCacheEntry *typentry;
+ 	FunctionCallInfoData locfcinfo;
+ 
+ 	/*
+ 	 * If the first argument is null
+ 	 * return NULL
+ 	 */
+ 	if (PG_ARGISNULL(0))
+ 		PG_RETURN_NULL();
+ 
+ 	v = PG_GETARG_ARRAYTYPE_P(0);
+ 
+ 	/*
+ 	 * If second argument is NULL, no match is possible
+ 	 * so return the first argument unchanged
+ 	 */
+ 	if (PG_ARGISNULL(1))
+ 		PG_RETURN_ARRAYTYPE_P(v);
+ 
+ 	ndim = ARR_NDIM(v);
+ 	dim = ARR_DIMS(v);
+ 	element_type = ARR_ELEMTYPE(v);
+ 	nitems = ArrayGetNItems(ndim, dim);
+ 
+ 	/* Check for empty array */
+ 	if (nitems <= 0)
+ 	{
+ 		/* Return empty array */
+ 		PG_RETURN_ARRAYTYPE_P(construct_empty_array(element_type));
+ 	}
+ 
+ 	/*
+ 	 * We arrange to look up the equality function only once per series of
+ 	 * calls, assuming the element type doesn't change underneath us.  The
+ 	 * typcache is used so that we have no memory leakage when being used
+ 	 * as an index support function.
+ 	 */

The second sentence does not apply here, either.

+ 	typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
+ 	if (typentry == NULL ||
+ 		typentry->type_id != element_type)
+ 	{
+ 		typentry = lookup_type_cache(element_type,
+ 									 TYPECACHE_EQ_OPR_FINFO);
+ 		if (!OidIsValid(typentry->eq_opr_finfo.fn_oid))
+ 			ereport(ERROR,
+ 					(errcode(ERRCODE_UNDEFINED_FUNCTION),
+ 			errmsg("could not identify an equality operator for type %s",
+ 				   format_type_be(element_type))));
+ 		fcinfo->flinfo->fn_extra = (void *) typentry;
+ 	}
+ 	typlen = typentry->typlen;
+ 	typbyval = typentry->typbyval;
+ 	typalign = typentry->typalign;
+ 
+ 	/*
+ 	 * apply the operator to each pair of array elements.
+ 	 */
+ 	InitFunctionCallInfoData(locfcinfo, &typentry->eq_opr_finfo, 2,
+ 							 collation, NULL, NULL);
+ 
+ 	/* Allocate temporary arrays for new values */
+ 	values = (Datum *) palloc(nitems * sizeof(Datum));
+ 	nulls = (bool *) palloc(nitems * sizeof(bool));
+ 
+ 	/* Loop over source data */
+ 	s = ARR_DATA_PTR(v);
+ 	bitmap = ARR_NULLBITMAP(v);
+ 	bitmask = 1;
+ 	hasnulls = false;
+ 
+ 	for (i = 0; i < nitems; i++)
+ 	{
+ 		bool		isNull;
+ 		bool		oprresult;
+ 
+ 		/* Get source element, checking for NULL */
+ 		if (bitmap && (*bitmap & bitmask) == 0)
+ 		{
+ 			isNull = true;
+ 		}
+ 		else
+ 		{
+ 			elt = fetch_att(s, typbyval, typlen);
+ 			s = att_addlength_datum(s, typlen, elt);
+ 			s = (char *) att_align_nominal(s, typalign);
+ 			isNull = false;
+ 
+ 			/*
+ 			 * Apply the operator to the element pair
+ 			 */
+ 			locfcinfo.arg[0] = elt;
+ 			locfcinfo.arg[1] = old_value;
+ 			locfcinfo.argnull[0] = false;
+ 			locfcinfo.argnull[1] = false;
+ 			locfcinfo.isnull = false;
+ 			oprresult = DatumGetBool(FunctionCallInvoke(&locfcinfo));
+ 			if (!oprresult)
+ 			{
+ 				values[i] = elt;
+ 			}
+ 			else if (!new_value_isnull)
+ 			{
+ 				values[i] = new_value;
+ 			}
+ 			else {
+ 				isNull = true;
+ 			}

Remove the braces around single-statement blocks.

+ 		}
+ 
+ 		nulls[i] = isNull;
+ 		if (isNull)
+ 			hasnulls = true;
+ 		else
+ 		{
+ 			/* Ensure data is not toasted */
+ 			if (typlen == -1)
+ 				values[i] = PointerGetDatum(PG_DETOAST_DATUM(values[i]));

The only value that might need a detoast is new_value. Do so once at the top
of the function.

+ 			/* Update total result size */
+ 			nbytes = att_addlength_datum(nbytes, typlen, values[i]);
+ 			nbytes = att_align_nominal(nbytes, typalign);
+ 			/* check for overflow of total request */
+ 			if (!AllocSizeIsValid(nbytes))
+ 				ereport(ERROR,
+ 						(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ 						 errmsg("array size exceeds the maximum allowed (%d)",
+ 								(int) MaxAllocSize)));
+ 		}
+ 
+ 		/* advance bitmap pointer if any */
+ 		if (bitmap)
+ 		{
+ 			bitmask <<= 1;
+ 			if (bitmask == 0x100)
+ 			{
+ 				bitmap++;
+ 				bitmask = 1;
+ 			}
+ 		}
+ 	}
+ 
+ 	/* Allocate and initialize the result array */
+ 	if (hasnulls)
+ 	{
+ 		dataoffset = ARR_OVERHEAD_WITHNULLS(ndim, nitems);
+ 		nbytes += dataoffset;
+ 	}
+ 	else
+ 	{
+ 		dataoffset = 0;			/* marker for no null bitmap */
+ 		nbytes += ARR_OVERHEAD_NONULLS(ndim);
+ 	}
+ 	result = (ArrayType *) palloc0(nbytes);
+ 	SET_VARSIZE(result, nbytes);
+ 	result->ndim = ndim;
+ 	result->dataoffset = dataoffset;
+ 	result->elemtype = element_type;
+ 	memcpy(ARR_DIMS(result), ARR_DIMS(v), 2 * ndim * sizeof(int));
+ 
+ 	/*
+ 	 * Note: do not risk trying to pfree the results of the called function
+ 	 */

The comment does not apply here; a comparison function has no result to free.

+ 	CopyArrayEls(result,
+ 				 values, nulls, nitems,
+ 				 typlen, typbyval, typalign,
+ 				 false);
+ 
+ 	pfree(values);
+ 	pfree(nulls);
+ 
+ 	PG_RETURN_ARRAYTYPE_P(result);
+ }
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 03a974a..58a92e1 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c

*************** typedef struct RI_ConstraintInfo
*** 106,111 ****
--- 110,116 ----
NameData	conname;		/* name of the FK constraint */
Oid			pk_relid;		/* referenced relation */
Oid			fk_relid;		/* referencing relation */
+ 	bool		confisarray;

Comment that member.

+ Datum
+ RI_FKey_arrcascade_upd(PG_FUNCTION_ARGS)
+ {

These belong earlier in the file, adjacent to the other PK trigger functions.

+ 				/* ----------
+ 				 * The query string built is
+ 				 *	UPDATE ONLY <fktable> SET fkatt1 = array_replace(fkatt1, $n, $1) [, ...]
+ 				 *			WHERE $n = fkatt1 [AND ...]
+ 				 * The type id's for the $ parameters are those of the
+ 				 * corresponding PK attributes.
+ 				 * ----------
+ 				 */

RI_FKey_cascade_upd() has this to say at the corresponding juncture:

/* ----------
* The query string built is
* UPDATE ONLY <fktable> SET fkatt1 = $1 [, ...]
* WHERE $n = fkatt1 [AND ...]
* The type id's for the $ parameters are those of the
* corresponding PK attributes. Note that we are assuming
* there is an assignment cast from the PK to the FK type;
* else the parser will fail.
* ----------
*/

Since we're matching a polymorphic function here, the types of the second and
third arguments must precisely match the element type of the first argument.
Even when an implicit cast exists, we must insert cast syntax. Test case:

BEGIN;
CREATE TABLE parent (c smallint PRIMARY KEY);
CREATE TABLE child (c int[] EACH REFERENCES parent
ON UPDATE ARRAY CASCADE
ON DELETE ARRAY CASCADE);
INSERT INTO parent VALUES (1), (2);
INSERT INTO child VALUES ('{1,2}');
UPDATE parent SET c = 3 WHERE c = 2;
DELETE FROM parent WHERE c = 1;
ROLLBACK;

+ 				initStringInfo(&querybuf);
+ 				initStringInfo(&qualbuf);
+ 				quoteRelationName(fkrelname, fk_rel);
+ 				appendStringInfo(&querybuf, "UPDATE ONLY %s SET", fkrelname);
+ 				querysep = "";
+ 				qualsep = "WHERE";
+ 				for (i = 0, j = riinfo.nkeys; i < riinfo.nkeys; i++, j++)
+ 				{
+ 					Oid			pk_type = RIAttType(pk_rel, riinfo.pk_attnums[i]);
+ 					Oid			fk_type = RIAttType(fk_rel, riinfo.fk_attnums[i]);
+ 
+ 					quoteOneName(attname,
+ 								 RIAttName(fk_rel, riinfo.fk_attnums[i]));
+ 					appendStringInfo(&querybuf,
+ 									 "%s %s = array_replace(%s, $%d, $%d)",
+ 									 querysep, attname, attname, j + 1, i + 1);

Break this line to keep it within 78 columns. Otherwise, pgindent will
reverse-indent it. There are some other examples in your patch. Please run
your patch through this command, inspect the output, and fix any that don't
belong:

<patchfile expand -t4 | awk 'length > 78'

*** a/src/include/catalog/pg_constraint.h
--- b/src/include/catalog/pg_constraint.h
*************** CATALOG(pg_constraint,2606)
*** 78,83 ****
--- 78,84 ----
* constraint.	Otherwise confrelid is 0 and the char fields are spaces.
*/
Oid			confrelid;		/* relation referenced by foreign key */
+ 	bool		confisarray;	/* true if an EACH REFERENCE foreign key */
char		confupdtype;	/* foreign key's ON UPDATE action */
char		confdeltype;	/* foreign key's ON DELETE action */
char		confmatchtype;	/* foreign key's match type */

Putting the field at this location adds three bytes of padding. Please locate
it elsewhere to avoid that.

*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h

*************** DESCR("referential integrity ON DELETE N
*** 1976,1981 ****
--- 1981,1995 ----
DATA(insert OID = 1655 (  RI_FKey_noaction_upd PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 2279 "" _null_ _null_ _null_ _null_ RI_FKey_noaction_upd _null_ _null_ _null_ ));
DESCR("referential integrity ON UPDATE NO ACTION");

+ DATA(insert OID = 3144 (  RI_FKey_arrcascade_del	PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 2279 "" _null_ _null_ _null_ _null_ RI_FKey_arrcascade_del _null_ _null_ _null_ ));
+ DESCR("referential integrity ON DELETE CASCADE");
+ DATA(insert OID = 3145 (  RI_FKey_arrcascade_upd	PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 2279 "" _null_ _null_ _null_ _null_ RI_FKey_arrcascade_upd _null_ _null_ _null_ ));
+ DESCR("referential integrity ON UPDATE CASCADE");
+ DATA(insert OID = 3146 (  RI_FKey_arrsetnull_del	PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 2279 "" _null_ _null_ _null_ _null_ RI_FKey_arrsetnull_del _null_ _null_ _null_ ));
+ DESCR("referential integrity ON DELETE SET NULL");
+ DATA(insert OID = 3147 (  RI_FKey_arrsetnull_upd	PGNSP PGUID 12 1 0 0 0 f f f t f v 0 0 2279 "" _null_ _null_ _null_ _null_ RI_FKey_arrsetnull_upd _null_ _null_ _null_ ));
+ DESCR("referential integrity ON UPDATE SET NULL");

Those descriptions need to reflect the actual clauses involved.

Thanks,
nm

#21

Simon Riggs

simon@2ndQuadrant.com

about 14 years ago

In reply to: Noah Misch (#20)

#22

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Simon Riggs (#21)

#23

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

about 14 years ago

In reply to: Noah Misch (#20)

#24

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Gabriele Bartolini (#23)

#25

Erik Rijkers

er@xs4all.nl

about 14 years ago

In reply to: Marco Nenciarini (#18)

#26

Peter Geoghegan

pg@bowt.ie

about 14 years ago

In reply to: Erik Rijkers (#25)

#27

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

about 14 years ago

In reply to: Erik Rijkers (#25)

#28

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Marco Nenciarini (#24)

#29

Robert Haas

robertmhaas@gmail.com

about 14 years ago

In reply to: Noah Misch (#28)

#30

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Robert Haas (#29)

#31

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Noah Misch (#28)

#32

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

about 14 years ago

In reply to: Marco Nenciarini (#31)

#33

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Marco Nenciarini (#31)

#34

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Noah Misch (#33)

#35

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Marco Nenciarini (#34)

#36

Tom Lane

tgl@sss.pgh.pa.us

about 14 years ago

In reply to: Marco Nenciarini (#34)

#37

Gianni Ciolli

gianni.ciolli@2ndquadrant.it

about 14 years ago

In reply to: Tom Lane (#36)

#38

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

about 14 years ago

In reply to: Tom Lane (#36)

#39

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

about 14 years ago

In reply to: Marco Nenciarini (#34)

#40

Peter Eisentraut

peter_e@gmx.net

about 14 years ago

In reply to: Gianni Ciolli (#37)

#41

Noah Misch

noah@leadboat.com

about 14 years ago

In reply to: Peter Eisentraut (#40)

#42

Noah Misch

noah@leadboat.com

almost 14 years ago

In reply to: Marco Nenciarini (#39)

#43

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

almost 14 years ago

In reply to: Noah Misch (#42)

#44

Noah Misch

noah@leadboat.com

almost 14 years ago

In reply to: Gabriele Bartolini (#43)

#45

Marco Nenciarini

marco.nenciarini@2ndquadrant.it

almost 14 years ago

In reply to: Noah Misch (#44)

#46

Simon Riggs

simon@2ndQuadrant.com

almost 14 years ago

In reply to: Peter Eisentraut (#40)

#47

Jeff Davis

pgsql@j-davis.com

almost 14 years ago

In reply to: Gabriele Bartolini (#1)

#48

Misa Simic

misa.simic@gmail.com

almost 14 years ago

In reply to: Jeff Davis (#47)

#49

Simon Riggs

simon@2ndQuadrant.com

almost 14 years ago

In reply to: Misa Simic (#48)

#50

Kevin Grittner

Kevin.Grittner@wicourts.gov

almost 14 years ago

In reply to: Simon Riggs (#49)

#51

Misa Simic

misa.simic@gmail.com

almost 14 years ago

In reply to: Simon Riggs (#49)

#52

Misa Simic

misa.simic@gmail.com

almost 14 years ago

In reply to: Kevin Grittner (#50)

#53

Gianni Ciolli

gianni.ciolli@2ndquadrant.it

almost 14 years ago

In reply to: Kevin Grittner (#50)

#54

Jeff Davis

pgsql@j-davis.com

almost 14 years ago

In reply to: Simon Riggs (#49)

#55

Kevin Grittner

Kevin.Grittner@wicourts.gov

almost 14 years ago

In reply to: Misa Simic (#52)

#56

Misa Simic

misa.simic@gmail.com

almost 14 years ago

In reply to: Kevin Grittner (#55)

#57

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

over 13 years ago

In reply to: Simon Riggs (#46)

#58

Simon Riggs

simon@2ndQuadrant.com

over 13 years ago

In reply to: Gabriele Bartolini (#57)

#59

Robert Haas

robertmhaas@gmail.com

over 13 years ago

In reply to: Gabriele Bartolini (#57)

#60

Alvaro Herrera

alvherre@2ndquadrant.com

over 13 years ago

In reply to: Simon Riggs (#58)

#61

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

over 13 years ago

In reply to: Alvaro Herrera (#60)

#62

Gabriele Bartolini

gabriele.bartolini@2ndQuadrant.it

over 13 years ago

In reply to: Gabriele Bartolini (#61)

[PATCH] Support for foreign keys with arrays

Attachments:

Attachments:

Attachments: