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