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(at)leadboat(dot)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.