Re: ALTER TABLE ... ADD FOREIGN KEY ... NOT ENFORCED

The new SQL Standard (SQL:2011) contains this:
"Table constraints are either enforced or not enforced. Domain
constraints and assertions are always enforced.", 4.17.2

The SQL Standard allows you to turn the checking on and off for CHECK
constraints, UNIQUE constraints and FOREIGN KEYS.

Which of those make sense for us, if any? The ability to create FKs
without checking all the data has been frequently requested to me over
many years. OTOH, I can't really see any point in turning on/off all of
the other aspects mentioned by the SQL Standard, especially indexes.
It's lots of work and seems likely to end with poorer data quality. And
the obvious thing is if you don't want a CHECK constraint, just drop
it...

My proposal is that we add a short and simple clause NOT ENFORCED onto
the ADD constraint syntax. So we have

ALTER TABLE foo
ADD FOREIGN KEY .... NOT ENFORCED;

The "enforced" state is not persisted - once added the FK is checked
every time. So there is no additional column on pg_constraint.

The benefit here is that we implement a capability that allows skipping
very long running SQL statements when required, and doesn't require too
much code. It has been discussed before on hackers, but that was before
it was part of the SQL Standard. Oracle has had this for years and it is
popular feature. We can expect other RDBMS to implement this feature,
now it is part of the standard.

If you want more than my good-bits-only proposal, it really isn't going
to happen for 9.1, and seems pretty pointless anyway.

Very short hack to implement this attached for discussion. No tests, not
even a compile - just showing how quick a patch this can be.

Thoughts? Alternative syntax?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> The new SQL Standard (SQL:2011) contains this:
> "Table constraints are either enforced or not enforced. Domain
> constraints and assertions are always enforced.", 4.17.2

> The SQL Standard allows you to turn the checking on and off for CHECK
> constraints, UNIQUE constraints and FOREIGN KEYS.

Huh? It allows you to postpone the check until commit. That's far from
not enforcing it.

regards, tom lane

On Sun, 2010-12-12 at 17:57 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > The new SQL Standard (SQL:2011) contains this:
> > "Table constraints are either enforced or not enforced. Domain
> > constraints and assertions are always enforced.", 4.17.2
>
> > The SQL Standard allows you to turn the checking on and off for CHECK
> > constraints, UNIQUE constraints and FOREIGN KEYS.
>
> Huh? It allows you to postpone the check until commit. That's far from
> not enforcing it.

"When a <commit statement> is executed, all enforced constraints are
effectively checked and, if any enforced
constraint is not satisfied, then an exception condition is raised and
the SQL-transaction is terminated by an
implicit <rollback statement>."

This clearly implies that un-enforced constraints are not checked at
commit.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Sun, 2010-12-12 at 17:57 -0500, Tom Lane wrote:
>> Huh? It allows you to postpone the check until commit. That's far from
>> not enforcing it.

> This clearly implies that un-enforced constraints are not checked at
> commit.

[ shrug... ] I can't argue with you about what may or may not be in an
unpublished draft of an unratified version of the standard, since I
don't have a copy. But allow me to harbor doubts that they really
intend to allow someone to force a constraint to be considered valid
without any verification. This proposal strikes me as something mysql
would do, not the standards committee. (In particular, can a constraint
go from not-enforced to enforced state without getting checked at that
time?)

Even if you're reading the draft correctly, and the wording makes it
into a released standard, the implementation you propose would break our
code. The incremental FK checks are designed on the assumption that the
constraint condition held before; they aren't likely to behave very
sanely if the data is bad. I'd want to see a whole lot more analysis of
the resulting behavior before even considering an idea like this.

regards, tom lane

On Sun, Dec 12, 2010 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On Sun, 2010-12-12 at 17:57 -0500, Tom Lane wrote:
>>> Huh?  It allows you to postpone the check until commit.  That's far from
>>> not enforcing it.
>
>> This clearly implies that un-enforced constraints are not checked at
>> commit.
>
> [ shrug... ]  I can't argue with you about what may or may not be in an
> unpublished draft of an unratified version of the standard, since I
> don't have a copy.  But allow me to harbor doubts that they really
> intend to allow someone to force a constraint to be considered valid
> without any verification.  This proposal strikes me as something mysql
> would do, not the standards committee.  (In particular, can a constraint
> go from not-enforced to enforced state without getting checked at that
> time?)
>
> Even if you're reading the draft correctly, and the wording makes it
> into a released standard, the implementation you propose would break our
> code.  The incremental FK checks are designed on the assumption that the
> constraint condition held before; they aren't likely to behave very
> sanely if the data is bad.  I'd want to see a whole lot more analysis of
> the resulting behavior before even considering an idea like this.

Wow, you've managed to bash Simon, MySQL, and the SQL standards
committee all in one email.

