From: Neil Conway <neilc@samurai.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: bug in ALTER TABLE / TYPE
Date: Wed, 29 Jun 2005 18:08:13 +1000
Message-ID: <42C256ED.3000504@samurai.com> (raw)
A coworker of mine reported a subtle issue in ATExecAlterColumnType() in
tablecmds.c. Suppose we have the following DDL:
CREATE TABLE pktable (a int primary key, b int);
CREATE TABLE fktable (fk int references pktable, c int);
ALTER TABLE pktable ALTER COLUMN a TYPE bigint;
Circa line 4891 in current sources, we begin a system table scan to look
for pg_depend rows that depend on the column we're modifying. In this
case, there are two dependencies on the column: the pg_constraint row
for pktable's PK constraint, and the pg_constraint row for fktable's FK
constraint. The bug is that we require the systable scan to return the
FK constraint before the PK constraint -- if we attempt to remove the PK
constraint first, it will fail because the FK constraint still exists
and depends on the PK constraint.
This bug is difficult to reproduce with a normal postmaster and vanilla
sources, as the systable scan is usually implemented via a B+-tree scan
on (refclassid, refobjid, refobjsubid). For this particular test case
these three fields have equal values for the PK and FK constraint
pg_depend rows, so the order in which the two constraints are returned
is undefined. It just so happens that the Postgres b+-tree
implementation *usually* returns the FK constraint first (per comments
in nbtinsert.c, we usually place an equal key value at the end of a
chain of equal keys, but stop looking for the end of the chain with a
probability of 1%). And of course there is no ordering constraint if the
systable scan is implemented via a heap scan (which would happen if,
say, we're ignoring indexes on system catalogs in a standalone backend).
To reproduce, any of:
(1) Run the above SQL in a standalone backend started with the "-P" flag
(2) Change "true" to "false" in the third argument to
systable_beginscan() at tablecmds.c:4891, which means a heap scan will
be used by a normal backend.
(3) I've attached a dirty kludge of a patch that shuffles the results of
the systable scan with probability 50%. Applying the patch should repro
the bug with a normal backend (approx. 1 in 2 times, naturally).
I'm not too familiar with the pg_depend code, so I'm not sure the right
fix. Comments?
-Neil
Attachments:alter_table_bug_repro-1.patch (text/x-patch, 2.3K) download
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.162
diff -c -r1.162 tablecmds.c
*** src/backend/commands/tablecmds.c 28 Jun 2005 05:08:54 -0000 1.162
--- src/backend/commands/tablecmds.c 29 Jun 2005 07:15:07 -0000
***************
*** 4801,4806 ****
--- 4801,4809 ----
ScanKeyData key[3];
SysScanDesc scan;
HeapTuple depTup;
+ List *tmp_list = NIL;+ List *tmp_list2 = NIL;+ ListCell *lc;
attrelation = heap_open(AttributeRelationId, RowExclusiveLock);
***************
*** 4893,4901 ****
while (HeapTupleIsValid(depTup = systable_getnext(scan)))
{
! Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(depTup);
ObjectAddress foundObject;
/* We don't expect any PIN dependencies on columns */
if (foundDep->deptype == DEPENDENCY_PIN)
elog(ERROR, "cannot alter type of a pinned column");
--- 4896,4941 ----
while (HeapTupleIsValid(depTup = systable_getnext(scan)))
{
! tmp_list = lappend(tmp_list, heap_copytuple(depTup));
! }
!
! foreach (lc, tmp_list)
! {
! if (lnext(lc) != NULL)
! {
! Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT((HeapTuple) lfirst(lc));
! Form_pg_depend foundDepNext = (Form_pg_depend) GETSTRUCT((HeapTuple) lfirst(lnext(lc)));
!
! if (foundDep->refclassid == foundDepNext->refclassid &&
! foundDep->refobjid == foundDepNext->refobjid &&
! foundDep->refobjsubid == foundDepNext->refobjsubid)
! {
! if (random() > (MAX_RANDOM_VALUE / 2))
! {
! tmp_list2 = lappend(tmp_list2, lfirst(lnext(lc)));
! tmp_list2 = lappend(tmp_list2, lfirst(lc));
! lc = lnext(lc);
! elog(NOTICE, "choosing to shuffle");
! continue;
! }
! else
! elog(NOTICE, "choosing not to shuffle");
! }
! }
!
! tmp_list2 = lappend(tmp_list2, lfirst(lc));
! }
!
! Assert(list_length(tmp_list) == list_length(tmp_list2));
!
! foreach (lc, tmp_list2)
! {
! Form_pg_depend foundDep;
ObjectAddress foundObject;
+ depTup = lfirst(lc);+ foundDep = (Form_pg_depend) GETSTRUCT(depTup);+
/* We don't expect any PIN dependencies on columns */
if (foundDep->deptype == DEPENDENCY_PIN)
elog(ERROR, "cannot alter type of a pinned column");
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: neilc@samurai.com
Subject: Re: bug in ALTER TABLE / TYPE
In-Reply-To: <42C256ED.3000504@samurai.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox