Re: WIP: Faster Expression Processing v4

Hi,

On 2017-03-07 18:46:31 -0500, Peter Eisentraut wrote:
> I looked over
> 0001-Add-expression-dependencies-on-composite-type-whole-.patch.  That
> seems useful independent of the things you have following.
> 
> Even though it is presented more as a preparatory change, it appears to
> have noticeable user-facing impact, which we should analyze.

Indeed.


> For
> example, in the regression test, the command
> 
>     alter table tt14t drop column f3;
> 
> is now rejected, rightly, but the command
> 
>     alter table tt14t drop column f3 cascade;
> 
> ends up dropping the whole view, which might be a bit surprising.  We
> don't support dropping columns from views, so there might be no
> alternative

It seems strictly better than silently breaking the view.


> , but I wonder what else is changing because of this.  I
> think that might deserve a bit more analysis and perhaps some test cases.

There are already some tests. More is probably good - if you have
specific cases in mind...


> --- a/src/backend/catalog/dependency.c
> +++ b/src/backend/catalog/dependency.c
> @@ -206,6 +206,9 @@ static bool object_address_present_add_flags(const
> ObjectAddress *object,
>  static bool stack_address_present_add_flags(const ObjectAddress *object,
>                                                                 int flags,
> 
> ObjectAddressStack *stack);
> +static void add_type_addresses(Oid typeid, bool include_components,
> ObjectAddresses *addrs);
> +static void add_type_component_addresses(Oid typeid, ObjectAddresses
> *addrs);
> +static void add_class_component_addresses(Oid relid, ObjectAddresses
> *addrs);
>  static void DeleteInitPrivs(const ObjectAddress *object);
> 
> For some reason, the function add_object_address() is singular, and
> these new functions are plural  Clearly, they take a plural argument, so
> your version seems more correct, but I wonder if we should keep that
> consistent.

I named them plural, because add_object_address only adds a single new
entry, but add_type_addresses etc potentially add multiple ones.  So I
think plural/singular as used here is actually consistent?


> +                        * e.g. not to the right thing for column-less
> tables.  Note that
> 
> Small typo there.  (to -> do)

Oops.


> @@ -1639,6 +1641,9 @@ find_expr_references_walker(Node *node,
> 
>                 add_object_address(OCLASS_PROC, funcexpr->funcid, 0,
>                                                    context->addrs);
> +               /* dependency on type itself already exists via function */
> +               add_type_component_addresses(funcexpr->funcresulttype,
> context->addrs);
> +
>                 /* fall through to examine arguments */
>         }
>         else if (IsA(node, OpExpr))
> 
> Why shouldn't the function itself also depend on the components of its
> return type?

Because that'd make it impossible to change the return type components -
if the return type is baked into the view that's necessary, but for a
"freestanding function" it's not.  If you e.g. have a function that just
returns a table's rows, it'd certainly be annoying if that'd prevent you
from dropping columns.


> How are argument types handled?

We fall through to

	return expression_tree_walker(node, find_expr_references_walker,
								  (void *) context);

which'll add a dependency if necessary.  If it's e.g. a table column,
function return type or such we'll add a dependency there.


Greetings,

Andres Freund