Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS - Mailing list pgsql-hackers

From Amit Langote
Subject Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
Date
Msg-id CA+HiwqF6HXDTnkcb07HV+ErzcYpAL_0_MwviNwGi+VkzbYdkEQ@mail.gmail.com
Whole thread Raw
In response to Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Subscription tests fail under CLOBBER_CACHE_ALWAYS
List pgsql-hackers
On Thu, May 20, 2021 at 5:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@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



pgsql-hackers by date:

Previous
From: David Rowley
Date:
Subject: Re: Skip partition tuple routing with constant partition key
Next
From: David Rowley
Date:
Subject: Re: Skip partition tuple routing with constant partition key