I'm not going to argue that careful analysis isn't needed before doing
something like this - and, in particular, if we ever get inner-join
removal, which I'm still hoping to do at some point, a foreign key
that isn't actually guaranteed to be valid might result in queries
returning different answers depending on whether or not a join is
removed. I guess we'd have to define that as the user's problem for
alleging a foreign-key relationship that doesn't truly exist. On the
other hand, there's clearly also a use case for this behavior. If a
bulk load of prevalidated data forces an expensive revalidation of
constraints that are already known to hold, there's a real chance the
DBA will be backed into a corner where he simply has no choice but to
not use foreign keys, even though he might really want to validate the
foreign-key relationships on a going-forward basis.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> ... On the
> other hand, there's clearly also a use case for this behavior. If a
> bulk load of prevalidated data forces an expensive revalidation of
> constraints that are already known to hold, there's a real chance the
> DBA will be backed into a corner where he simply has no choice but to
> not use foreign keys, even though he might really want to validate the
> foreign-key relationships on a going-forward basis.

There may well be a case to be made for doing this on grounds of
practical usefulness. I'm just voicing extreme skepticism that it can
be supported by reference to the standard.

Personally I'd prefer to see us look into whether we couldn't arrange
for low-impact establishment of a verified FK relationship, analogous to
CREATE INDEX CONCURRENTLY. We don't let people just arbitrarily claim
that a uniqueness condition exists, and ISTM that if we can handle that
case we probably ought to be able to handle FK checking similarly.

regards, tom lane

On Sun, Dec 12, 2010 at 7:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> ...  On the
>> other hand, there's clearly also a use case for this behavior.  If a
>> bulk load of prevalidated data forces an expensive revalidation of
>> constraints that are already known to hold, there's a real chance the
>> DBA will be backed into a corner where he simply has no choice but to
>> not use foreign keys, even though he might really want to validate the
>> foreign-key relationships on a going-forward basis.
>
> There may well be a case to be made for doing this on grounds of
> practical usefulness.  I'm just voicing extreme skepticism that it can
> be supported by reference to the standard.

Dunno, I haven't read it either. But it does seem like the natural
interpretation of "NOT ENFORCED".

> Personally I'd prefer to see us look into whether we couldn't arrange
> for low-impact establishment of a verified FK relationship, analogous to
> CREATE INDEX CONCURRENTLY.  We don't let people just arbitrarily claim
> that a uniqueness condition exists, and ISTM that if we can handle that
> case we probably ought to be able to handle FK checking similarly.

That'd be useful, too, but I don't think it would remove the use case
for skipping the check altogether.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Dec 12, 2010 at 4:49 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Dec 12, 2010 at 6:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>>> On Sun, 2010-12-12 at 17:57 -0500, Tom Lane wrote:
>>>> Huh?  It allows you to postpone the check until commit.  That's far from
>>>> not enforcing it.
>>
>>> This clearly implies that un-enforced constraints are not checked at
>>> commit.
>>
>> [ shrug... ]  I can't argue with you about what may or may not be in an
>> unpublished draft of an unratified version of the standard, since I
>> don't have a copy.  But allow me to harbor doubts that they really
>> intend to allow someone to force a constraint to be considered valid
>> without any verification.  This proposal strikes me as something mysql
>> would do, not the standards committee.  (In particular, can a constraint
>> go from not-enforced to enforced state without getting checked at that
>> time?)
>>
>> Even if you're reading the draft correctly, and the wording makes it
>> into a released standard, the implementation you propose would break our
>> code.  The incremental FK checks are designed on the assumption that the
>> constraint condition held before; they aren't likely to behave very
>> sanely if the data is bad.  I'd want to see a whole lot more analysis of
>> the resulting behavior before even considering an idea like this.
>
> Wow, you've managed to bash Simon, MySQL, and the SQL standards
> committee all in one email.
>
> I'm not going to argue that careful analysis isn't needed before doing
> something like this - and, in particular, if we ever get inner-join
> removal, which I'm still hoping to do at some point, a foreign key
> that isn't actually guaranteed to be valid might result in queries
> returning different answers depending on whether or not a join is
> removed.  I guess we'd have to define that as the user's problem for
> alleging a foreign-key relationship that doesn't truly exist.  On the
> other hand, there's clearly also a use case for this behavior.  If a
> bulk load of prevalidated data forces an expensive revalidation of
> constraints that are already known to hold, there's a real chance the
> DBA will be backed into a corner where he simply has no choice but to
> not use foreign keys, even though he might really want to validate the
> foreign-key relationships on a going-forward basis.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

MySQL does in fact have this feature and it is used by mysqldump. This
feature is very useful.

--
Rob Wultsch
wultsch(at)gmail(dot)com

On 12/12/2010 08:27 PM, Rob Wultsch wrote:
>
> MySQL does in fact have this feature and it is used by mysqldump. This
> feature is very useful.
>

The trouble is that FK's have more than one use. In particular, they
have a documentary use that's used by tools that analyze databases, as
well as by tools like htsql. They also have a role as an enforced
constraint.

