On Thu, Jul 18, 2019 at 4:51 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Thu, Jul 18, 2019 at 2:53 PM Andres Freund <and...@anarazel.de> wrote: > > On 2019-07-18 14:24:29 +0900, Amit Langote wrote: > > > On Thu, Jul 18, 2019 at 10:09 AM Andres Freund <and...@anarazel.de> wrote: > > > > 1) How come partition routing is done outside of ExecInsert()? > > > > > > > > case CMD_INSERT: > > > > /* Prepare for tuple routing if needed. */ > > > > if (proute) > > > > slot = ExecPrepareTupleRouting(node, estate, proute, > > > > resultRelInfo, slot); > > > > slot = ExecInsert(node, slot, planSlot, > > > > estate, node->canSetTag); > > > > /* Revert ExecPrepareTupleRouting's state change. */ > > > > if (proute) > > > > estate->es_result_relation_info = resultRelInfo; > > > > break; > > > > > > > > That already seems like a layering violation, > > > > > > The decision to move partition routing out of ExecInsert() came about > > > when we encountered a bug [1] whereby ExecInsert() would fail to reset > > > estate->es_result_relation_info back to the root table if it had to > > > take an abnormal path out (early return), of which there are quite a > > > few instances. The first solution I came up with was to add a goto > > > label for the code to reset estate->es_result_relation_info and jump > > > to it from the various places that do an early return, which was > > > complained about as reducing readability. So, the solution we > > > eventually settled on in 6666ee49f was to perform ResultRelInfos > > > switching at a higher level. > > > > I think that was the wrong path, given that the code now lives in > > multiple places. Without even a comment explaining that if one has to be > > changed, the other has to be changed too.
I thought this would be OK because we have the ExecPrepareTupleRouting() call in just two places in a single source file, at least currently. > > Or perhaps the actually correct fix is to remove es_result_relation_info > > alltogether, and just pass it down the places that need it - we've a lot > > more code setting it than using the value. And it'd not be hard to > > actually pass it to the places that read it. Given all the > > setting/resetting of it it's pretty obvious that a query-global resource > > isn't the right place for it. > > I tend to agree that managing state through es_result_relation_info > across various operations on a result relation has turned a bit messy > at this point. That said, while most of the places that access the > currently active result relation from es_result_relation_info can be > easily modified to receive it directly, the FDW API BeginDirectModify > poses bit of a challenge. BeginDirectlyModify() is called via > ExecInitForeignScan() that in turn can't be changed to add a result > relation (Index or ResultRelInfo *) argument, so the only way left for > BeginDirectlyModify() is to access it via es_result_relation_info. That's right. I'm not sure that's a good idea, because I think other extensions also might look at es_result_relation_info, and if so, removing es_result_relation_info altogether would require the extension authors to update their extensions without any benefit, which I think isn't a good thing. Best regards, Etsuro Fujita
