Re: [POC] Fast COPY FROM command for the table with foreign partitions

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