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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers