On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
> * Noah Misch ([email protected]) wrote:
> > On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
> > > In ATColumnChangeSetWorkLevel(), I'm really not a huge fan of using a
> > > passed-in argument to move through a list with.. I'd suggest using a
> > > local variable that is set from what's passed in. I do see that's
> > > inheirited, but still, you've pretty much redefined that entire
> > > function anyway..
> >
> > Could do that. However, the function would reference the original argument
> > just
> > once, to make that copy. I'm not seeing a win.
>
> Perhaps I'm just deficient in this area, but I think of arguments,
> unless specifically intended otherwise, to be 'read-only' and based on
> that assumption, seeing it come up as an lvalue hits me as wrong.
>
> I'm happy enough to let someone else decide if they agree with me or you
> though. :)
Same here. If there's a general project tendency either way, I'll comply. I
haven't noticed one, but I haven't been looking.
I've attached a new version of the patch that attempts to flesh out the comments
based on your feedback. Does it improve things?
> > The validity of this optimization does not
> > rely on any user-settable property of a data type, but it does lean heavily
> > on
> > the execution semantics of specific nodes.
>
> After thinking through this and diving into coerce_to_target_type() a
> bit, I'm finally coming to understand how this is working. The gist of
> it, if I follow correctly, is that if the planner doesn't think we have
> to do anything but copy this value to make it the new data type, then
> we're good to go. That makes sense, when you think about it, but boy
> does it go the long way around to get there.
Essentially. The planner is not yet involved. We're looking at an expression
tree after parse analysis, before planning.
> Essentially, coerce_to_target_type() is returning an expression tree and
> ATColumnChangeSetWorkLevel() is checking to see if that expression tree
> is "copy the value". Maybe this is a bit much, but if
> coerce_to_target_type() knows the expression given to it is a
> straight-up copy, perhaps it could pass that information along in an
> easier to use format than an expression tree? That would obviate the
> need for ATColumnChangeSetWorkLevel() entirely, if I understand
> correctly.
PostgreSQL has a strong tradition of passing around expression trees and walking
them to (re-)discover facts. See the various clauses.h functions. Also, when
you have a USING clause, coerce_to_target_type() doesn't know the whole picture.
We could teach it to discover the whole picture, but that would amount to a very
similar tree walk in a different place.
> Of course, coerce_to_target_type() is used by lots of other places,
> almost all of which probably have to have an expression tree to stuff
> into a plan, so maybe a simpler function could be defined which operates
> at the level that ATColumnChangeSetWorkLevel() needs?
Offhand, I can't think of any concrete implementation along those lines that
would simplify the overall task. Did you have something more specific in mind?
> Or perhaps other
> places would benefit from knowing that a given conversion is an actual
> no-op rather than copying the value?
RelabelType itself does not cause a copy; the existing datum passes through. In
the case of ALTER TABLE, we do eventually copy the datum via heap_form_tuple.
There may be other places that would benefit from similar analysis. For that
reason, I originally had ATColumnChangeSetWorkLevel() in parse_coerce.c with the
name GetCoerceExemptions(). I'm not aware of any specific applications, though.
For now it seemed like premature abstraction.
Thanks again,
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..ab0bcda 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,6722 ----
}
/*
! * Decide how much phase-3 per-tuple work this column type change entails,
based
! * on its transformation expression tree. There are three possibilities:
! *
! * 1. Rewrite; always sufficient. Example: int2 -> int4.
! * 2. Verification scan. Example: text -> constrained domain over text.
! * 3. No per-tuple work. Example: varchar(N) -> text.
! *
! * To certify #2 or #3 as sufficient, the transformation expression tree must
! * consist solely of nodes from this list:
! *
! * A. Var with the varattno of the column under alteration. Allows #2 or
#3.
! * B. RelabelType. Allows #2 or #3.
! * C. CoerceToDomain. Allows #2; allows #3 when GetDomainConstraints() ==
NIL.
! *
! * (A) primordially conveys the current column data. (B) and (C) are
acceptable
! * because they always pass a datum through unchanged; any other node having
! * that property could potentially join this list.
*/
! static void
! ATColumnChangeSetWorkLevel(AlteredTableInfo *tab,
! Node *expr, AttrNumber
varattno)
{
Assert(expr != NULL);
! /*
! * Walking an expression tree is normally a recursive operation. Since
all
! * node types on our current whitelist have no more than one descendent
node
! * requiring assessment, the recursion degenerates to iteration.
! *
! * When tab->rewrite becomes true, no optimizations are possible, and
we're
! * done. It may be set already from analysis of an earlier subcommand
in
! * the same ALTER TABLE operation, and that's fine.
! */
! while (!tab->rewrite)
{
! /*
! * Non-matching varattno arises when an explicit USING clause
references
! * a different column. Only one varno, so no need to check
that. When
! * we reach this node, we're done, and some optimizations do
apply.
! */
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
