Re: [PATCH] Support for foreign keys with arrays

Hi Noah,

thank you again for your thorough review, which is much appreciated.

> I think the patch has reached the stage where a committer can review
> it without wasting much time on things that might change radically.
> So, I'm marking it Ready for Committer. Please still submit an update
> correcting the above; I'm sure your committer will appreciate it.

Attached is v5, which should address all the remaining issues.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco(dot)nenciarini(at)2ndQuadrant(dot)it | www.2ndQuadrant.it

On Fri, Mar 16, 2012 at 11:33:12PM -0400, Noah Misch wrote:
> I recommend removing the new block of code in RI_FKey_eachcascade_del() and
> letting array_remove() throw the error. If you find a way to throw a nicer
> error without an extra scan, by all means submit that to a future CF as an
> improvement. I don't think it's important enough to justify cycles at this
> late hour of the current CF.

It makes sense; we have removed the block of code and updated the error
message following your suggestion. Now the error is thrown by array_remove()
itself.
We'll keep an eye on this for further improvements in the future.

> > > pg_constraint.confeach needs documentation.
> >
> > Most of pg_constraint columns, including all the boolean ones, are
> > documented only in the "description" column of
> >
> > http://www.postgresql.org/docs/9.1/static/catalog-pg-constraint.html#AEN85760
> >
> > it seems that confiseach should not be an exception, since it just
> > indicates whether the constraint is of a certain kind or not.
>
> Your patch adds two columns to pg_constraint, confiseach and confeach, but it
> only mentions confiseach in documentation. Just add a similar minimal mention
> of confeach.

Sorry, our mistake; a mention for confeach has now been added, and both
entries have been reordered to reflect the column position in
pg_constraint).

> That is to say, they start with a capital letter and end with a period. Your
> old text was fine apart from the lack of a period. (Your new text also lacks
> a period.)

Fixed, it should be fine now (another misunderstanding on our side, apologies).

> If the cost doesn't exceed O(F log P), where F is the size of the FK table and
> P is the size of the PK table, I'm not worried. If it can be O(F^2), we would
> have a problem to be documented, if not fixed.

We have rewritten the old query in a simpler way; now its cost is O(F log P).
Here F must represent the size of the "flattened" table, that is, the total
number of values that must be checked, which seems a reasonable assumption
in any case.

> Your change to array_replace() made me look at array_remove() again and
> realize that it needs the same treatment. This command yields a segfault:
> SELECT array_remove('{a,null}'::text[], null);

Fixed.

>
> This latest version introduces two calls to get_element_type(), both of which
> should be get_base_element_type().

Fixed.

> Patch "Avoid FK validations for no-rewrite ALTER TABLE ALTER TYPE", committed
> between v3b and v4 of this patch, added code to ATAddForeignKeyConstraint()
> requiring an update in this patch. Run this in the regression DB:
> [local] regression=# alter table fktableforeachfk alter ftest1 type int[];
> ERROR: could not find cast from 1007 to 23

Thanks for pointing it out. We have added a regression test and then fixed the issue.

>
> RI_PLAN_EACHCASCADE_DEL_DOUPDATE should be RI_PLAN_EACHCASCADE_DEL_DODELETE.

Fixed.