On Sat, May 22, 2021 at 6:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > 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.
>
> Yeah, that's a fair point, and after some reflection I think that
> repeatedly changing the "active" field of the struct is exactly
> what was bothering me about the v2 patch. So in the attached v3,
> I went back to passing that as an explicit argument. The state
> struct now has no fields that need to change after first being set.
Thanks, that looks good to me.
> I did notice that we could remove some other random arguments
> by adding the LogicalRepRelMapEntry* to the state struct,
> so this also does that.
That seems fine.
BTW, I think we'd need to cherry-pick f3b141c4825 (or maybe parts of
it) into v13 branch for back-patching this.
--
Amit Langote
EDB: http://www.enterprisedb.com