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