In fact it's possible now to disable FK enforcement, by disabling the
triggers. It's definitely a footgun though. Just the other day I was
asked how data violating the constraint could have got into the table,
and caused some surprise by demonstrating how easy this was to produce.

So what would actually be an advance in my view would be a mechanism
that allowed explicit disabling of a constraint but ensured that it was
not violated when re-enabling it.

cheers

andrew

I wouldn't like to comment on whether or not Simon has correctly
interpreted the words of the SQL standards committee, because
standards committees sometimes word things in an intentionally
ambiguous way to placate different interests, and because it seems
fairly inconsequential in this case. IMHO this is a useful feature
that should be pursued.

There is another precedent that no one mentioned - DB2. From their docs:

"You can add a foreign key with the NOT ENFORCED option to create an
informational referential constraint. This action does not leave the
table space in CHECK-pending status, and you do not need to execute
CHECK DATA."

I understand that DB2's informational referential constraints won't
ever be enforced (they just show intent, which is useful to their
planner), so this isn't really the same thing. However, DB2 apparently
doesn't initially enforce referential integrity when an FK is created
on a table with existing data, without any special syntax on the
CREATE:

"DB2 does not validate the data when you add the foreign key. Instead,
if the table is populated....the table space that contains the table
is placed in CHECK-pending status, just as if it had been loaded with
ENFORCE NO. In this case, you need to execute the CHECK DATA utility
to clear the CHECK-pending status."

If I am not mistaken, this is almost exactly the behaviour described
by Simon, because referential integrity is, presumably, enforced after
the FK is created, but before the CHECK DATA utility is optionally run
to ensure that we actually have referential integrity at a later time.
I believe that Simon's proposal is essentially sound. I don't know why
CHECK DATA operates at the tablespace granularity rather than the FK
granularity - IANADB2U.

If we followed this behaviour, we wouldn't "let people just
arbitrarily claim" that a referential condition exists - rather, we'd
let them assert that it /ought/ to exist, and that it will be
maintained going forward, and give them the option of verifying that
assertion at a later time, after which it actually exists.
Unfortunately, this refinement of Simon's proposal would probably
entail adding an additional column to pg_constraint.

--
Regards,
Peter Geoghegan

On Sun, Dec 12, 2010 at 7:24 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> In fact it's possible now to disable FK enforcement, by disabling the
> triggers. It's definitely a footgun though. Just the other day I was asked
> how data violating the constraint could have got into the table, and caused
> some surprise by demonstrating how easy this was to produce.

Ugh. I have read the entire pg manual and I did not recall that
footgun. At least in MySQL disabling fk's is explicit. There is
something to be said for being able to tell the database: "Hey, hold
my beer and watch this, it might be stupid but it is what we are going
to do". The database telling it's user that is a much larger issue
(and yes, MySQL is generally worse). The user at least gets to talk to
db through sql, the database only really gets to talk to the user
through errors and the manual.

The fact that fk checks are implemented by the trigger system somehow
seems "surprising".

--
Rob Wultsch
wultsch(at)gmail(dot)com

On Sun, 2010-12-12 at 19:07 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > ... On the
> > other hand, there's clearly also a use case for this behavior. If a
> > bulk load of prevalidated data forces an expensive revalidation of
> > constraints that are already known to hold, there's a real chance the
> > DBA will be backed into a corner where he simply has no choice but to
> > not use foreign keys, even though he might really want to validate the
> > foreign-key relationships on a going-forward basis.
>
> There may well be a case to be made for doing this on grounds of
> practical usefulness. I'm just voicing extreme skepticism that it can
> be supported by reference to the standard.
>
> Personally I'd prefer to see us look into whether we couldn't arrange
> for low-impact establishment of a verified FK relationship, analogous to
> CREATE INDEX CONCURRENTLY. We don't let people just arbitrarily claim
> that a uniqueness condition exists, and ISTM that if we can handle that
> case we probably ought to be able to handle FK checking similarly.

I think we should do *both* things. Sometimes you already know the check
will pass, sometimes you don't. In particular, reloading data from
another source where you knew the checks passed. Enforcing re-checking
in that case reduces data availability.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> The incremental FK checks are designed on the assumption that the
> constraint condition held before; they aren't likely to behave very
> sanely if the data is bad. I'd want to see a whole lot more analysis of
> the resulting behavior before even considering an idea like this.

ALTER TABLE foo DISABLE TRIGGER ALL;

I must have missed the point when PostgreSQL stoped providing this foot
gun already, so that it's arguable less a surprise to spell the
misfeature NOT ENFORCED rather than DISABLE TRIGGER.

Seriously, real-world use cases such as Kevin's one seems to warrant
that we are able to create a table withouth enforcing the FK. That's
horrid, yes, that's needed, too. Maybe some operations would have to be
instructed that the constraint ain't trustworthy but just declared to be
so by the user?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

2010/12/13 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> But allow me to harbor doubts that they really
> intend to allow someone to force a constraint to be considered valid
> without any verification.

