Re: ALTER TYPE 2: skip already-provable no-work rewrites

On Tue, Jan 25, 2011 at 06:40:08PM -0500, Robert Haas wrote:
> On Mon, Jan 24, 2011 at 7:10 PM, Noah Misch <noah@leadboat.com> wrote:
> > * at1.1-default-composite.patch
> > Remove the error when the user adds a column with a default value to a table
> > whose rowtype is used in a column elsewhere.
> 
> Can we fix this without moving the logic around quite so much?  I'm
> worried that could introduce bugs.
> 
> It strikes me that the root of the problem here is that this test is
> subtly wrong:
> 
>         if (newrel)
>                 find_composite_type_dependencies(oldrel->rd_rel->reltype,
> 
>           RelationGetRelationName(oldrel),
> 
>           NULL);

Correct.

> So it seems
> to me that we could fix this with something like the attached.
> Thoughts?

I'm fine with this patch.  A few notes based on its context in the larger
project:

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c

> @@ -3366,14 +3367,14 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
>  	}
>  
>  	/*
> -	 * If we need to rewrite the table, the operation has to be propagated to
> -	 * tables that use this table's rowtype as a column type.
> +	 * If we change column data types or add/remove OIDs, the operation has to
> +	 * be propagated to tables that use this table's rowtype as a column type.
>  	 *
>  	 * (Eventually this will probably become true for scans as well, but at
>  	 * the moment a composite type does not enforce any constraints, so it's
>  	 * not necessary/appropriate to enforce them just during ALTER.)
>  	 */
> -	if (newrel)
> +	if (tab->new_changetypes || tab->new_changeoids)

The next patch removed new_changeoids, so we would instead be keeping it with
this as the only place referencing it.

>  		find_composite_type_dependencies(oldrel->rd_rel->reltype,
>  										 RelationGetRelationName(oldrel),
>  										 NULL);
> @@ -6347,6 +6348,7 @@ ATPrepAlterColumnType(List **wqueue,
>  		newval->expr = (Expr *) transform;
>  
>  		tab->newvals = lappend(tab->newvals, newval);
> +		tab->new_changetypes = true;

The at2v2 patch would then morph to do something like:

if (worklevel != WORK_NONE)
	tab->new_changetypes = true;

That weakens the name "new_changetypes" a bit.