Re: 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(at)2ndquadrant(dot)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(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> On 15 June 2011 07:56, Jaime Casanova <jaime(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> writes:
>> On 15 June 2011 07:56, Jaime Casanova <jaime(at)2ndquadrant(dot)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(dot)a(dot)rasheed(at)gmail(dot)com> writes:
> > On 15 June 2011 07:56, Jaime Casanova <jaime(at)2ndquadrant(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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