"Table constraints are either enforced or not enforced. Domain
constraints and assertions are always enforced.", 4.17.2

I don't read that as meaning that such unenforced constraints are
considered "valid". It sounds as if unenforced constraints are the
same as non-existing constraints (think: constraint "templates"),
possibly as a means to "remember" that they should be re-enabled at
some point.

I.e., marking a constraint as unenforced and then as enforced again
would be a shortcut for removing and re-adding the constraint, while
having the advantage that one doesn't have to keep a list of
constraint definitions that must be re-added.

> (In particular, can a constraint
> go from not-enforced to enforced state without getting checked at that
> time?)

I assume not.

Nicolas

On 13 December 2010 10:30, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> Seriously, real-world use cases such as Kevin's one seems to warrant
> that we are able to create a table withouth enforcing the FK. That's
> horrid, yes, that's needed, too. Maybe some operations would have to be
> instructed that the constraint ain't trustworthy but just declared to be
> so by the user?

Might I suggest that we call them "aspirational foreign keys", while
sticking with Simon's syntax?

Reasons:

1. It's suggestive of the lack of certainty about the referential
integrity of the underlying data - They're like a foreign key, but not
quite as good.
2. It's indicative that they may one day become actual foreign keys
through the use of something like the CHECK DATA utility. I'd favour
doing this with a separate DDL command.
3. It's suggestive that they aren't just syntactic sugar or an
expression of intent, as DB2's NOT ENFORCED FKs are, but rather that
they behave like Oracle's NOT ENFORCED FKs.
4. It's memorable, I think.

By the way, the DISABLE TRIGGER ALL method isn't equivalent to this.
Apart from hackishly depending on an implementation detail, it isn't
possible to prevent the big, up-front enforcement in the first place
when the FK is declared, because DISABLE TRIGGER ALL only disables
existing triggers. Perhaps, if and when this feature is implemented,
it will also be possible to use some explicit mechanism to disable and
re-enable an FK. However, that's secondary I think.

--
Regards,
Peter Geoghegan

On Mon, Dec 13, 2010 at 12:59 AM, Rob Wultsch <wultsch(at)gmail(dot)com> wrote:
> On Sun, Dec 12, 2010 at 7:24 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> In fact it's possible now to disable FK enforcement, by disabling the
>> triggers. It's definitely a footgun though. Just the other day I was asked
>> how data violating the constraint could have got into the table, and caused
>> some surprise by demonstrating how easy this was to produce.
>
> Ugh. I have read the entire pg manual and I did not recall that
> footgun.  At least in MySQL disabling fk's is explicit. There is
> something to be said for being able to tell the database: "Hey, hold
> my beer and watch this, it might be stupid but it is what we are going
> to do".

I couldn't agree more, and that's a great way to put it. The user is
in charge. Our job is to prevent the user from *accidentally*
shooting themselves in the foot. But if a crocodile is biting their
foot off and they want to fire their gun in that direction and take
their chances, it's not our job to say "oh, no, you might injure your
foot". DBAs hate getting eaten by crocodiles.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, 2010-12-13 at 11:54 +0000, Peter Geoghegan wrote:
> On 13 December 2010 10:30, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:
> > Seriously, real-world use cases such as Kevin's one seems to warrant
> > that we are able to create a table withouth enforcing the FK. That's
> > horrid, yes, that's needed, too. Maybe some operations would have to be
> > instructed that the constraint ain't trustworthy but just declared to be
> > so by the user?
>
> Might I suggest that we call them "aspirational foreign keys", while
> sticking with Simon's syntax?

Just checking what we are saying:

1.
(a) ALTER TABLE ... ADD FOREIGN KEY ... NOT VALIDATED INITIALLY;
will add a FK but NOT run the check - we mark it as "check pending".
Lock held: ShareRowExclusiveLock

(b) Every new change to the table has the FK enforced - the triggers are
fully enabled and active. (That could mean we update a row and have the
update fail because of a FK violation.)

2. pg_validate_foreign_key('constraint name');
Returns immediately if FK is valid
Returns SETOF rows that violate the constraint, or if no rows are
returned it updates constraint to show it is now valid.
Lock held: AccessShareLock

Note that 1 & 2 together are the equivalent of ADD FK CONCURRENTLY,
except that step 2 more usefully tells you which rows fail.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> (a) ALTER TABLE ... ADD FOREIGN KEY ... NOT VALIDATED INITIALLY;
> will add a FK but NOT run the check - we mark it as "check pending".
> Lock held: ShareRowExclusiveLock

Seems about right. Not sure whether the lock strength is correct.

> (b) Every new change to the table has the FK enforced - the triggers are
> fully enabled and active. (That could mean we update a row and have the
> update fail because of a FK violation.)

Also seems about right.

> 2. pg_validate_foreign_key('constraint name');
> Returns immediately if FK is valid
> Returns SETOF rows that violate the constraint, or if no rows are
> returned it updates constraint to show it is now valid.
> Lock held: AccessShareLock

