[HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
Hi Simon, On 1/14/2011 1:15 PM, Simon Riggs wrote: Patch to implement the proposed feature attached, for CFJan2011. Overall, I think the patch looks good, but I found some problems with it. In tablecmds.c you have: + if (found con-contype == CONSTR_FOREIGN !con-convalidated) which I don't think is correct, and my tests seem to agree; the actual validation doesn't happen at all. Changing that to CONSTRAINT_FOREIGN makes the validation part work, but then I get: ERROR: cache lookup failed for constraint 16419 when trying to drop the table and the regression tests fail because of this. Also having a regression test where the validation fails seems like a good idea. Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. I also think that having the function for getting a list of values that violate the constraint would be helpful. Any particular reason why you decided to omit it from this patch? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On Sun, 2011-01-23 at 19:50 +0200, Marko Tiikkaja wrote: Hi Simon, On 1/14/2011 1:15 PM, Simon Riggs wrote: Patch to implement the proposed feature attached, for CFJan2011. Overall, I think the patch looks good Thanks for the review. , but I found some problems with it. In tablecmds.c you have: + if (found con-contype == CONSTR_FOREIGN !con-convalidated) which I don't think is correct, and my tests seem to agree; the actual validation doesn't happen at all. Changing that to CONSTRAINT_FOREIGN makes the validation part work, but then I get: ERROR: cache lookup failed for constraint 16419 when trying to drop the table and the regression tests fail because of this. Also having a regression test where the validation fails seems like a good idea. Thanks. Will fix. Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. Should it? What command do you think needs changing? I also think that having the function for getting a list of values that violate the constraint would be helpful. Any particular reason why you decided to omit it from this patch? Yes, the consensus was that DDL was required, not a function. Function was my preferred approach originally. That now appears to be an additional request from a couple of people. At present, its easy enough to write the SQL statement yourself, so that's non-essential, and maybe/likely won't make this release (not sure, depends upon how other aspects go). There is no option to invoke this yet from pg_restore, which seems likely to top the list of priorities. Would you agree? -- Simon Riggs http://www.2ndQuadrant.com/books/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On 1/23/2011 8:23 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 19:50 +0200, Marko Tiikkaja wrote: Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. Should it? What command do you think needs changing? \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. I also think that having the function for getting a list of values that violate the constraint would be helpful. Any particular reason why you decided to omit it from this patch? Yes, the consensus was that DDL was required, not a function. Function was my preferred approach originally. While I do agree that the DDL command should be the preferred way to validate the constraint, I think the function adds a significant value when the validation does not succeed. That now appears to be an additional request from a couple of people. At present, its easy enough to write the SQL statement yourself, so that's non-essential, and maybe/likely won't make this release (not sure, depends upon how other aspects go). I understand. There is no option to invoke this yet from pg_restore, which seems likely to top the list of priorities. Would you agree? I don't understand what you mean with this. Could you be a bit more elaborate? Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On Sun, 2011-01-23 at 20:33 +0200, Marko Tiikkaja wrote: On 1/23/2011 8:23 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 19:50 +0200, Marko Tiikkaja wrote: Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. Should it? What command do you think needs changing? \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. Neither \d nor \di shows invalid indexes. Should we add validation for FKs when it is not there for indexes? My feeling was no. Desirable for both? Yes, but not as part of this patch. There is no option to invoke this yet from pg_restore, which seems likely to top the list of priorities. Would you agree? I don't understand what you mean with this. Could you be a bit more elaborate? The purpose of this patch is performance. pg_restore will be faster if it uses this new feature, so I expect to add an option to reload data without validating FKs. -- Simon Riggs http://www.2ndQuadrant.com/books/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On 1/23/2011 8:43 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 20:33 +0200, Marko Tiikkaja wrote: On 1/23/2011 8:23 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 19:50 +0200, Marko Tiikkaja wrote: Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. Should it? What command do you think needs changing? \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. Neither \d nor \di shows invalid indexes. What exactly are you referring to? An index with indisvalid=false looks like this in my psql: fooindex btree (a) INVALID And even if it didn't, I don't think we should be adding more deficiencies to psql. Should we add validation for FKs when it is not there for indexes? My feeling was no. Desirable for both? Yes, but not as part of this patch. There is no option to invoke this yet from pg_restore, which seems likely to top the list of priorities. Would you agree? I don't understand what you mean with this. Could you be a bit more elaborate? The purpose of this patch is performance. pg_restore will be faster if it uses this new feature, so I expect to add an option to reload data without validating FKs. Ah. Right, that would make sense. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On Sun, 2011-01-23 at 20:56 +0200, Marko Tiikkaja wrote: On 1/23/2011 8:43 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 20:33 +0200, Marko Tiikkaja wrote: On 1/23/2011 8:23 PM, Simon Riggs wrote: On Sun, 2011-01-23 at 19:50 +0200, Marko Tiikkaja wrote: Another problem I found is that psql doesn't indicate in any way that a FOREIGN KEY constraint is not validated yet. Should it? What command do you think needs changing? \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. Neither \d nor \di shows invalid indexes. What exactly are you referring to? An index with indisvalid=false looks like this in my psql: fooindex btree (a) INVALID And even if it didn't, I don't think we should be adding more deficiencies to psql. OK, thanks, I wasn't aware of that. I'll add something similar for FKs. -- Simon Riggs http://www.2ndQuadrant.com/books/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
Simon Riggs si...@2ndquadrant.com writes: On Sun, 2011-01-23 at 20:33 +0200, Marko Tiikkaja wrote: \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. Neither \d nor \di shows invalid indexes. Even if that were true, it's a poor analogy, since a disabled foreign key has visible *semantic* impact, whereas a disabled index doesn't. 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
Re: [HACKERS] REVIEW: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED
On Sun, 2011-01-23 at 14:45 -0500, Tom Lane wrote: Simon Riggs si...@2ndquadrant.com writes: On Sun, 2011-01-23 at 20:33 +0200, Marko Tiikkaja wrote: \d table now only shows that there's a FOREIGN KEY, which might lead the user to think that there should not be any values that don't exist in the referenced table. Neither \d nor \di shows invalid indexes. Even if that were true, it's a poor analogy, since a disabled foreign key has visible *semantic* impact, whereas a disabled index doesn't. Sure. My agreement to add something appears to have crossed with your comments. I'd appreciate you reviewing the parser aspects of the patch. $TITLE no longer reflects the syntax. -- Simon Riggs http://www.2ndQuadrant.com/books/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
