Second thoughts on CheckIndexCompatible() vs. operator families
In 367bc426a1c22b9f6badb06cd41fc438fd034639, I introduced a
CheckIndexCompatible() that approves btree and hash indexes having changed to
a different operator class within the same operator family. To make that
valid, I also tightened the operator family contracts for those access methods
to address casts. However, I've found two retained formal hazards. They're
boring and perhaps unlikely to harm real users, but I'd rather nail them down
just in case.
First, opclasses like array_ops have polymorphic opcintype. Members of such
operator classes could, in general, change behavior arbitrarily in response to
get_fn_expr_argtype(). No core polymorphic operator family member does this.
Nor could they, for no core index access method sets fn_expr. In the absence
of responses to my previous inquiry[1]http://archives.postgresql.org/message-id/20111229211711.GA8085@tornado.leadboat.com on the topic, I'm taking the position
that the lack of fn_expr in these calls is an isolated implementation detail
that could change at any time. Therefore, we should only preserve an index of
polymorphic operator class when the old and new opcintype match exactly. This
patch's test suite addition illustrates one ALTER TABLE ALTER TYPE affected by
this new restriction: a conversion from an array type to a domain over that
array type will now require an index rebuild.
Second, as a thought experiment, consider a database with these three types:
1. int4, the core type.
2. int4neg, stores to disk as the negation of its logical value. Includes a
complete set of operators in the integer_ops btree family, but defines no
casts to other integer_ops-represented types.
3. int4other, stores to disk like int4. No operators. Has a implicit binary
coercion cast to int4 and an explicit binary coercion cast to int4neg.
Suppose a table in this database has a column of type int4other with an index
of default operator class. By virtue of the implicit binary coercion and lack
of other candidates, the index will use int4_ops. Now, change the type of
that column from int4other to int4neg. The operator class changes to
int4neg_ops, still within the integer_ops family, and we do not rebuild the
index. However, the logical sign of each value just flipped, making the index
invalid. Where did CheckIndexCompatible() miss? An operator family only
promises cast-compatibility when there exists an implicit or binary coercion
cast between the types in question. There's no int4->int4neg cast here, not
even a multiple-step pathway. CheckIndexCompatible() assumes that our ability
to perform a no-rewrite ALTER TABLE ALTER TYPE implies the existence of such a
cast. It does imply that for the before-and-after _table column types_, but
it's the before-and-after _opcintype_ that matter here.
I think we could close this hazard by having CheckIndexCompatible() test for
an actual implicit or binary coercion cast between the opcintype of the old
and new operator classes. However, I'm not confident to say that no similar
problem would remain. Therefore, I propose ceasing to optimize intra-family
index operator transitions and simply requiring exact matches of operator
classes and exclusion operators. This does not remove any actual optimization
for changes among core types, and it will be simpler to validate. (I designed
the current rules under the misapprehension that varchar_ops was the default
operator class for varchar columns. However, varchar_ops is a copy of
text_ops apart from the name and being non-default. We ship no operator with
a varchar operand, relying instead on binary coercion to text.)
This patch does not, however, remove the new terms from the operator family
contracts. While core PostgreSQL will no longer depend on them, they may
again prove handy for future optimizations like this. I remain of the opinion
that they're already widely (perhaps even universally) obeyed.
This patch conflicts trivially with my patch "Avoid FK validations for
no-rewrite ALTER TABLE ALTER TYPE" in that they both add test cases to the
same location in alter_table.sql. They're otherwise independent, albeit
reflecting parallel principles.
Thanks,
nm
[1]: http://archives.postgresql.org/message-id/20111229211711.GA8085@tornado.leadboat.com
Attachments:
at-index-tighten-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 712b0b0..1bf1de5 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 23,28 ****
--- 23,29 ----
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
+ #include "catalog/pg_type.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
#include "commands/tablecmds.h"
***************
*** 52,57 ****
--- 53,59 ----
/* non-export function prototypes */
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
+ Oid *typeOidP,
Oid *collationOidP,
Oid *classOidP,
int16 *colOptionP,
***************
*** 87,104 **** static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite will not invalidate
! * indexes. For btree and hash indexes, we assume continued validity when
! * each column of an index would have the same operator family before and
! * after the change. Since we do not document a contract for GIN or GiST
! * operator families, we require an exact operator class match for them and
! * for any other access methods.
! *
! * DefineIndex always verifies that each exclusion operator shares an operator
! * family with its corresponding index operator class. For access methods
! * having no operator family contract, confirm that the old and new indexes
! * use the exact same exclusion operator. For btree and hash, there's nothing
! * more to check.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
--- 89,105 ----
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite do not invalidate
! * indexes. We ackowledge this when all operator classes, collations and
! * exclusion operators match. Though we could further permit intra-opfamily
! * changes for btree and hash indexes, that adds subtle complexity with no
! * concrete benefit for core types.
!
! * When a comparison or exclusion operator has a polymorphic input type, the
! * actual input types must also match. This defends against the possibility
! * that operators could vary behavior in response to get_fn_expr_argtype().
! * At present, this hazard is theoretical: check_exclusion_constraint() and
! * all core index access methods decline to set fn_expr for such calls.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
***************
*** 111,116 **** CheckIndexCompatible(Oid oldId,
--- 112,118 ----
List *exclusionOpNames)
{
bool isconstraint;
+ Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
Oid accessMethodId;
***************
*** 123,132 **** CheckIndexCompatible(Oid oldId,
int numberOfAttributes;
int old_natts;
bool isnull;
- bool family_am;
bool ret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
int i;
Datum d;
--- 125,134 ----
int numberOfAttributes;
int old_natts;
bool isnull;
bool ret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
+ Relation irel;
int i;
Datum d;
***************
*** 168,177 **** CheckIndexCompatible(Oid oldId,
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
--- 170,181 ----
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
+ typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo,
! typeObjectId, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
***************
*** 191,202 **** CheckIndexCompatible(Oid oldId,
return false;
}
! /*
! * If the old and new operator class of any index column differ in
! * operator family or collation, regard the old index as incompatible.
! * For access methods other than btree and hash, a family match has no
! * defined meaning; require an exact operator class match.
! */
old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
Assert(old_natts == numberOfAttributes);
--- 195,201 ----
return false;
}
! /* Any change in operator class or collation breaks compatibility. */
old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
Assert(old_natts == numberOfAttributes);
***************
*** 208,259 **** CheckIndexCompatible(Oid oldId,
Assert(!isnull);
old_indclass = (oidvector *) DatumGetPointer(d);
! family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
!
! for (i = 0; i < old_natts; i++)
! {
! Oid old_class = old_indclass->values[i];
! Oid new_class = classObjectId[i];
!
! if (!(old_indcollation->values[i] == collationObjectId[i]
! && (old_class == new_class
! || (family_am && (get_opclass_family(old_class)
! == get_opclass_family(new_class))))))
! {
! ret = false;
! break;
! }
! }
ReleaseSysCache(tuple);
! /*
! * For btree and hash, exclusion operators need only fall in the same
! * operator family; ComputeIndexAttrs already verified that much. If we
! * get this far, we know that the index operator family has not changed,
! * and we're done. For other access methods, require exact matches for
! * all exclusion operators.
! */
! if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
{
- Relation irel;
Oid *old_operators, *old_procs;
uint16 *old_strats;
- /* Caller probably already holds a stronger lock. */
- irel = index_open(oldId, AccessShareLock);
RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
! for (i = 0; i < old_natts; i++)
! if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
! {
! ret = false;
! break;
! }
! index_close(irel, NoLock);
}
return ret;
}
--- 207,248 ----
Assert(!isnull);
old_indclass = (oidvector *) DatumGetPointer(d);
! ret = (memcmp(old_indclass->values, classObjectId,
! old_natts * sizeof(Oid)) == 0 &&
! memcmp(old_indcollation->values, collationObjectId,
! old_natts * sizeof(Oid)) == 0);
ReleaseSysCache(tuple);
! /* For polymorphic opcintype, column type changes break compatibility. */
! irel = index_open(oldId, AccessShareLock); /* caller probably has a lock */
! for (i = 0; i < old_natts && ret; i++)
! ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i])) ||
! irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
!
! /* Any change in exclusion operator selections breaks compatibility. */
! if (ret && indexInfo->ii_ExclusionOps != NULL)
{
Oid *old_operators, *old_procs;
uint16 *old_strats;
RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+ ret = memcmp(old_operators, indexInfo->ii_ExclusionOps,
+ old_natts * sizeof(Oid)) == 0;
! /* Require an exact input type match for polymorphic operators. */
! for (i = 0; i < old_natts && ret; i++)
! {
! Oid left,
! right;
! op_input_types(indexInfo->ii_ExclusionOps[i], &left, &right);
! ret = (!(IsPolymorphicType(left) || IsPolymorphicType(right)) ||
! irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
! }
}
+ index_close(irel, NoLock);
return ret;
}
***************
*** 315,320 **** DefineIndex(RangeVar *heapRelation,
--- 304,310 ----
bool quiet,
bool concurrent)
{
+ Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
Oid accessMethodId;
***************
*** 550,559 **** DefineIndex(RangeVar *heapRelation,
indexInfo->ii_Concurrent = concurrent;
indexInfo->ii_BrokenHotChain = false;
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
--- 540,551 ----
indexInfo->ii_Concurrent = concurrent;
indexInfo->ii_BrokenHotChain = false;
+ typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo,
! typeObjectId, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
***************
*** 980,985 **** CheckPredicate(Expr *predicate)
--- 972,978 ----
*/
static void
ComputeIndexAttrs(IndexInfo *indexInfo,
+ Oid *typeOidP,
Oid *collationOidP,
Oid *classOidP,
int16 *colOptionP,
***************
*** 1108,1113 **** ComputeIndexAttrs(IndexInfo *indexInfo,
--- 1101,1108 ----
}
}
+ typeOidP[attn] = atttype;
+
/*
* Apply collation override if any
*/
diff --git a/src/test/regress/expected/index 57096f2..5e11d3b 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1656,1661 **** where oid = 'test_storage'::regclass;
--- 1656,1671 ----
t
(1 row)
+ -- SET DATA TYPE without a rewrite
+ SET client_min_messages = debug1;
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array"
+ DEBUG: building index "norewrite_array_pkey" on table "norewrite_array"
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ DEBUG: building index "norewrite_array_pkey" on table "norewrite_array"
+ RESET client_min_messages;
--
-- lock levels
--
diff --git a/src/test/regress/sql/alter_table.sqindex faafb22..b546134 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1190,1195 **** select reltoastrelid <> 0 as has_toast_table
--- 1190,1203 ----
from pg_class
where oid = 'test_storage'::regclass;
+ -- SET DATA TYPE without a rewrite
+ SET client_min_messages = debug1;
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ RESET client_min_messages;
+
--
-- lock levels
--
New version that repairs a defective test case.
Attachments:
at-index-tighten-v2.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 712b0b0..1bf1de5 100644
*** a/src/backend/commands/indexcmds.c
--- b/src/backend/commands/indexcmds.c
***************
*** 23,28 ****
--- 23,29 ----
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
+ #include "catalog/pg_type.h"
#include "commands/dbcommands.h"
#include "commands/defrem.h"
#include "commands/tablecmds.h"
***************
*** 52,57 ****
--- 53,59 ----
/* non-export function prototypes */
static void CheckPredicate(Expr *predicate);
static void ComputeIndexAttrs(IndexInfo *indexInfo,
+ Oid *typeOidP,
Oid *collationOidP,
Oid *classOidP,
int16 *colOptionP,
***************
*** 87,104 **** static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite will not invalidate
! * indexes. For btree and hash indexes, we assume continued validity when
! * each column of an index would have the same operator family before and
! * after the change. Since we do not document a contract for GIN or GiST
! * operator families, we require an exact operator class match for them and
! * for any other access methods.
! *
! * DefineIndex always verifies that each exclusion operator shares an operator
! * family with its corresponding index operator class. For access methods
! * having no operator family contract, confirm that the old and new indexes
! * use the exact same exclusion operator. For btree and hash, there's nothing
! * more to check.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
--- 89,105 ----
* of columns and that if one has an expression column or predicate, both do.
* Errors arising from the attribute list still apply.
*
! * Most column type changes that can skip a table rewrite do not invalidate
! * indexes. We ackowledge this when all operator classes, collations and
! * exclusion operators match. Though we could further permit intra-opfamily
! * changes for btree and hash indexes, that adds subtle complexity with no
! * concrete benefit for core types.
!
! * When a comparison or exclusion operator has a polymorphic input type, the
! * actual input types must also match. This defends against the possibility
! * that operators could vary behavior in response to get_fn_expr_argtype().
! * At present, this hazard is theoretical: check_exclusion_constraint() and
! * all core index access methods decline to set fn_expr for such calls.
*
* We do not yet implement a test to verify compatibility of expression
* columns or predicates, so assume any such index is incompatible.
***************
*** 111,116 **** CheckIndexCompatible(Oid oldId,
--- 112,118 ----
List *exclusionOpNames)
{
bool isconstraint;
+ Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
Oid accessMethodId;
***************
*** 123,132 **** CheckIndexCompatible(Oid oldId,
int numberOfAttributes;
int old_natts;
bool isnull;
- bool family_am;
bool ret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
int i;
Datum d;
--- 125,134 ----
int numberOfAttributes;
int old_natts;
bool isnull;
bool ret = true;
oidvector *old_indclass;
oidvector *old_indcollation;
+ Relation irel;
int i;
Datum d;
***************
*** 168,177 **** CheckIndexCompatible(Oid oldId,
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
--- 170,181 ----
indexInfo->ii_ExclusionOps = NULL;
indexInfo->ii_ExclusionProcs = NULL;
indexInfo->ii_ExclusionStrats = NULL;
+ typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo,
! typeObjectId, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
***************
*** 191,202 **** CheckIndexCompatible(Oid oldId,
return false;
}
! /*
! * If the old and new operator class of any index column differ in
! * operator family or collation, regard the old index as incompatible.
! * For access methods other than btree and hash, a family match has no
! * defined meaning; require an exact operator class match.
! */
old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
Assert(old_natts == numberOfAttributes);
--- 195,201 ----
return false;
}
! /* Any change in operator class or collation breaks compatibility. */
old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts;
Assert(old_natts == numberOfAttributes);
***************
*** 208,259 **** CheckIndexCompatible(Oid oldId,
Assert(!isnull);
old_indclass = (oidvector *) DatumGetPointer(d);
! family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID;
!
! for (i = 0; i < old_natts; i++)
! {
! Oid old_class = old_indclass->values[i];
! Oid new_class = classObjectId[i];
!
! if (!(old_indcollation->values[i] == collationObjectId[i]
! && (old_class == new_class
! || (family_am && (get_opclass_family(old_class)
! == get_opclass_family(new_class))))))
! {
! ret = false;
! break;
! }
! }
ReleaseSysCache(tuple);
! /*
! * For btree and hash, exclusion operators need only fall in the same
! * operator family; ComputeIndexAttrs already verified that much. If we
! * get this far, we know that the index operator family has not changed,
! * and we're done. For other access methods, require exact matches for
! * all exclusion operators.
! */
! if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL)
{
- Relation irel;
Oid *old_operators, *old_procs;
uint16 *old_strats;
- /* Caller probably already holds a stronger lock. */
- irel = index_open(oldId, AccessShareLock);
RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
! for (i = 0; i < old_natts; i++)
! if (old_operators[i] != indexInfo->ii_ExclusionOps[i])
! {
! ret = false;
! break;
! }
! index_close(irel, NoLock);
}
return ret;
}
--- 207,248 ----
Assert(!isnull);
old_indclass = (oidvector *) DatumGetPointer(d);
! ret = (memcmp(old_indclass->values, classObjectId,
! old_natts * sizeof(Oid)) == 0 &&
! memcmp(old_indcollation->values, collationObjectId,
! old_natts * sizeof(Oid)) == 0);
ReleaseSysCache(tuple);
! /* For polymorphic opcintype, column type changes break compatibility. */
! irel = index_open(oldId, AccessShareLock); /* caller probably has a lock */
! for (i = 0; i < old_natts && ret; i++)
! ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i])) ||
! irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
!
! /* Any change in exclusion operator selections breaks compatibility. */
! if (ret && indexInfo->ii_ExclusionOps != NULL)
{
Oid *old_operators, *old_procs;
uint16 *old_strats;
RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats);
+ ret = memcmp(old_operators, indexInfo->ii_ExclusionOps,
+ old_natts * sizeof(Oid)) == 0;
! /* Require an exact input type match for polymorphic operators. */
! for (i = 0; i < old_natts && ret; i++)
! {
! Oid left,
! right;
! op_input_types(indexInfo->ii_ExclusionOps[i], &left, &right);
! ret = (!(IsPolymorphicType(left) || IsPolymorphicType(right)) ||
! irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);
! }
}
+ index_close(irel, NoLock);
return ret;
}
***************
*** 315,320 **** DefineIndex(RangeVar *heapRelation,
--- 304,310 ----
bool quiet,
bool concurrent)
{
+ Oid *typeObjectId;
Oid *collationObjectId;
Oid *classObjectId;
Oid accessMethodId;
***************
*** 550,559 **** DefineIndex(RangeVar *heapRelation,
indexInfo->ii_Concurrent = concurrent;
indexInfo->ii_BrokenHotChain = false;
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
--- 540,551 ----
indexInfo->ii_Concurrent = concurrent;
indexInfo->ii_BrokenHotChain = false;
+ typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid));
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
! ComputeIndexAttrs(indexInfo,
! typeObjectId, collationObjectId, classObjectId,
coloptions, attributeList,
exclusionOpNames, relationId,
accessMethodName, accessMethodId,
***************
*** 980,985 **** CheckPredicate(Expr *predicate)
--- 972,978 ----
*/
static void
ComputeIndexAttrs(IndexInfo *indexInfo,
+ Oid *typeOidP,
Oid *collationOidP,
Oid *classOidP,
int16 *colOptionP,
***************
*** 1108,1113 **** ComputeIndexAttrs(IndexInfo *indexInfo,
--- 1101,1108 ----
}
}
+ typeOidP[attn] = atttype;
+
/*
* Apply collation override if any
*/
diff --git a/src/test/regress/expected/index 57096f2..58762a3 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1656,1661 **** where oid = 'test_storage'::regclass;
--- 1656,1670 ----
t
(1 row)
+ -- SET DATA TYPE without a rewrite
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array"
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ DEBUG: building index "norewrite_array_pkey" on table "norewrite_array"
+ RESET client_min_messages;
--
-- lock levels
--
diff --git a/src/test/regress/sql/alter_table.sqindex faafb22..585cf95 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1190,1195 **** select reltoastrelid <> 0 as has_toast_table
--- 1190,1203 ----
from pg_class
where oid = 'test_storage'::regclass;
+ -- SET DATA TYPE without a rewrite
+ CREATE DOMAIN other_textarr AS text[];
+ CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
+ SET client_min_messages = debug1;
+ ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
+ ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
+ RESET client_min_messages;
+
--
-- lock levels
--
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++)
+ ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i
+ irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.
I find that code way too clever.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jan 25, 2012 at 3:52 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.I find that code way too clever.
The way he wrote it, or the way I proposed to write it?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié ene 25 19:05:44 -0300 2012:
On Wed, Jan 25, 2012 at 3:52 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.I find that code way too clever.
The way he wrote it, or the way I proposed to write it?
The code as committed.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Jan 25, 2012 at 03:32:49PM -0500, Robert Haas wrote:
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
Thanks.
+ for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.
I value the savings in vertical space more than the lost idiomaticness. This
decision is 90+% subjective, so I cannot blame you for concluding otherwise.
I do know the feeling of looking at PostgreSQL source code and wishing the
author had not attempted to conserve every line.
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Robert Haas's message of mié ene 25 17:32:49 -0300 2012:
On Sun, Jan 22, 2012 at 12:23 AM, Noah Misch <noah@leadboat.com> wrote:
New version that repairs a defective test case.
Committed. I don't find this to be particularly good style:
+ for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]);...but I am not sure whether we have any formal policy against it, so
I just committed it as-is for now. I would have surrounded the loop
with an if (ret) block and written the body of the loop as if
(condition) { ret = false; break; }.
I find that code way too clever.
Not only is that code spectacularly unreadable, but has nobody noticed
that this commit broke the buildfarm?
regards, tom lane
On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote:
Not only is that code spectacularly unreadable, but has nobody noticed
that this commit broke the buildfarm?
Thanks for reporting the problem. This arose because the new test case
temporarily sets client_min_messages=DEBUG1. The default buildfarm
configuration sets log_statement=all in its postgresql.conf, so the client
gets those log_statement lines. I would fix this as attached, resetting the
optional logging to defaults during the test cases in question. Not
delightful, but that's what we have to work with.
nm
Attachments:
20120125-buildfarm-v1.patchtext/plain; charset=us-asciiDownload
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1665,1671 **** where oid = 'test_storage'::regclass;
--- 1665,1679 ----
t
(1 row)
+ --
-- SET DATA TYPE without a rewrite
+ --
+ -- Temporarily disable optional logging that "make installcheck" testers
+ -- reasonably might enable.
+ SET log_duration = off;
+ SET log_lock_waits = off;
+ SET log_statement = none;
+ SET log_temp_files = -1;
CREATE DOMAIN other_textarr AS text[];
CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array"
***************
*** 1674,1679 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1682,1691 ----
ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
DEBUG: building index "norewrite_array_pkey" on table "norewrite_array"
RESET client_min_messages;
+ RESET log_duration;
+ RESET log_lock_waits;
+ RESET log_statement;
+ RESET log_temp_files;
--
-- lock levels
--
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1197,1203 **** select reltoastrelid <> 0 as has_toast_table
--- 1197,1213 ----
from pg_class
where oid = 'test_storage'::regclass;
+ --
-- SET DATA TYPE without a rewrite
+ --
+
+ -- Temporarily disable optional logging that "make installcheck" testers
+ -- reasonably might enable.
+ SET log_duration = off;
+ SET log_lock_waits = off;
+ SET log_statement = none;
+ SET log_temp_files = -1;
+
CREATE DOMAIN other_textarr AS text[];
CREATE TABLE norewrite_array(c text[] PRIMARY KEY);
SET client_min_messages = debug1;
***************
*** 1205,1210 **** ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work
--- 1215,1225 ----
ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index
RESET client_min_messages;
+ RESET log_duration;
+ RESET log_lock_waits;
+ RESET log_statement;
+ RESET log_temp_files;
+
--
-- lock levels
--
On Thu, Jan 26, 2012 at 6:55 AM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote:
Not only is that code spectacularly unreadable, but has nobody noticed
that this commit broke the buildfarm?Thanks for reporting the problem. This arose because the new test case
temporarily sets client_min_messages=DEBUG1. The default buildfarm
configuration sets log_statement=all in its postgresql.conf, so the client
gets those log_statement lines. I would fix this as attached, resetting the
optional logging to defaults during the test cases in question. Not
delightful, but that's what we have to work with.
I'm just going to remove the test. This is not very future-proof and
an ugly pattern if it gets copied to other places. We need to work on
a more sensible way for ALTER TABLE to report what it did, but a
solution based on what GUCs the build-farm happens to set doesn't seem
like it's justified for the narrowness of the case we're testing here.
Whether or not we allow this case to work without a rewrite is in
some sense arbitrary. There's no real reason it can't be done; rather,
we're just exercising restraint to minimize the risk of future bugs.
So I don't want to go to great lengths to test it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Jan 26, 2012 at 08:23:34AM -0500, Robert Haas wrote:
On Thu, Jan 26, 2012 at 6:55 AM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote:
Not only is that code spectacularly unreadable, but has nobody noticed
that this commit broke the buildfarm?Thanks for reporting the problem. ?This arose because the new test case
temporarily sets client_min_messages=DEBUG1. ?The default buildfarm
configuration sets log_statement=all in its postgresql.conf, so the client
gets those log_statement lines. ?I would fix this as attached, resetting the
optional logging to defaults during the test cases in question. ?Not
delightful, but that's what we have to work with.I'm just going to remove the test. This is not very future-proof and
It would deserve an update whenever we add a new optional-logging GUC
pertaining to user backends. Other tests require similarly-infrequent
refreshes in response to other changes. Of course, buildfarm members would
not be setting those new GUCs the day they're available. Calling for an
update to the test could reasonably fall to the first buildfarm member owner
who actually decides to use a new GUC in his configuration.
an ugly pattern if it gets copied to other places. We need to work on
I would rather folks introduce ugliness to the test suite than introduce
behaviors having no test coverage.
a more sensible way for ALTER TABLE to report what it did, but a
solution based on what GUCs the build-farm happens to set doesn't seem
like it's justified for the narrowness of the case we're testing here.
It's not based on what GUCs the buildfarm happens to set. I looked up all
GUCs that might create problems such as the one observed here. They were the
four I included in the patch, plus debug_pretty_print, debug_print_parse,
debug_print_plan and debug_print_rewritten. I concluded that the four debug_*
ones were materially less likely to ever get set in postgresql.conf for a
"make installcheck" run, so I left them out for brevity.
The setting changed *by default* for buildfarm clients is log_statement.
Buildfarm member owners can do as they wish, though.
Whether or not we allow this case to work without a rewrite is in
some sense arbitrary. There's no real reason it can't be done; rather,
we're just exercising restraint to minimize the risk of future bugs.
So I don't want to go to great lengths to test it.
I used the same strategy in another ALTER TABLE patch this CF:
http://archives.postgresql.org/message-id/20120126033956.GC15670@tornado.leadboat.com
If we pay its costs for those tests, we then may as well let this test case
ride their coattails.
Noah Misch <noah@leadboat.com> writes:
On Thu, Jan 26, 2012 at 08:23:34AM -0500, Robert Haas wrote:
I'm just going to remove the test. This is not very future-proof and
[ objections ]
FWIW, I concur with Robert's choice here. This test method is ugly and
fragile, and I'm not even thinking about the question of whether an
installation might have GUC settings that affect it. My objection is
that you're assuming that nowhere else, anywhere in the large amount of
code executed by the queries under test, will anyone ever wish to insert
another elog(DEBUG) message.
I used the same strategy in another ALTER TABLE patch this CF:
http://archives.postgresql.org/message-id/20120126033956.GC15670@tornado.leadboat.com
That's going to need to be removed before commit too, then.
regards, tom lane