Re: ALTER TYPE 2: skip already-provable no-work rewrites - Mailing list pgsql-hackers

* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Feb 14, 2011 at 01:12:21PM -0500, Stephen Frost wrote:
> > First question is- why do you use #ifdef USE_ASSERT_CHECKING ..?
>
> The other six code sites checking assert_enabled directly do the same.

Wow, I could have sworn that I looked at what others were doing and
didn't see that.  Sorry for the noise.

> > 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. :)

> The way I like to think about it is that we're recursively walking an expression
> tree to determine which of three categories it falls into: always produces the
> same value without error; always produces the same value on success, but may
> throw an error; neither of the above.  We have a whitelist of node types that
> are acceptable, and anything else makes us assume the worst.  (The nodes we
> accept are simple enough that the recursion degenerates to iteration.)

It's that degeneration that definitely hits me as making the whole thing
look a bit 'funny'.  When I first looked at the loop, I was looking for
a tree structure and trying to figure out how it could work with just a
simple while().

> http://archives.postgresql.org/message-id/20101231013534.GA7521@tornado.leadboat.com
>
> Should I restore some of that?  Any other particular text that would have helped?

I definitely think the examples given, enumerating the types of nodes
that matter for this (and why they're the ones we look for), and the
rules that are followed would help a great deal.  Anyone else who comes
across this code may be wondering "do I need to do something here for
this new node type that I just added".

> > It also seems like it'd make more sense to me to
> > be a while() controlled by (IsA(expr, Var) && ((Var *) expr)->varattno
> > == varattno), since that's really the normal "stopping point".
>
> If we can optimize to some extent, that is the stopping point.  If not,
> tab->rewrite is the stopping point.  I picked the no-optimization case as
> "normal" and used that as the loop condition, but one could go either way.

I would think you could still short-circuit the loop when you've
discovered a case where we have to rewrite the table anyway.  Having the
comments updated to reflect what's going on and why stopping on
tab->rewrite, in particular, makes sense is more important though.

> 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, 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.

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?  Or perhaps other
places would benefit from knowing that a given conversion is an actual
no-op rather than copying the value?

Just my 2c, I don't believe the patch could cause problems now that I'm
understanding it better, but it sure does seem excessive to use an
expression tree to figure out when something is a no-op.
Thanks,
    Stephen