On Sun, Feb 13, 2011 at 02:53:07PM -0500, Robert Haas wrote:
> On Sun, Feb 13, 2011 at 6:50 AM, Noah Misch <n...@leadboat.com> wrote:
> >> Yikes.
> >> ?I didn't like that Assert much, but maybe we need it, because this is
> >> scary.
> >
> > Can you elaborate on the fear-inducing aspect? ?I don't mind re-adding the
> > Assert, but it seems that some positive understanding of the assumption's
> > validity is in order.
>
> Well, it's pretty obvious that if somehow we were to go down that code
> path and not get back a value that was identical to the one we had
> before, we'd have a corrupted table.
Certainly. Understand that the first increment of this patch already made a
similar assumption about RelabelType, and CoerceToDomain is no less stable in
this respect. We're exposed to this level of responsibility already. We had
better get the code right, but what else is new?
> It seems that just about any
> refactoring of tablecmds.c might create such a possibility.
Uhhh. Example?
> Assertions are a good idea when the reasons why something is true are
> far-removed (in the code) from the places where they need to be true.
Yes. Anyway, since we both clearly like the assertion, I've re-added it.
> I'm somewhat uncomfortable also with the dependency of this code on
> very subtle semantic differences between if (newrel) and if
> (tab->newvals != NIL). I think this would blow up and die if for any
> reason newrel were non-NULL but tab->newvals were NIL. Actually,
> doesn't that happen if we're adding or dropping an OID column?
Adding or dropping OIDs as the sole operation of the ALTER TABLE does result in
newrel != NULL and tab->newvals == NIL, but the code handles that fine. The
loop just re-forms the tuple from its original values/nulls lists.
> I think it might be cleaner to have tab->verify_constraints rather
> than tab->verify. Then ATRewriteTables() could test if
> (tab->verify_constraints || tab->new_notnull), and you wouldn't need
> the bit that sets at->verify if at->new_notnull is already set.
I wouldn't care for that change. Despite the name, NOT NULL and FOREIGN KEY
constraints wouldn't be represented. Casting to a domain to check the domain
constraints is a stretch of the term, and it will be fully obsolete when we
optimize xml -> text and such.
However, I've moved the setting of tab->verify to the points where we set
tab->new_notnull, rather than doing it later in that one place.
> Correct me if I'm wrong here, but we could handle the
> domains-without-contraints part here with about three additional lines
> of code, right? It's only the domains-with-constraints part that
> requires the rest of this.
Correct.
> I'm half-tempted to put that part off to
> 9.2, in the hopes of getting a more substantial solution that can also
> handle things like text -> xml which we don't have time to re-engineer
> right now.
I see.
Anyway, I've attached an updated version with these changes:
- Restored the transform expression walk to its own function
- Assert re-added
- tab->verify set alongside tab->new_notnull, not later
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..06942c3 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 71,76 ****
--- 71,77 ----
#include "storage/smgr.h"
#include "utils/acl.h"
#include "utils/builtins.h"
+ #include "utils/datum.h"
#include "utils/fmgroids.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
***************
*** 142,147 **** typedef struct AlteredTableInfo
--- 143,149 ----
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);
--- 338,345 ----
AlteredTableInfo *tab, Relation rel,
bool recurse, bool recursing,
AlterTableCmd *cmd, LOCKMODE
lockmode);
! static void ATColumnChangeSetWorkLevel(AlteredTableInfo *tab,
! 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);
***************
*** 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);
/*
--- 3316,3322 ----
* 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;
--- 3390,3395 ----
***************
*** 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;
--- 3448,3453 ----
***************
*** 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;
--- 3482,3489 ----
***************
*** 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)
--- 3547,3560 ----
while ((tuple = heap_getnext(scan, ForwardScanDirection)) !=
NULL)
{
! Oid tupOid = InvalidOid;
+ if (newrel
+ #ifdef USE_ASSERT_CHECKING
+ || assert_enabled
+ #endif
+ )
+ {
/* 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;
--- 3563,3577 ----
/* 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;
***************
*** 3577,3584 **** ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap,
LOCKMODE lockmode)
--- 3584,3618 ----
econtext,
&isnull[ex->attnum - 1],
NULL);
+
+ #ifdef USE_ASSERT_CHECKING
+ if (assert_enabled)
+ {
+ Datum oldval =
values[ex->attnum - 1];
+ bool oldisnull =
isnull[ex->attnum - 1];
+ Form_pg_attribute f =
newTupDesc->attrs[ex->attnum - 1];
+
+ if (f->attbyval && f->attlen ==
-1)
+ oldval =
PointerGetDatum(PG_DETOAST_DATUM(oldval));
+
+ /*
+ * We don't detect the gross
error of !newrel when the
+ * typlen actually changed.
attbyval could differ in
+ * theory, but we assume it
does not.
+ */
+ Assert(newrel ||
+ (isnull[ex->attnum -
1] == oldisnull
+ && (oldisnull ||
+
datumIsEqual(oldval,
+
values[ex->attnum - 1],
+
f->attbyval, f->attlen))));
+ }
+ #endif
}
+ }
+ if (newrel)
+ {
/*
* Form the new tuple. Note that we don't
explicitly pfree it,
* since the per-tuple memory context will be
reset shortly.
***************
*** 4357,4362 **** ATExecAddColumn(AlteredTableInfo *tab, Relation rel,
--- 4391,4397 ----
* anything.)
*/
tab->new_notnull |= colDef->is_not_null;
+ tab->verify |= tab->new_notnull;
}
/*
***************
*** 4553,4558 **** ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
--- 4588,4594 ----
/* Tell Phase 3 it needs to test the constraint */
tab->new_notnull = true;
+ tab->verify = true;
}
heap_close(attr_rel, RowExclusiveLock);
***************
*** 5270,5275 **** ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab,
Relation rel,
--- 5306,5312 ----
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,
--- 5655,5664 ----
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)
{
--- 6622,6628 ----
newval->expr = (Expr *) transform;
tab->newvals = lappend(tab->newvals, newval);
! ATColumnChangeSetWorkLevel(tab, transform, attnum);
}
else if (tab->relkind == RELKIND_FOREIGN_TABLE)
{
***************
*** 6623,6647 **** ATPrepAlterColumnType(List **wqueue,
}
/*
! * 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;
}
}
--- 6663,6697 ----
}
/*
! * 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.
*/
! static void
! ATColumnChangeSetWorkLevel(AlteredTableInfo *tab,
! Node *expr, AttrNumber
varattno)
{
Assert(expr != NULL);
! while (!tab->rewrite)
{
/* only one varno, so no need to check that */
if (IsA(expr, Var) && ((Var *) expr)->varattno == varattno)
! break;
else if (IsA(expr, RelabelType))
expr = (Node *) ((RelabelType *) expr)->arg;
+ else if (IsA(expr, CoerceToDomain))
+ {
+ CoerceToDomain *d = (CoerceToDomain *) expr;
+
+ if (GetDomainConstraints(d->resulttype) != NIL)
+ tab->verify = true;
+ expr = (Node *) d->arg;
+ }
else
! tab->rewrite = true;
}
}
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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
