New VACUUM FULL crashes on temp relations

Started by Simon Riggsalmost 16 years ago6 messages
#1Simon Riggs
simon@2ndQuadrant.com
1 attachment(s)

TRAP: FailedAssertion("!(typeNamespace == typ->typnamespace)", File:
"pg_type.c", Line: 658)

Test case attached, repeated, consistent failure on CVS HEAD.

Crash occurs with either CLUSTER or VACUUM FULL.

Passed on without further investigation.

--
Simon Riggs www.2ndQuadrant.com

Attachments:

vftest.sqltext/x-sql; charset=UTF-8; name=vftest.sqlDownload
#2Takahiro Itagaki
itagaki.takahiro@oss.ntt.co.jp
In reply to: Simon Riggs (#1)
Re: New VACUUM FULL crashes on temp relations

Simon Riggs <simon@2ndQuadrant.com> wrote:

TRAP: FailedAssertion("!(typeNamespace == typ->typnamespace)", File:
"pg_type.c", Line: 658)

Test case attached, repeated, consistent failure on CVS HEAD.

I see the same assertion failure on 8.4.2, too.
I'll investigating it...

-- minimum reproducible pattern
drop table if exists footemp;
create temp table footemp (col1 serial, col2 text);
create index footemp_col1_idx on footemp (col1);
cluster footemp using footemp_col1_idx;

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

#3Takahiro Itagaki
itagaki.takahiro@oss.ntt.co.jp
In reply to: Takahiro Itagaki (#2)
1 attachment(s)
Re: New VACUUM FULL crashes on temp relations

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> wrote:

TRAP: FailedAssertion("!(typeNamespace == typ->typnamespace)", File:
"pg_type.c", Line: 658)

It comes from swapping toast relations:
DEBUG: typeNamespace mismatch: 99 (pg_toast) vs. 16386 (pg_toast_temp_2)

AFAICS, the assertion is broken, but the code is correct. We just need to
adjust the expression in the assertion.
Patch attached, including new regression tests for clustering a temp table.

I see the same assertion failure on 8.4.2, too.

I'll go for backpatcting if the attached fix is correct. Comments welcome.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Attachments:

cluster_assert_20100202.patchapplication/octet-stream; name=cluster_assert_20100202.patchDownload
diff -cprN head/src/backend/catalog/pg_type.c work/src/backend/catalog/pg_type.c
*** head/src/backend/catalog/pg_type.c	2010-01-04 09:10:26.638773000 +0900
--- work/src/backend/catalog/pg_type.c	2010-02-02 13:26:19.509652431 +0900
***************
*** 18,23 ****
--- 18,24 ----
  #include "access/xact.h"
  #include "catalog/dependency.h"
  #include "catalog/indexing.h"
+ #include "catalog/namespace.h"
  #include "catalog/pg_namespace.h"
  #include "catalog/pg_proc.h"
  #include "catalog/pg_type.h"
*************** RenameTypeInternal(Oid typeOid, const ch
*** 654,661 ****
  		elog(ERROR, "cache lookup failed for type %u", typeOid);
  	typ = (Form_pg_type) GETSTRUCT(tuple);
  
! 	/* We are not supposed to be changing schemas here */
! 	Assert(typeNamespace == typ->typnamespace);
  
  	arrayOid = typ->typarray;
  
--- 655,664 ----
  		elog(ERROR, "cache lookup failed for type %u", typeOid);
  	typ = (Form_pg_type) GETSTRUCT(tuple);
  
! 	/* We are not supposed to be changing schemas here except toast types */
! 	Assert(typeNamespace == typ->typnamespace ||
! 		   (typeNamespace == PG_TOAST_NAMESPACE &&
! 			isTempToastNamespace(typ->typnamespace)));
  
  	arrayOid = typ->typarray;
  
diff -cprN head/src/test/regress/expected/cluster.out work/src/test/regress/expected/cluster.out
*** head/src/test/regress/expected/cluster.out	2007-09-03 11:41:01.356378000 +0900
--- work/src/test/regress/expected/cluster.out	2010-02-02 13:48:30.442646909 +0900
*************** SELECT * FROM clustertest;
*** 434,439 ****
--- 434,443 ----
   100
  (5 rows)
  
+ -- Test temp table
+ CREATE TEMP TABLE temptbl (col1 integer, col2 text);
+ CREATE INDEX temptbl_col1_idx on temptbl (col1);
+ CLUSTER temptbl USING temptbl_col1_idx;
  -- clean up
  \c -
  DROP TABLE clustertest;
diff -cprN head/src/test/regress/sql/cluster.sql work/src/test/regress/sql/cluster.sql
*** head/src/test/regress/sql/cluster.sql	2007-09-03 11:41:01.356378000 +0900
--- work/src/test/regress/sql/cluster.sql	2010-02-02 13:48:36.679647971 +0900
*************** COMMIT;
*** 187,192 ****
--- 187,197 ----
  
  SELECT * FROM clustertest;
  
+ -- Test temp table
+ CREATE TEMP TABLE temptbl (col1 integer, col2 text);
+ CREATE INDEX temptbl_col1_idx on temptbl (col1);
+ CLUSTER temptbl USING temptbl_col1_idx;
+ 
  -- clean up
  \c -
  DROP TABLE clustertest;
#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Takahiro Itagaki (#3)
Re: New VACUUM FULL crashes on temp relations

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:

AFAICS, the assertion is broken, but the code is correct. We just need to
adjust the expression in the assertion.

I think this is 100% wrong. Toast tables shouldn't be changing
namespace either; which means you broke something somewhere else.

regards, tom lane

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#4)
Re: New VACUUM FULL crashes on temp relations

I wrote:

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> writes:

AFAICS, the assertion is broken, but the code is correct. We just need to
adjust the expression in the assertion.

I think this is 100% wrong. Toast tables shouldn't be changing
namespace either; which means you broke something somewhere else.

After tracing through it, the problem is that rebuild_relation() assumes
toast tables are always in PG_TOAST_NAMESPACE; which has not been true
since 8.3. CLUSTER has been renaming temp toast tables into the wrong
namespace right along. Without the assert to call attention to it, who
knows how long it would've taken to notice :-(

Will fix.

regards, tom lane

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#5)
Re: New VACUUM FULL crashes on temp relations

I wrote:

After tracing through it, the problem is that rebuild_relation() assumes
toast tables are always in PG_TOAST_NAMESPACE; which has not been true
since 8.3. CLUSTER has been renaming temp toast tables into the wrong
namespace right along. Without the assert to call attention to it, who
knows how long it would've taken to notice :-(

No, on closer inspection the bug was introduced here:
http://archives.postgresql.org/pgsql-committers/2008-10/msg00118.php
so 8.3 was OK. In a non-asserting build the only consequence would have
been that checks for conflicting names were done in the wrong namespace.
Given the improbability of a conflict, we could have gone a very long
time before noticing the problem. But it was still wrong.

regards, tom lane