Re: ON COMMIT and foreign keys

Hackers,

There's a bug with temporary tables signalled ON COMMIT DELETE ROWS,
when they contain foreign key references. An example:

alvherre=# begin;
BEGIN
alvherre=# CREATE TEMP TABLE foo (a int PRIMARY KEY) ON COMMIT DELETE ROWS;
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo"
CREATE TABLE
alvherre=# CREATE TEMP TABLE bar (a int REFERENCES foo) ON COMMIT DELETE ROWS;
CREATE TABLE
alvherre=# COMMIT;
ERROR: cannot truncate a table referenced in a foreign key constraint
DETAIL: Table "bar" references "foo" via foreign key constraint "bar_a_fkey".

Say again? Certainly this shouldn't happen, because both tables are
supposed to lose rows on transaction commit. But this isn't working.

The attached patch fixes this bug. (In all likelyhood, not a lot of
people uses referential integrity on temp tables, and that's why this
hasn't been reported. But it's a bug anyway.)

Incidentally ("collateral damage"), the patch modifies the TRUNCATE
command so that it can work on multiple tables. In particular, if
foreign key references are all internal to the group that's being
truncated, the command is allowed.

There's one thing that bothers me on this patch: the fact that
pg_constraint has to be scanned multiple times, and they are all
seqscans. Not sure what to do about that. Maybe there's a way to do
better?

Also, observe that when the TRUNCATE operation is aborted because of a
foreign key, a HINT is emitted as well telling the user to truncate the
referencing table too. This is IMHO a good hint, but it may be
misleading when the truncation has taken the ON COMMIT DELETE ROWS path.
Not sure if it's worth fixing (maybe the hint should suggest to add ON
COMMIT DELETE ROWS to the referencing table as well?).

Please have a look. The patch is not as intrusive as it looks; there's
a lot of whitespace change.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> Incidentally ("collateral damage"), the patch modifies the TRUNCATE
> command so that it can work on multiple tables. In particular, if
> foreign key references are all internal to the group that's being
> truncated, the command is allowed.

I think this is a feature addition that you've cleverly managed to
present as a bug fix ;-). While I like the feature, I've got very
mixed emotions about applying it this late in beta. The "bug" exists
in 7.4 but has yet to be reported in the field, which says to me that
we could leave it unfixed in 8.0 as well. Conservatism would suggest
holding the change for 8.1. OTOH, multiple TRUNCATE would be a useful
feature --- it came up just the other day.

Comments anyone?

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> > Incidentally ("collateral damage"), the patch modifies the TRUNCATE
> > command so that it can work on multiple tables. In particular, if
> > foreign key references are all internal to the group that's being
> > truncated, the command is allowed.
>
> I think this is a feature addition that you've cleverly managed to
> present as a bug fix ;-). While I like the feature, I've got very
> mixed emotions about applying it this late in beta. The "bug" exists
> in 7.4 but has yet to be reported in the field, which says to me that
> we could leave it unfixed in 8.0 as well. Conservatism would suggest
> holding the change for 8.1. OTOH, multiple TRUNCATE would be a useful
> feature --- it came up just the other day.
>
> Comments anyone?

Alvaro, would you post the patch skipping whitespace changes. As posted
the patch does look very large, as you suggested.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> > > Incidentally ("collateral damage"), the patch modifies the TRUNCATE
> > > command so that it can work on multiple tables. In particular, if
> > > foreign key references are all internal to the group that's being
> > > truncated, the command is allowed.
> >
> > I think this is a feature addition that you've cleverly managed to
> > present as a bug fix ;-). While I like the feature, I've got very
> > mixed emotions about applying it this late in beta. The "bug" exists
> > in 7.4 but has yet to be reported in the field, which says to me that
> > we could leave it unfixed in 8.0 as well. Conservatism would suggest
> > holding the change for 8.1. OTOH, multiple TRUNCATE would be a useful
> > feature --- it came up just the other day.
> >
> > Comments anyone?
>
> Alvaro, would you post the patch skipping whitespace changes. As posted
> the patch does look very large, as you suggested.

I generated the diff myself, attached.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

> Tom Lane wrote:
> > Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> > > Incidentally ("collateral damage"), the patch modifies the TRUNCATE
> > > command so that it can work on multiple tables. In particular, if
> > > foreign key references are all internal to the group that's being
> > > truncated, the command is allowed.
> >
> > I think this is a feature addition that you've cleverly managed to
> > present as a bug fix ;-). While I like the feature, I've got very
> > mixed emotions about applying it this late in beta.

Ok, you caught me :-) Actually, I developed the patch thinking only on
the multi-relation TRUNCATE, with the goal of having it for 8.1. Only
after I prepared a first patch I noticed the temp-table-FKs problem.

So I don't really care if it has to wait for 8.1 because that was the
original intention anyway.