I'm less sure about this part. I think there should be a DDL
statement to validate the foreign key. The "return the problem" rows
behavior could be done some other way, or just left to the user to
write their own query.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 13 December 2010 16:08, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> 2. pg_validate_foreign_key('constraint name');
>> Returns immediately if FK is valid
>> Returns SETOF rows that violate the constraint, or if no rows are
>> returned it updates constraint to show it is now valid.
>> Lock held: AccessShareLock
>
> I'm less sure about this part.  I think there should be a DDL
> statement to validate the foreign key.  The "return the problem" rows
> behavior could be done some other way, or just left to the user to
> write their own query.

+1. I think that a DDL statement is more appropriate, because it makes
the process sort of symmetrical.

Perhaps we could error on the first FK violation found, and give a
"value 'foo' not present in table bar". It ought to not matter that
there could be a lot of violations, because they will be exceptional
if you're using the feature as intended - presumably, you're going to
want to comb through the data to find out exactly what has gone wrong
for each violation. On the off chance that it actually is a problem,
the user can go ahead and write their own query, like Robert
suggested.

--
Regards,
Peter Geoghegan

On Mon, Dec 13, 2010 at 05:15:29PM +0000, Peter Geoghegan wrote:
> On 13 December 2010 16:08, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> 2. pg_validate_foreign_key('constraint name');
> >> Returns immediately if FK is valid
> >> Returns SETOF rows that violate the constraint, or if no rows are
> >> returned it updates constraint to show it is now valid.
> >> Lock held: AccessShareLock
> >
> > I'm less sure about this part.  I think there should be a DDL
> > statement to validate the foreign key.  The "return the problem" rows
> > behavior could be done some other way, or just left to the user to
> > write their own query.
>
> +1. I think that a DDL statement is more appropriate, because it makes
> the process sort of symmetrical.
>
> Perhaps we could error on the first FK violation found, and give a
> "value 'foo' not present in table bar". It ought to not matter that
> there could be a lot of violations, because they will be exceptional
> if you're using the feature as intended - presumably, you're going to
> want to comb through the data to find out exactly what has gone wrong
> for each violation. On the off chance that it actually is a problem,
> the user can go ahead and write their own query, like Robert
> suggested.

Perhaps the errhint could suggest a query. All the information needed
for it would be available :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, 2010-12-13 at 17:15 +0000, Peter Geoghegan wrote:
> On 13 December 2010 16:08, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> 2. pg_validate_foreign_key('constraint name');
> >> Returns immediately if FK is valid
> >> Returns SETOF rows that violate the constraint, or if no rows are
> >> returned it updates constraint to show it is now valid.
> >> Lock held: AccessShareLock
> >
> > I'm less sure about this part. I think there should be a DDL
> > statement to validate the foreign key. The "return the problem" rows
> > behavior could be done some other way, or just left to the user to
> > write their own query.
>
> +1. I think that a DDL statement is more appropriate, because it makes
> the process sort of symmetrical.

OK, sold.

> Perhaps we could error on the first FK violation found, and give a
> "value 'foo' not present in table bar". It ought to not matter that
> there could be a lot of violations, because they will be exceptional
> if you're using the feature as intended - presumably, you're going to
> want to comb through the data to find out exactly what has gone wrong
> for each violation. On the off chance that it actually is a problem,
> the user can go ahead and write their own query, like Robert
> suggested.

I think a function would help here, so I'll do that also.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> ... On the
>> other hand, there's clearly also a use case for this behavior. If a
>> bulk load of prevalidated data forces an expensive revalidation of
>> constraints that are already known to hold, there's a real chance the
>> DBA will be backed into a corner where he simply has no choice but to
>> not use foreign keys, even though he might really want to validate the
>> foreign-key relationships on a going-forward basis.
>
> There may well be a case to be made for doing this on grounds of
> practical usefulness. I'm just voicing extreme skepticism that it can
> be supported by reference to the standard.
>
> Personally I'd prefer to see us look into whether we couldn't arrange
> for low-impact establishment of a verified FK relationship, analogous to
> CREATE INDEX CONCURRENTLY. We don't let people just arbitrarily claim
> that a uniqueness condition exists, and ISTM that if we can handle that
> case we probably ought to be able to handle FK checking similarly.

I can point to a use case that has proven useful...

Slony-I deactivates indices during the subscription process, because it
is enormously more efficient to load the data into the tables
sans-indices, and then re-index afterwards.

The same would apply for FK constraints.

I observe that the deactivation of indices is the sole remaining feature
in Slony-I that still requires catalog access in a "corruptive" sense.
(With the caveat that this corruption is now only a temporary one; the
indexes are returned into play before the subscription process
finishes.)

That would be eliminated by adding in:
"ALTER TABLE ... DISABLE INDEX ..."
"ALTER TABLE ... ENABLE INDEX ..."

