From: Amit Langote <amitlangote09@gmail.com>
> Think I have mentioned upthread that this looks better as:
>
> if (rootResultRelInfo->ri_usesMultiInsert)
> leaf_part_rri->ri_usesMultiInsert = ExecMultiInsertAllowed(leaf_part_rri);
>
> This keeps the logic confined to ExecInitPartitionInfo() where it
> belongs. No point in burdening other callers of
> ExecMultiInsertAllowed() in deciding whether or not it should pass a
> valid value for the 2nd parameter.
Oh, that's a good idea. (Why didn't I think of such a simple idea?)
> Maybe a bit late realizing this, but why does BeginForeignCopy()
> accept a ModifyTableState pointer whereas maybe just an EState pointer
> will do? I can't imagine why an FDW would want to look at the
> ModifyTableState. Case in point, I see that
> postgresBeginForeignCopy() only uses the EState from the
> ModifyTableState passed to it. I think the ResultRelInfo that's being
> passed to the Copy APIs contains most of the necessary information.
You're right. COPY is not under the control of a ModifyTable plan, so it's strange to pass ModifyTableState.
> Also, EndForeignCopy() seems fine with just receiving the EState.
I think this can have the ResultRelInfo like EndForeignInsert() and EndForeignModify() to correspond to the Begin
function:"begin/end COPYing into this relation."
> /*
> * Check whether we support copying data out of the specified relation,
> * unless the caller also passed a non-NULL data_dest_cb, in which case,
> * the callback will take care of it
> */
> if (rel != NULL && rel->rd_rel->relkind != RELKIND_RELATION &&
> data_dest_cb == NULL)
>
> I just checked that this works or at least doesn't break any newly added tests.
Good idea, too. The code has become more readable.
Thank you a lot. Your other comments that are not mentioned above are also reflected. The attached patch passes the
postgres_fdwregression test.
Regards
Takayuki Tsunakawa
