More FK patches
This patch should help fix cases with two separate fk constraints
in a row that happen to reference the same pk constraint with
an on update cascade and similar cases. It now should detect
correctly that a pk row was added later after a delete or update
on a no action deferred fk and not incorrect error as well
as dropping a check on insert/update to the fk table if the
row we're being referred to no longer is valid (I'm using
HeapTupleSatisfiesItself because the comment implied it was
what I was looking for and it appears to work :) ).
I've got regression tests but I'm holding off on those until
we decide whether or not we're dropping the triggered data change
errors since a couple of the tests would hit that case and I'll
either change them or drop them if we're not dropping the error.
Attachments:
fix1.patchtext/plain; charset=US-ASCII; name=fix1.patchDownload
*** pgsql/src/backend/utils/adt/ri_triggers_old.c Sat Nov 10 00:01:56 2001
--- pgsql/src/backend/utils/adt/ri_triggers.c Sat Nov 10 00:31:54 2001
***************
*** 218,223 ****
--- 218,234 ----
new_row = trigdata->tg_trigtuple;
}
+ /*
+ * We should not even consider checking the row if it is no longer
+ * valid since it was either deleted (doesn't matter) or updated
+ * (in which case it'll be checked with its final values).
+ */
+ if (new_row) {
+ if (!HeapTupleSatisfiesItself(new_row->t_data)) {
+ return PointerGetDatum(NULL);
+ }
+ }
+
/* ----------
* SQL3 11.9 <referential constraint definition>
* Gereral rules 2) a):
***************
*** 613,643 ****
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
char buf[256];
! char querystr[8192];
char *querysep;
Oid queryoids[RI_MAX_NUMKEYS];
/* ----------
* The query string built is
* SELECT 1 FROM ONLY <fktable> WHERE fkatt1 = $1 [AND ...]
* The type id's for the $ parameters are those of the
* corresponding PK attributes. Thus, SPI_prepare could
* eventually fail if the parser cannot identify some way
* how to compare these two types by '='.
* ----------
*/
sprintf(querystr, "SELECT 1 FROM ONLY \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
querysep = "WHERE";
for (i = 0; i < qkey.nkeypairs; i++)
{
sprintf(buf, " %s \"%s\" = $%d", querysep,
tgargs[4 + i * 2], i + 1);
strcat(querystr, buf);
querysep = "AND";
queryoids[i] = SPI_gettypeid(pk_rel->rd_att,
qkey.keypair[i][RI_KEYPAIR_PK_IDX]);
}
sprintf(buf, " FOR UPDATE OF \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
strcat(querystr, buf);
--- 624,668 ----
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
char buf[256];
! char querystr[16384];
! char notexiststr[8192];
char *querysep;
Oid queryoids[RI_MAX_NUMKEYS];
/* ----------
* The query string built is
* SELECT 1 FROM ONLY <fktable> WHERE fkatt1 = $1 [AND ...]
+ * AND NOT EXISTS (SELECT 1 FROM ONLY <pktable> WHERE pkatt1 = $1 [AND ...])
* The type id's for the $ parameters are those of the
* corresponding PK attributes. Thus, SPI_prepare could
* eventually fail if the parser cannot identify some way
* how to compare these two types by '='.
+ * 11/10/01 - The not exists clause was added to the previous
+ * definition to handle the case where a new row is added to the
+ * pktable which has the same key values (and thus the constraint
+ * is satisified).
* ----------
*/
sprintf(querystr, "SELECT 1 FROM ONLY \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
+ sprintf(notexiststr, "SELECT 1 FROM ONLY \"%s\"",
+ tgargs[RI_PK_RELNAME_ARGNO]);
querysep = "WHERE";
for (i = 0; i < qkey.nkeypairs; i++)
{
sprintf(buf, " %s \"%s\" = $%d", querysep,
tgargs[4 + i * 2], i + 1);
strcat(querystr, buf);
+ sprintf(buf, " %s \"%s\" = $%d", querysep,
+ tgargs[5 + i * 2], i + 1);
+ strcat(notexiststr, buf);
querysep = "AND";
queryoids[i] = SPI_gettypeid(pk_rel->rd_att,
qkey.keypair[i][RI_KEYPAIR_PK_IDX]);
}
+ strcat(querystr, " AND NOT EXISTS (");
+ strcat(querystr, notexiststr);
+ strcat(querystr, ")");
sprintf(buf, " FOR UPDATE OF \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
strcat(querystr, buf);
***************
*** 827,839 ****
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
char buf[256];
! char querystr[8192];
char *querysep;
Oid queryoids[RI_MAX_NUMKEYS];
/* ----------
* The query string built is
* SELECT 1 FROM ONLY <fktable> WHERE fkatt1 = $1 [AND ...]
* The type id's for the $ parameters are those of the
* corresponding PK attributes. Thus, SPI_prepare could
* eventually fail if the parser cannot identify some way
--- 852,866 ----
if ((qplan = ri_FetchPreparedPlan(&qkey)) == NULL)
{
char buf[256];
! char querystr[16384];
! char notexiststr[8192];
char *querysep;
Oid queryoids[RI_MAX_NUMKEYS];
/* ----------
* The query string built is
* SELECT 1 FROM ONLY <fktable> WHERE fkatt1 = $1 [AND ...]
+ * AND NOT EXISTS (SELECT 1 FROM ONLY <pktable> WHERE pkatt1 = $1 [AND ...])
* The type id's for the $ parameters are those of the
* corresponding PK attributes. Thus, SPI_prepare could
* eventually fail if the parser cannot identify some way
***************
*** 842,857 ****
--- 869,892 ----
*/
sprintf(querystr, "SELECT 1 FROM ONLY \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
+ sprintf(notexiststr, "SELECT 1 FROM ONLY \"%s\"",
+ tgargs[RI_PK_RELNAME_ARGNO]);
querysep = "WHERE";
for (i = 0; i < qkey.nkeypairs; i++)
{
sprintf(buf, " %s \"%s\" = $%d", querysep,
tgargs[4 + i * 2], i + 1);
strcat(querystr, buf);
+ sprintf(buf, " %s \"%s\" = $%d", querysep,
+ tgargs[5 + i * 2], i + 1);
+ strcat(notexiststr, buf);
querysep = "AND";
queryoids[i] = SPI_gettypeid(pk_rel->rd_att,
qkey.keypair[i][RI_KEYPAIR_PK_IDX]);
}
+ strcat(querystr, " AND NOT EXISTS (");
+ strcat(querystr, notexiststr);
+ strcat(querystr, ")");
sprintf(buf, " FOR UPDATE OF \"%s\"",
tgargs[RI_FK_RELNAME_ARGNO]);
strcat(querystr, buf);
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
This patch should help fix cases with two separate fk constraints
in a row that happen to reference the same pk constraint with
an on update cascade and similar cases.
Aren't those NOT EXISTS clauses going to cause a humungous
performance hit?
Seems it would be better for the RI triggers to do more in C code
and stop expecting the query engine to handle these things. I've
always thought that ReferentialIntegritySnapshotOverride was an
absolutely unacceptable kluge, not least because it's turned on
*before* we do parsing/planning of the RI queries, and so is
likely to screw up system catalog checks.
regards, tom lane
On Mon, 12 Nov 2001, Tom Lane wrote:
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
This patch should help fix cases with two separate fk constraints
in a row that happen to reference the same pk constraint with
an on update cascade and similar cases.Aren't those NOT EXISTS clauses going to cause a humungous
performance hit?
You're right. Thinking about it, it would make more sense to check it
once for the cases we support, since the only case where a different
row would come up would be in match partial. So that should probably
go away to a direct search for a matching row.
Seems it would be better for the RI triggers to do more in C code
and stop expecting the query engine to handle these things. I've
always thought that ReferentialIntegritySnapshotOverride was an
absolutely unacceptable kluge, not least because it's turned on
*before* we do parsing/planning of the RI queries, and so is
likely to screw up system catalog checks.
Well, would it time correctly if the override was only around the
actual execp rather than the prepare and such?
Do you think it would be better to directly implement the constraint
checks and actions using scans and C modifying rows rather than the
query planner and switch over in 7.3? Without looking too hard yet, I'd be
worried that it'd end up reimplementing alot of glue code that already
exists, but maybe that's not so bad. I'm willing to give it a shot, but
I couldn't guarantee anything and I'd also like to know the reasoning Jan
had for his decisions so as to make an informed attempt. :)
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
Well, would it time correctly if the override was only around the
actual execp rather than the prepare and such?
That would definitely feel better, but ultimately global variables
changing the behavior of low-level subroutines are Bad News.
Do you think it would be better to directly implement the constraint
checks and actions using scans and C modifying rows rather than the
query planner and switch over in 7.3?
I believe that's the way to go in the long run, but I don't have any
idea how much work might be involved. What I don't like about the
present setup is (a) the overhead involved, and (b) the fact that we
can't implement quite the right semantics using only user-level queries.
SELECT FOR UPDATE doesn't get the kind of lock we want, and there are
these other issues too.
I'd also like to know the reasoning Jan had for his decisions so as to
make an informed attempt. :)
I should probably let Jan speak for himself, but I'm guessing that
it was an easy way to get a prototype implementation up and going.
That was fine at the time --- it was a pretty neat hack, in fact.
But we need to start thinking about an industrial-strength implementation.
regards, tom lane
Was this resolved?
---------------------------------------------------------------------------
This patch should help fix cases with two separate fk constraints
in a row that happen to reference the same pk constraint with
an on update cascade and similar cases. It now should detect
correctly that a pk row was added later after a delete or update
on a no action deferred fk and not incorrect error as well
as dropping a check on insert/update to the fk table if the
row we're being referred to no longer is valid (I'm using
HeapTupleSatisfiesItself because the comment implied it was
what I was looking for and it appears to work :) ).I've got regression tests but I'm holding off on those until
we decide whether or not we're dropping the triggered data change
errors since a couple of the tests would hit that case and I'll
either change them or drop them if we're not dropping the error.
Content-Description:
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026On Wed, 21 Nov 2001, Bruce Momjian wrote:
Was this resolved?
Not yet. Per Tom's message on this, I've started looking at working
on doing more of the fk stuff without relying on SPI. I could do
a version of the patch that did a single query for the two no action
cases rather than the not exists, but I'm not sure if that's worth
it then.
Is there a TODO item to add then?
---------------------------------------------------------------------------
On Wed, 21 Nov 2001, Bruce Momjian wrote:
Was this resolved?
Not yet. Per Tom's message on this, I've started looking at working
on doing more of the fk stuff without relying on SPI. I could do
a version of the patch that did a single query for the two no action
cases rather than the not exists, but I'm not sure if that's worth
it then.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026On Thu, 22 Nov 2001, Bruce Momjian wrote:
Is there a TODO item to add then?
Fix foreign key constraints to not error on intermediate states
of the database.
On Thu, 22 Nov 2001, Bruce Momjian wrote:
Is there a TODO item to add then?
Fix foreign key constraints to not error on intermediate states
of the database.
Added to TODO. Thanks. I put your name on it. :-)
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026