readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.
As previously discussed, parallel query will use outfuncs/readfuncs to
copy the Plan to be executed by a parallel worker to that worker.
That plan may contain expressions, and the round-trip through
outfuncs/readfuncs will lose their opfuncids. In this case, that's
pretty annoying, if not outright wrong. It's annoying because it
means that, after the worker reads the plan tree, it's got to iterate
over the whole thing and repeat the lookups of all the opfuncid
fields. This turns out not to be straightforward to code, because we
don't have a generic plan tree walker, and even if we did, you still
need knowledge of which plan nodes have expressions inside which
fields, and we don't have a generic facility for that either: it's
there inside e.g. set_plan_refs, but not in a form other code can use.
Moreover, if we ever did have an ALTER OPERATOR command that could
change the pg_proc mapping, this would go from annoying to outright
broken, because it would be real bad if the worker and the leader came
to different conclusions about what opfuncid to use. Maybe we could
add heavyweight locking to prevent that, but that would be expensive
and we surely don't have it today.
So I think we should abandon the approach Amit took, namely trying to
reset all of the opfuncids. Instead, I think we should provide a
method of not throwing them out in the first place. The attached
patch does by adding an "int flags" field to the relevant read
routines. stringToNode() becomes a macro which passes
STRINGTONODE_INVALIDATE_OPFUNCID to stringToNodeExtended(), which is
one of the functions that now takes an additional "int flags"
argument. If a caller doesn't wish to ignore opfuncid, they can call
stringToNodeExtended directly. This way, existing stringToNode()
callers see no behavior change, but the parallel query code can easily
get the behavior that it wants.
Thoughts? Better ideas? Objections?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
