On Wed, May 19, 2021 at 04:23:55PM -0400, Tom Lane wrote: > 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.
Okay. > 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. Not sure about this one. It may be better to wait until this gets more expanded, if it gets expanded. > * 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. Agreed that it does not seem like a good idea to blindly overwrite resultRelInfo with the partition targetted for the apply. > * 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. This one sounds more natural to me, though. > Thoughts? May I ask why you are not moving the snapshot pop and push into the finish() and create() routines for this patch? Also, any thoughts about adding the trigger tests from 013_partition.pl to REL_13_STABLE? -- Michael
signature.asc
Description: PGP signature