> > OTOH, multiple TRUNCATE would be a useful feature --- it came up
> > just the other day.

Right, that's why I did it in the first place.

On Sat, Nov 06, 2004 at 12:56:24AM -0500, Bruce Momjian wrote:

> Alvaro, would you post the patch skipping whitespace changes. As posted
> the patch does look very large, as you suggested.

Ok, turns out it wasn't that much smaller anyway :-) But really it's
only stashing a couple of foreach loops in two places, which is a fairly
trivial change; and the other important part is the modification of
heap_truncate_check_FKs and it's fairly localized anyway.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
>> Tom Lane wrote:
> I think this is a feature addition that you've cleverly managed to
> present as a bug fix ;-). While I like the feature, I've got very
> mixed emotions about applying it this late in beta.

> Ok, you caught me :-) Actually, I developed the patch thinking only on
> the multi-relation TRUNCATE, with the goal of having it for 8.1. Only
> after I prepared a first patch I noticed the temp-table-FKs problem.

Well, like I said, I have mixed emotions. Anyone else have an opinion
whether to apply now or hold for 8.1?

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)cl> writes:
> >> Tom Lane wrote:
> > I think this is a feature addition that you've cleverly managed to
> > present as a bug fix ;-). While I like the feature, I've got very
> > mixed emotions about applying it this late in beta.
>
> > Ok, you caught me :-) Actually, I developed the patch thinking only on
> > the multi-relation TRUNCATE, with the goal of having it for 8.1. Only
> > after I prepared a first patch I noticed the temp-table-FKs problem.
>
> Well, like I said, I have mixed emotions. Anyone else have an opinion
> whether to apply now or hold for 8.1?

My initial reaction is to hold it all for 8.1. The patch is large and
the bug is rare.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

FYI, this was already applied:

2005-01-26 22:17 tgl

* doc/src/sgml/ref/truncate.sgml, src/backend/catalog/heap.c,
src/backend/commands/tablecmds.c, src/backend/nodes/copyfuncs.c,
src/backend/nodes/equalfuncs.c, src/backend/parser/gram.y,
src/backend/tcop/utility.c, src/include/catalog/heap.h,
src/include/commands/tablecmds.h, src/include/nodes/parsenodes.h,
src/test/regress/expected/temp.out,
src/test/regress/expected/truncate.out,
src/test/regress/sql/temp.sql, src/test/regress/sql/truncate.sql:
Generalize TRUNCATE to support truncating multiple tables in one
command. This is useful because we can allow truncation of tables
referenced by foreign keys, so long as the referencing table is
truncated in the same command.

Alvaro Herrera

---------------------------------------------------------------------------

Alvaro Herrera wrote:
> Hackers,
>
> There's a bug with temporary tables signalled ON COMMIT DELETE ROWS,
> when they contain foreign key references. An example:
>
> alvherre=# begin;
> BEGIN
> alvherre=# CREATE TEMP TABLE foo (a int PRIMARY KEY) ON COMMIT DELETE ROWS;
> NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo"
> CREATE TABLE
> alvherre=# CREATE TEMP TABLE bar (a int REFERENCES foo) ON COMMIT DELETE ROWS;
> CREATE TABLE
> alvherre=# COMMIT;
> ERROR: cannot truncate a table referenced in a foreign key constraint
> DETAIL: Table "bar" references "foo" via foreign key constraint "bar_a_fkey".
>
> Say again? Certainly this shouldn't happen, because both tables are
> supposed to lose rows on transaction commit. But this isn't working.
>
> The attached patch fixes this bug. (In all likelyhood, not a lot of
> people uses referential integrity on temp tables, and that's why this
> hasn't been reported. But it's a bug anyway.)
>
>
> Incidentally ("collateral damage"), the patch modifies the TRUNCATE
> command so that it can work on multiple tables. In particular, if
> foreign key references are all internal to the group that's being
> truncated, the command is allowed.
>
> There's one thing that bothers me on this patch: the fact that
> pg_constraint has to be scanned multiple times, and they are all
> seqscans. Not sure what to do about that. Maybe there's a way to do
> better?
>
> Also, observe that when the TRUNCATE operation is aborted because of a
> foreign key, a HINT is emitted as well telling the user to truncate the
> referencing table too. This is IMHO a good hint, but it may be
> misleading when the truncation has taken the ON COMMIT DELETE ROWS path.
> Not sure if it's worth fixing (maybe the hint should suggest to add ON
> COMMIT DELETE ROWS to the referencing table as well?).
>
> Please have a look. The patch is not as intrusive as it looks; there's
> a lot of whitespace change.
>
> --
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> "Ellos andaban todos desnudos como su madre los pari?, y tambi?n las mujeres,
> aunque no vi m?s que una, harto moza, y todos los que yo vi eran todos
> mancebos, que ninguno vi de edad de m?s de XXX a?os" (Crist?bal Col?n)

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)org)

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073