Message-ID: <42C256ED.3000504@samurai.com>
Date: Wed, 29 Jun 2005 18:08:13 +1000
From: Neil Conway <neilc@samurai.com>
User-Agent: Debian Thunderbird 1.0.2 (X11/20050602)
MIME-Version: 1.0
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: bug in ALTER TABLE / TYPE
Content-Type: multipart/mixed; boundary="------------030500070306080402010509"

This is a multi-part message in MIME format.
--------------030500070306080402010509
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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


--------------030500070306080402010509
Content-Type: text/x-patch;
 name="alter_table_bug_repro-1.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="alter_table_bug_repro-1.patch"

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");

--------------030500070306080402010509--