On Sat, May 22, 2021 at 6:01 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Amit Langote <amitlangot...@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
