Hi, On 2019-11-12 10:19:30 -0500, Tom Lane wrote: > I think that the long-term answer, if Andres gets somewhere with his > project to autogenerate code like this, is that we'd rely on annotating > the struct declarations to tell us what to do. In the case at hand, > I could imagine annotations that say "this field contains a function OID" > or "this list contains collation OIDs" and then the find_expr_references > logic could be derived from that. Now, that's not perfect either, because > it's always possible to forget to annotate something. But it'd be a lot > easier, because there'd be tons of nearby examples of doing it right.
Yea, I think that'd be going in the right direction. I've a few annotations for other purposes in my local version of the patch (e.g. to ignore fields for comparison), and adding further ones for purposes like this ought to be easy. I want to attach some annotations to types, rather than fields. I e.g. introduced a Location typedef, annotated as being ignored for equality purposes, instead of annotating each 'int location'. Wonder if we should also do something like that for your hypothetical "function OID" etc. above - seems like it also might give the human reader of code a hint. On 2019-11-11 16:41:41 -0500, Tom Lane wrote: > I happened to notice that find_expr_references_walker has not > been taught anything about TableFunc nodes, which means it will > miss the type and collation OIDs embedded in such a node. > Would it be a good idea to move find_expr_references_walker to > nodeFuncs.c, in hopes of making it more visible to people adding > new node types? Can't hurt, at least. Reducing the number of files that need to be fairly mechanically be touched when adding a node type / node type field strikes me as a good idea. Wonder if there's any way to write an assertion check that verifies we have the necessary dependencies. But the only idea I have - basically record all the syscache lookups while parse analysing an expression, and then check that all the necessary dependencies exist - seems too complicated to be worthwhile. > We could decouple it from the specific use-case > of recordDependencyOnExpr by having it call a callback function > for each identified OID; although maybe there's no point in that, > since I'm not sure there are any other use-cases. I could see it being useful for a few other purposes, e.g. it seems *marginally* possible we could share *some* code with extract_query_dependencies(). But I think I'd only go there if we actually convert something else to it. Greetings, Andres Freund