For similar to apply to FK constraints would involve similar logic.
--
output = reverse("moc.liamg" "@" "enworbbc")
http://linuxdatabases.info/info/rdbms.html
"The code should be beaten into submission" -- Arthur Norman

Robert Haas wrote:
> On Mon, Dec 13, 2010 at 12:59 AM, Rob Wultsch <wultsch(at)gmail(dot)com> wrote:
> > On Sun, Dec 12, 2010 at 7:24 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> >> In fact it's possible now to disable FK enforcement, by disabling the
> >> triggers. It's definitely a footgun though. Just the other day I was asked
> >> how data violating the constraint could have got into the table, and caused
> >> some surprise by demonstrating how easy this was to produce.
> >
> > Ugh. I have read the entire pg manual and I did not recall that
> > footgun. ?At least in MySQL disabling fk's is explicit. There is
> > something to be said for being able to tell the database: "Hey, hold
> > my beer and watch this, it might be stupid but it is what we are going
> > to do".
>
> I couldn't agree more, and that's a great way to put it. The user is
> in charge. Our job is to prevent the user from *accidentally*
> shooting themselves in the foot. But if a crocodile is biting their
> foot off and they want to fire their gun in that direction and take
> their chances, it's not our job to say "oh, no, you might injure your
> foot". DBAs hate getting eaten by crocodiles.

Is this a TODO?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Wed, 2011-01-05 at 22:05 -0500, Bruce Momjian wrote:
> Robert Haas wrote:
> > On Mon, Dec 13, 2010 at 12:59 AM, Rob Wultsch <wultsch(at)gmail(dot)com> wrote:
> > > On Sun, Dec 12, 2010 at 7:24 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> > >> In fact it's possible now to disable FK enforcement, by disabling the
> > >> triggers. It's definitely a footgun though. Just the other day I was asked
> > >> how data violating the constraint could have got into the table, and caused
> > >> some surprise by demonstrating how easy this was to produce.
> > >
> > > Ugh. I have read the entire pg manual and I did not recall that
> > > footgun. ?At least in MySQL disabling fk's is explicit. There is
> > > something to be said for being able to tell the database: "Hey, hold
> > > my beer and watch this, it might be stupid but it is what we are going
> > > to do".
> >
> > I couldn't agree more, and that's a great way to put it. The user is
> > in charge. Our job is to prevent the user from *accidentally*
> > shooting themselves in the foot. But if a crocodile is biting their
> > foot off and they want to fire their gun in that direction and take
> > their chances, it's not our job to say "oh, no, you might injure your
> > foot". DBAs hate getting eaten by crocodiles.
>
> Is this a TODO?

The patch I'll be submitting, or getting eaten by crocodiles?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Mon, 2010-12-13 at 17:15 +0000, Peter Geoghegan wrote:
> On 13 December 2010 16:08, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> 2. pg_validate_foreign_key('constraint name');
> >> Returns immediately if FK is valid
> >> Returns SETOF rows that violate the constraint, or if no rows are
> >> returned it updates constraint to show it is now valid.
> >> Lock held: AccessShareLock
> >
> > I'm less sure about this part. I think there should be a DDL
> > statement to validate the foreign key. The "return the problem" rows
> > behavior could be done some other way, or just left to the user to
> > write their own query.
>
> +1. I think that a DDL statement is more appropriate, because it makes
> the process sort of symmetrical.

Patch to implement the proposed feature attached, for CFJan2011.

2 sub-command changes:

ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;

ALTER TABLE foo VALIDATE CONSTRAINT fkoo;

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Jan 14, 2011, at 5:15 AM, Simon Riggs wrote:
> On Mon, 2010-12-13 at 17:15 +0000, Peter Geoghegan wrote:
>> On 13 December 2010 16:08, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Mon, Dec 13, 2010 at 10:49 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> 2. pg_validate_foreign_key('constraint name');
>>>> Returns immediately if FK is valid
>>>> Returns SETOF rows that violate the constraint, or if no rows are
>>>> returned it updates constraint to show it is now valid.
>>>> Lock held: AccessShareLock
>>>
>>> I'm less sure about this part. I think there should be a DDL
>>> statement to validate the foreign key. The "return the problem" rows
>>> behavior could be done some other way, or just left to the user to
>>> write their own query.
>>
>> +1. I think that a DDL statement is more appropriate, because it makes
>> the process sort of symmetrical.
>
> Patch to implement the proposed feature attached, for CFJan2011.
>
> 2 sub-command changes:
>
> ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
>
> ALTER TABLE foo VALIDATE CONSTRAINT fkoo;

Sorry for the late reply; I just saw this...

Is there any way to be able to get the bad records out of the ALTER ... VALIDATE? I know it's pretty unusual, but for a set of large tables, it could take hours to run a separate query that gives you the results.

