From: Marco Nenciarini <marco.nenciarini@2ndQuadrant.it>
To: pgsql-hackers@postgresql.org
Cc: Noah Misch <noah@leadboat.com>
Subject: Re: [PATCH] Support for foreign keys with arrays
Date: Mon, 19 Mar 2012 18:41:39 +0100
Message-ID: <1332178899.2278.4.camel@greygoo.devise-it.lan> (raw)
In-Reply-To: <20120317033312.GB19556@tornado.leadboat.com>
References: <4EC8CA0F.8040405@2ndQuadrant.it>
<20111120130526.GD12013@tornado.leadboat.com>
<4EE31CB9.90005@2ndQuadrant.it>
<20111212004506.GB10399@tornado.leadboat.com>
<1326568728.2613.16.camel@greygoo.devise-it.lan>
<20120121204223.GA2388@tornado.leadboat.com>
<4F280EFE.1080604@2ndQuadrant.it>
<1328551482.3354.64.camel@greygoo.devise-it.lan>
<20120225020135.GA27204@tornado.leadboat.com>
<1331748188.6744.15.camel@greygoo.devise-it.lan>
<20120317033312.GB19556@tornado.leadboat.com>
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.nenciarini@2ndQuadrant.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.
Attachments:
EACH-foreign-key.v5.patch.bz2 (application/x-bzip, 28.7K) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: marco.nenciarini@2ndQuadrant.it, noah@leadboat.com
Subject: Re: [PATCH] Support for foreign keys with arrays
In-Reply-To: <1332178899.2278.4.camel@greygoo.devise-it.lan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox