Thread: FK NOT VALID can't be deferrable?
Hi, Testing the CHECK NOT VALID patch i found $subject... is this intended? -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación
On 15 June 2011 07:56, Jaime Casanova <jaime@2ndquadrant.com> wrote: > Hi, > > Testing the CHECK NOT VALID patch i found $subject... is this intended? > I just noticed that too, and was about to raise it as a bug. If it is intended, then it's not documented. I noticed it while browsing gram.y, and thought it looks a bit ugly having 2 almost identical code blocks, one for the normal case and one for NOT VALID. The second block doesn't have a ConstraintAttributeSpec, so won't allow any deferrable options. Aside from the ugliness of the code, we can't just add a ConstraintAttributeSpec to the second block, because that would enforce an order to these options. OTOH adding NOT VALID to ConstraintAttributeSpec is a bit invasive, since it's used in quite a few places, including CREATE TABLE, where NOT VALID is never allowed. Thoughts? Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 15 June 2011 07:56, Jaime Casanova <jaime@2ndquadrant.com> wrote:
>> Testing the CHECK NOT VALID patch i found $subject... is this intended?
> Aside from the ugliness of the code, we can't just add a
> ConstraintAttributeSpec to the second block, because that would
> enforce an order to these options.
> OTOH adding NOT VALID to ConstraintAttributeSpec is a bit invasive,
> since it's used in quite a few places, including CREATE TABLE, where
> NOT VALID is never allowed.
> Thoughts?
I think we need to do the second one, ie, add it to
ConstraintAttributeSpec and do what's necessary to filter later.
The reason we have a problem here is exactly that somebody took
shortcuts.
It'd probably be sufficient to have one or two places in
parse_utilcmds.c know which variants of Constraint actually support
NOT VALID, and throw an error for the rest.
regards, tom lane
On Wed, Jun 15, 2011 at 4:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: >> On 15 June 2011 07:56, Jaime Casanova <jaime@2ndquadrant.com> wrote: >>> Testing the CHECK NOT VALID patch i found $subject... is this intended? > >> Aside from the ugliness of the code, we can't just add a >> ConstraintAttributeSpec to the second block, because that would >> enforce an order to these options. > >> OTOH adding NOT VALID to ConstraintAttributeSpec is a bit invasive, >> since it's used in quite a few places, including CREATE TABLE, where >> NOT VALID is never allowed. > >> Thoughts? > > I think we need to do the second one, ie, add it to > ConstraintAttributeSpec and do what's necessary to filter later. > The reason we have a problem here is exactly that somebody took > shortcuts. There were grammar issues in the NOT VALID patch which I sought to resolve. Those new suggestions may fall foul of the same issues. I raised that point and asked for input prior to commit. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Excerpts from Tom Lane's message of mié jun 15 11:54:25 -0400 2011: > Dean Rasheed <dean.a.rasheed@gmail.com> writes: > > On 15 June 2011 07:56, Jaime Casanova <jaime@2ndquadrant.com> wrote: > >> Testing the CHECK NOT VALID patch i found $subject... is this intended? > > > Aside from the ugliness of the code, we can't just add a > > ConstraintAttributeSpec to the second block, because that would > > enforce an order to these options. > > > OTOH adding NOT VALID to ConstraintAttributeSpec is a bit invasive, > > since it's used in quite a few places, including CREATE TABLE, where > > NOT VALID is never allowed. > > I think we need to do the second one, ie, add it to > ConstraintAttributeSpec and do what's necessary to filter later. > The reason we have a problem here is exactly that somebody took > shortcuts. > > It'd probably be sufficient to have one or two places in > parse_utilcmds.c know which variants of Constraint actually support > NOT VALID, and throw an error for the rest. So is somebody from 2nd Quadrant going to supply a patch to fix this? -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jun 15, 2011 at 3:14 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > > So is somebody from 2nd Quadrant going to supply a patch to fix this? > well, i was going to give it a try... but in a couple of hours... -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación
On Wed, Jun 15, 2011 at 9:14 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > So is somebody from 2nd Quadrant going to supply a patch to fix this? My understanding was that your patch had a bug, rather than the existing code. If I misunderstood, please explain the bug. In terms of "2ndQuadrant" supplying patches, you may not be aware that we all work independently on community contributions... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Excerpts from Simon Riggs's message of mié jun 15 16:31:45 -0400 2011: > On Wed, Jun 15, 2011 at 9:14 PM, Alvaro Herrera > <alvherre@commandprompt.com> wrote: > > > So is somebody from 2nd Quadrant going to supply a patch to fix this? > > My understanding was that your patch had a bug, rather than the > existing code. If I misunderstood, please explain the bug. This is about FOREIGN KEY NOT VALID, which is your patch that's already in 9.1. My patch doesn't touch foreign keys. CHECK constraints cannot be deferrable anyway. > In terms of "2ndQuadrant" supplying patches, you may not be aware that > we all work independently on community contributions... I wasn't, but that doesn't change anything -- I mean, you were from 2nd Quadrant last time I checked. If Jaime is going to submit a patch to fix the bug, I assume you're fine with that too? (The other alternative is to leave the bug open until Robert or Tom fix it, I guess) -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> So is somebody from 2nd Quadrant going to supply a patch to fix this?
I'm already on it. The whole patch appears to need some review,
considering this is about the fourth major flaw we've found in it.
regards, tom lane
On Wed, Jun 15, 2011 at 9:59 PM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Simon Riggs's message of mié jun 15 16:31:45 -0400 2011: >> On Wed, Jun 15, 2011 at 9:14 PM, Alvaro Herrera >> <alvherre@commandprompt.com> wrote: >> >> > So is somebody from 2nd Quadrant going to supply a patch to fix this? >> >> My understanding was that your patch had a bug, rather than the >> existing code. If I misunderstood, please explain the bug. > > This is about FOREIGN KEY NOT VALID, which is your patch that's already > in 9.1. My patch doesn't touch foreign keys. CHECK constraints cannot > be deferrable anyway. OK, thanks. My bug, my responsibility to provide a solution. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jun 15, 2011 at 10:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@commandprompt.com> writes: >> So is somebody from 2nd Quadrant going to supply a patch to fix this? > > I'm already on it. The whole patch appears to need some review, > considering this is about the fourth major flaw we've found in it. I'll leave it with you then, but I remain happy to fix. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services