Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS

On Thu, May 20, 2021 at 5:23 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)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