Hello 2011/11/17 Noah Misch <n...@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 > 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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers