On Thu, May 20, 2021 at 5:23 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Amit Langote <amitlangot...@gmail.com> writes: > > IOW, the patch you posted earlier seems like the way to go. > > I really dislike that patch. I think it's doubling down on the messy, > unstructured coding patterns that got us into this situation to begin > with. I'd prefer to expend a little effort on refactoring so that > the ExecCleanupTupleRouting call can be moved to the cleanup function > where it belongs. > > So, I propose the attached, which invents a new struct to carry > the stuff we've discovered to be necessary. This makes the APIs > noticeably cleaner IMHO.
Larger footprint, but definitely cleaner. Thanks. > I did not touch the APIs of the apply_XXX_internal functions, > as it didn't really seem to offer any notational advantage. > We can't simply collapse them to take an "edata" as I did for > apply_handle_tuple_routing, because the ResultRelInfo they're > supposed to operate on could be different from the original one. > I considered a couple of alternatives: > > * Replace their estate arguments with edata, but keep the separate > ResultRelInfo arguments. This might be worth doing in future, if we > add more fields to ApplyExecutionData. Right now it'd save nothing, > and it'd create a risk of confusion about when to use the > ResultRelInfo argument vs. edata->resultRelInfo. > > * Allow apply_handle_tuple_routing to overwrite edata->resultRelInfo > with the partition child's RRI, then simplify the apply_XXX_internal > functions to take just edata instead of separate estate and > resultRelInfo args. I think this would work right now, but it seems > grotty, and it might cause problems in future. > > * Replace the edata->resultRelInfo field with two fields, one for > the original parent and one for the actual/current target. Perhaps > this is worth doing, not sure. > > Thoughts? IMHO, it would be better to keep the lowest-level apply_handle_XXX_internal() out of this, because presumably we're only cleaning up the mess in higher-level callers. Somewhat related, one of the intentions behind a04daa97a43, which removed es_result_relation_info in favor of passing the ResultRelInfo explicitly to the executor's lower-level functions, was to avoid bugs caused by failing to set/reset that global field correctly in higher-level callers. Now worker.c is pretty small compared with the executor, but still it seems like a good idea to follow the same principle here. -- Amit Langote EDB: http://www.enterprisedb.com
