On 2/15/21 1:31 PM, Amit Langote wrote:
Tsunakawa-san, Andrey,
+static void
+postgresBeginForeignCopy(ModifyTableState *mtstate,
+ ResultRelInfo *resultRelInfo)
+{
...
+ if (resultRelInfo->ri_RangeTableIndex == 0)
+ {
+ ResultRelInfo *rootResultRelInfo = resultRelInfo->ri_RootResultRelInfo;
+
+ rte = exec_rt_fetch(rootResultRelInfo->ri_RangeTableIndex, estate);
It's better to add an Assert(rootResultRelInfo != NULL) here.
Apparently, there are cases where ri_RangeTableIndex == 0 without
ri_RootResultRelInfo being set. The Assert will ensure that
BeginForeignCopy() is not mistakenly called on such ResultRelInfos.
+1
I can't parse what the function's comment says about "using list of
parameters". Maybe it means to say "list of columns" specified in the
COPY FROM statement. How about writing this as:
/*
* Deparse remote COPY FROM statement
*
* Note that this explicitly specifies the list of COPY's target columns
* to account for the fact that the remote table's columns may not match
* exactly with the columns declared in the local definition.
*/
I'm hoping that I'm interpreting the original note correctly. Andrey?
Yes, this is a good option.
+ <para>
+ <literal>mtstate</literal> is the overall state of the
+ <structname>ModifyTable</structname> plan node being executed;
global data about
+ the plan and execution state is available via this structure.
...
+typedef void (*BeginForeignCopy_function) (ModifyTableState *mtstate,
+ ResultRelInfo *rinfo);
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.
Also, EndForeignCopy() seems fine with just receiving the EState.
+1
If the intention is to only prevent this error, maybe the condition
above could be changed as this:
/*
* 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)
Agreed. This is an atavism. In the first versions, I did not use the
data_dest_cb routine. But now this is a redundant parameter.
--
regards,
Andrey Lepikhov
Postgres Professional