BTW, I agree that this should be a DDL command, it would be very odd if it wasn't. But I also see it being very useful to be able to get the set of bad rows at the same time. Maybe if there was an SRF that did the real work and the ALTER just ignored the resultset?
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Tue, 2011-01-18 at 14:26 -0600, Jim Nasby wrote:
> >
> > 2 sub-command changes:
> >
> > ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
> >
> > ALTER TABLE foo VALIDATE CONSTRAINT fkoo;
>
> Sorry for the late reply; I just saw this...
>
> Is there any way to be able to get the bad records out of the ALTER ... VALIDATE? I know it's pretty unusual, but for a set of large tables, it could take hours to run a separate query that gives you the results.
>
> BTW, I agree that this should be a DDL command, it would be very odd if it wasn't. But I also see it being very useful to be able to get the set of bad rows at the same time. Maybe if there was an SRF that did the real work and the ALTER just ignored the resultset?

I agree. Unfortunately that wasn't the consensus when I proposed that
earlier, so its just a simple validation true/false.

I could add an SRF that ran the validation query but brought back the
rows, but if zero then it sets valid. We could have both...

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

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

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

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

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

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

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

Simon Riggs <simon(at)2ndQuadrant(dot)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

On Sun, 2011-01-23 at 14:45 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)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

On Tue, 2010-12-14 at 11:24 -0500, Chris Browne wrote:
> tgl(at)sss(dot)pgh(dot)pa(dot)us (Tom Lane) writes:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> ... On the
> >> other hand, there's clearly also a use case for this behavior. If a
> >> bulk load of prevalidated data forces an expensive revalidation of
> >> constraints that are already known to hold, there's a real chance the
> >> DBA will be backed into a corner where he simply has no choice but to
> >> not use foreign keys, even though he might really want to validate the
> >> foreign-key relationships on a going-forward basis.
> >
> > There may well be a case to be made for doing this on grounds of
> > practical usefulness. I'm just voicing extreme skepticism that it can
> > be supported by reference to the standard.
> >
> > Personally I'd prefer to see us look into whether we couldn't arrange
> > for low-impact establishment of a verified FK relationship, analogous to
> > CREATE INDEX CONCURRENTLY. We don't let people just arbitrarily claim
> > that a uniqueness condition exists, and ISTM that if we can handle that
> > case we probably ought to be able to handle FK checking similarly.
>
> I can point to a use case that has proven useful...
>
> Slony-I deactivates indices during the subscription process, because it
> is enormously more efficient to load the data into the tables
> sans-indices, and then re-index afterwards.
>
> The same would apply for FK constraints.
>
> I observe that the deactivation of indices is the sole remaining feature
> in Slony-I that still requires catalog access in a "corruptive" sense.
> (With the caveat that this corruption is now only a temporary one; the
> indexes are returned into play before the subscription process
> finishes.)
>
> That would be eliminated by adding in:
> "ALTER TABLE ... DISABLE INDEX ..."
> "ALTER TABLE ... ENABLE INDEX ..."
>
> For similar to apply to FK constraints would involve similar logic.

I just wanted to point out that the patch submitted here does not allow
what is requested here for FKs (nor indexes).

You can add an FK without an initial check, but the FK is enforced for
all further DML changes. You can then later validate the FK. So that
running these commands

ALTER TABLE foo ADD FOREIGN KEY ... NOT VALID;
ALTER TABLE foo VALIDATE CONSTRAINT ...;

is roughly equivalent to the concept of

ALTER TABLE foo ADD FOREIGN KEY ... CONCURRENTLY;

There is no command that makes the FK "NOT ENFORCED", so you can't turn
it off then back on again as was requested above.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> You can add an FK without an initial check, but the FK is enforced for
> all further DML changes.

I seem to recall pointing out upthread that the FK check triggers are
designed on the assumption that the constraint does hold currently.
Has any analysis been done on exactly how badly they'll fail when it
doesn't hold? I remain unconvinced that this behavior is desirable.

regards, tom lane

On Sun, 2011-01-23 at 16:13 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > You can add an FK without an initial check, but the FK is enforced for
> > all further DML changes.
>
> I seem to recall pointing out upthread that the FK check triggers are
> designed on the assumption that the constraint does hold currently.
> Has any analysis been done on exactly how badly they'll fail when it
> doesn't hold? I remain unconvinced that this behavior is desirable.

If you mean RESTRICT relationships, then yes.

I haven't foreseen other problems myself. What other ideas or risks
would you like me to confirm or deny for you?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

simon(at)2ndQuadrant(dot)com (Simon Riggs) writes:
> I just wanted to point out that the patch submitted here does not allow
> what is requested here for FKs (nor indexes).

That's fine; I was trying to support the thought that there was
something useful about this idea. Being able to expressly deactivate
indices seems like a reasonable thing to add as a separate TODO item,
and I'll see about doing so.
--
MICROS~1 has brought the microcomputer OS to the point where it is
more bloated than even OSes from what was previously larger classes of
machines altogether. This is perhaps Bill's single greatest
accomplishment.

On Fri, Jan 14, 2011 at 6:15 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Patch to implement the proposed feature attached, for CFJan2011.
>
> 2 sub-command changes:
>
> ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
>
> ALTER TABLE foo VALIDATE CONSTRAINT fkoo;

