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

On Mon, Feb 14, 2011 at 04:06:59PM -0500, Stephen Frost wrote:
> * Noah Misch (noah@leadboat.com) 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