On Fri, Feb 11, 2011 at 02:49:27PM -0500, Robert Haas wrote:
> You might want to consider a second boolean in lieu of a three way
> enum. I'm not sure if that's cleaner but if it lets you write:
>
> if (blah)
> at->verify = true;
>
> instead of:
>
> if (blah)
> at->worklevel = Min(at->worklevel, WORK_VERIFY);
>
> ...then I think that might be cleaner.
Good point; the Max() calls did not make much sense all by themselves. The
point was to make sure nothing decreased the worklevel. Wrapping them in a
macro, say, ATRequireWork, probably would have helped.
That said, I've tried both constructions, and I marginally prefer the end result
with AlteredTableInfo.verify. I've inlined ATColumnChangeRequiresRewrite into
ATPrepAlterColumnType; it would need to either pass back two bools or take an
AlteredTableInfo arg to mutate, so this seemed cleaner. I've omitted the
assertion that my previous version added to ATRewriteTable; it was helpful for
other scan-only type changes, but it's excessive for domains alone. Otherwise,
the differences are cosmetic.
The large block in ATRewriteTable is now superfluous. For easier review, I
haven't removed it.
I missed a typo in the last patch: "T if we a rewrite is forced". Not changed
in this patch as I assume you'll want to commit it separately.
nm
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 452ced6..466be25 100644
diff --git a/src/backend/commands/index 1db42d0..2e10661 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 142,147 **** typedef struct AlteredTableInfo
--- 142,148 ----
List *newvals; /* List of NewColumnValue */
bool new_notnull; /* T if we added new NOT NULL
constraints */
bool rewrite; /* T if we a rewrite is forced
*/
+ bool verify; /* T if we shall verify
constraints */
Oid newTableSpace; /* new tablespace; 0 means no
change */
/* Objects to rebuild after completing ALTER TYPE operations */
List *changedConstraintOids; /* OIDs of constraints to
rebuild */
***************
*** 336,342 **** static void ATPrepAlterColumnType(List **wqueue,
AlteredTableInfo *tab, Relation rel,
bool recurse, bool recursing,
AlterTableCmd *cmd, LOCKMODE
lockmode);
- static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno);
static void ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
const char *colName, TypeName
*typeName, LOCKMODE lockmode);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
LOCKMODE lockmode);
--- 337,342 ----
***************
*** 3242,3247 **** ATRewriteTables(List **wqueue, LOCKMODE lockmode)
--- 3242,3251 ----
heap_close(rel, NoLock);
}
+ /* New NOT NULL constraints always require a scan. */
+ if (tab->new_notnull)
+ tab->verify = true;
+
/*
* We only need to rewrite the table if at least one column
needs to
* be recomputed, or we are adding/removing the OID column.
***************
*** 3313,3319 **** ATRewriteTables(List **wqueue, LOCKMODE lockmode)
* Test the current data within the table against new
constraints
* generated by ALTER TABLE commands, but don't rebuild
data.
*/
! if (tab->constraints != NIL || tab->new_notnull)
ATRewriteTable(tab, InvalidOid, lockmode);
/*
--- 3317,3323 ----
* Test the current data within the table against new
constraints
* generated by ALTER TABLE commands, but don't rebuild
data.
*/
! if (tab->verify)
ATRewriteTable(tab, InvalidOid, lockmode);
/*
***************
*** 3387,3393 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
Relation newrel;
TupleDesc oldTupDesc;
TupleDesc newTupDesc;
- bool needscan = false;
List *notnull_attrs;
int i;
ListCell *l;
--- 3391,3396 ----
***************
*** 3446,3452 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
switch (con->contype)
{
case CONSTR_CHECK:
- needscan = true;
con->qualstate = (List *)
ExecPrepareExpr((Expr *) con->qual,
estate);
break;
--- 3449,3454 ----
***************
*** 3481,3491 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
!newTupDesc->attrs[i]->attisdropped)
notnull_attrs = lappend_int(notnull_attrs, i);
}
- if (notnull_attrs)
- needscan = true;
}
- if (newrel || needscan)
{
ExprContext *econtext;
Datum *values;
--- 3483,3490 ----
***************
*** 3549,3558 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
while ((tuple = heap_getnext(scan, ForwardScanDirection)) !=
NULL)
{
! if (tab->rewrite)
! {
! Oid tupOid = InvalidOid;
/* Extract data from old tuple */
heap_deform_tuple(tuple, oldTupDesc, values,
isnull);
if (oldTupDesc->tdhasoid)
--- 3548,3557 ----
while ((tuple = heap_getnext(scan, ForwardScanDirection)) !=
NULL)
{
! Oid tupOid = InvalidOid;
+ if (newrel)
+ {
/* Extract data from old tuple */
heap_deform_tuple(tuple, oldTupDesc, values,
isnull);
if (oldTupDesc->tdhasoid)
***************
*** 3561,3570 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
/* Set dropped attributes to null in new tuple
*/
foreach(lc, dropped_attrs)
isnull[lfirst_int(lc)] = true;
/*
* Process supplied expressions to replace
selected columns.
! * Expression inputs come from the old tuple.
*/
ExecStoreTuple(tuple, oldslot, InvalidBuffer,
false);
econtext->ecxt_scantuple = oldslot;
--- 3560,3574 ----
/* Set dropped attributes to null in new tuple
*/
foreach(lc, dropped_attrs)
isnull[lfirst_int(lc)] = true;
+ }
+ if (tab->newvals != NIL)
+ {
/*
* Process supplied expressions to replace
selected columns.
! * Expression inputs come from the old tuple.
If !newrel, we've
! * proven that no expression will change a
tuple value, but they
! * may throw errors.
*/
ExecStoreTuple(tuple, oldslot, InvalidBuffer,
false);
econtext->ecxt_scantuple = oldslot;
***************
*** 3578,3584 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
--- 3582,3591 ----
&isnull[ex->attnum - 1],
NULL);
}
+ }
+ if (newrel)
+ {
/*
* Form the new tuple. Note that we don't
explicitly pfree it,
* since the per-tuple memory context will be
reset shortly.
***************
*** 5270,5275 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab,
Relation rel,
--- 5277,5283 ----
newcon->qual = (Node *) make_ands_implicit((Expr *) ccon->expr);
tab->constraints = lappend(tab->constraints, newcon);
+ tab->verify = true;
/* Save the actually assigned name if it was defaulted */
if (constr->conname == NULL)
***************
*** 5618,5623 **** ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation
rel,
--- 5626,5635 ----
newcon->qual = (Node *) fkconstraint;
tab->constraints = lappend(tab->constraints, newcon);
+ /*
+ * No need to set tab->verify; foreign key validation is a
distinct
+ * aspect of Phase 3.
+ */
}
/*
***************
*** 6581,6588 **** ATPrepAlterColumnType(List **wqueue,
newval->expr = (Expr *) transform;
tab->newvals = lappend(tab->newvals, newval);
! if (ATColumnChangeRequiresRewrite(transform, attnum))
! tab->rewrite = true;
}
else if (tab->relkind == RELKIND_FOREIGN_TABLE)
{
--- 6593,6625 ----
newval->expr = (Expr *) transform;
tab->newvals = lappend(tab->newvals, newval);
!
! /*
! * Decide how much phase-3 per-tuple work this change entails.
None
! * needed when the present type changes to itself, to a
constraint-free
! * domain thereon, or to a type to which it is binary
coercible. Any
! * constrained domain requires a verification scan. Anything
else
! * requires a rewrite. Beware of a USING clause trying to
substitute
! * some other value.
! */
! while (!tab->rewrite)
! {
! /* only one varno, so no need to check that */
! if (IsA(transform, Var) && ((Var *)
transform)->varattno == attnum)
! break;
! else if (IsA(transform, RelabelType))
! transform = (Node *) ((RelabelType *)
transform)->arg;
! else if (IsA(transform, CoerceToDomain))
! {
! CoerceToDomain *d = (CoerceToDomain *)
transform;
!
! if (GetDomainConstraints(d->resulttype) != NIL)
! tab->verify = true;
! transform = (Node *) d->arg;
! }
! else
! tab->rewrite = true;
! }
}
else if (tab->relkind == RELKIND_FOREIGN_TABLE)
{
***************
*** 6622,6650 **** ATPrepAlterColumnType(List **wqueue,
ATTypedTableRecursion(wqueue, rel, cmd, lockmode);
}
- /*
- * When the data type of a column is changed, a rewrite might not be require
- * if the data type is being changed to its current type, or more
interestingly
- * to a type to which it is binary coercible. But we must check carefully
that
- * the USING clause isn't trying to insert some other value.
- */
- static bool
- ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
- {
- Assert(expr != NULL);
-
- for (;;)
- {
- /* only one varno, so no need to check that */
- if (IsA(expr, Var) && ((Var *) expr)->varattno == varattno)
- return false;
- else if (IsA(expr, RelabelType))
- expr = (Node *) ((RelabelType *) expr)->arg;
- else
- return true;
- }
- }
-
static void
ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
const char *colName, TypeName
*typeName, LOCKMODE lockmode)
--- 6659,6664 ----
diff --git a/src/test/regress/GNUmakefiindex 4c8af0f..eeb9487 100644
diff --git a/src/test/regress/expecnew file mode 100644
index 0000000..248fb39
diff --git a/src/test/regress/parallel_schedule b/srindex 3b99e86..0420922
100644
diff --git a/src/test/regress/serial_scheindex b348f0e..9954cf7 100644
diff --git a/src/test/regress/sql/big_anew file mode 100644
index 0000000..8302d87
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