This patch, which seems to be the latest version, no longer applies,
and has not been updated based on the previous provided review
comments.

Also, this diff hunk looks scary to me:

+ if (must_use_query)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot SELECT from primary
key of relation \"%s\"",
+ RelationGetRelationName(rel))));
+

What's the justification for that?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Feb 3, 2011 at 11:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 14, 2011 at 6:15 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Patch to implement the proposed feature attached, for CFJan2011.
>>
>> 2 sub-command changes:
>>
>> ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
>>
>> ALTER TABLE foo VALIDATE CONSTRAINT fkoo;
>
> This patch, which seems to be the latest version, no longer applies,
> and has not been updated based on the previous provided review
> comments.
>
> Also, this diff hunk looks scary to me:
>
> +       if (must_use_query)
> +               ereport(ERROR,
> +                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                                errmsg("cannot SELECT from primary
> key of relation \"%s\"",
> +                                               RelationGetRelationName(rel))));
> +
>
> What's the justification for that?

Since this patch was reviewed on January 23rd by Marko Tiikkaja and
more briefly on February 3rd by me, and has not been updated, I am
marking it Returned with Feedback.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 8 February 2011 03:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Feb 3, 2011 at 11:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jan 14, 2011 at 6:15 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> Patch to implement the proposed feature attached, for CFJan2011.
>>>
>>> 2 sub-command changes:
>>>
>>> ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
>>>
>>> ALTER TABLE foo VALIDATE CONSTRAINT fkoo;
>>
>> This patch, which seems to be the latest version, no longer applies,
>> and has not been updated based on the previous provided review
>> comments.
>>
>> Also, this diff hunk looks scary to me:
>>
>> +       if (must_use_query)
>> +               ereport(ERROR,
>> +                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> +                                errmsg("cannot SELECT from primary
>> key of relation \"%s\"",
>> +                                               RelationGetRelationName(rel))));
>> +
>>
>> What's the justification for that?
>
> Since this patch was reviewed on January 23rd by Marko Tiikkaja and
> more briefly on February 3rd by me, and has not been updated, I am
> marking it Returned with Feedback.

Just a note that since Alvaro created a patch to provide similar
functionality for constraints, I identified an issue with database
dumps, which apparently affects invalid foreign keys too:
http://archives.postgresql.org/pgsql-hackers/2011-06/msg00162.php

In other words, a database containing foreign keys that hasn't yet
been validated will not produce a dump containing the necessary NOT
VALID parameters. This would be fixed by Alvaro's patch.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 2, 2011 at 5:34 PM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 8 February 2011 03:50, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Feb 3, 2011 at 11:00 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Jan 14, 2011 at 6:15 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>>> Patch to implement the proposed feature attached, for CFJan2011.
>>>>
>>>> 2 sub-command changes:
>>>>
>>>> ALTER TABLE foo ADD FOREIGN KEY fkoo ... NOT VALID;
>>>>
>>>> ALTER TABLE foo VALIDATE CONSTRAINT fkoo;
>>>
>>> This patch, which seems to be the latest version, no longer applies,
>>> and has not been updated based on the previous provided review
>>> comments.
>>>
>>> Also, this diff hunk looks scary to me:
>>>
>>> +       if (must_use_query)
>>> +               ereport(ERROR,
>>> +                               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>>> +                                errmsg("cannot SELECT from primary
>>> key of relation \"%s\"",
>>> +                                               RelationGetRelationName(rel))));
>>> +
>>>
>>> What's the justification for that?
>>
>> Since this patch was reviewed on January 23rd by Marko Tiikkaja and
>> more briefly on February 3rd by me, and has not been updated, I am
>> marking it Returned with Feedback.
>
> Just a note that since Alvaro created a patch to provide similar
> functionality for constraints, I identified an issue with database
> dumps, which apparently affects invalid foreign keys too:
> http://archives.postgresql.org/pgsql-hackers/2011-06/msg00162.php
>
> In other words, a database containing foreign keys that hasn't yet
> been validated will not produce a dump containing the necessary NOT
> VALID parameters.  This would be fixed by Alvaro's patch.

Sounds like someone needs to extract and apply that portion of
Alvaro's patch. I've added this to the open items list.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of vie jun 03 09:34:03 -0400 2011:

> > Just a note that since Alvaro created a patch to provide similar
> > functionality for constraints, I identified an issue with database
> > dumps, which apparently affects invalid foreign keys too:
> > http://archives.postgresql.org/pgsql-hackers/2011-06/msg00162.php
> >
> > In other words, a database containing foreign keys that hasn't yet
> > been validated will not produce a dump containing the necessary NOT
> > VALID parameters.  This would be fixed by Alvaro's patch.
>
> Sounds like someone needs to extract and apply that portion of
> Alvaro's patch. I've added this to the open items list.

It's already a separate patch; I'll apply soon.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support