Re: [HACKERS] Add support for tuple routing to foreign partitions

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Maksim Milyutin <milyutinma(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Add support for tuple routing to foreign partitions
Date: 2018-03-30 12:56:59
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

(2018/03/19 20:25), Amit Langote wrote:
> On 2018/02/27 21:01, Etsuro Fujita wrote:
>> Attached is a new version of the patch set.

> * Comments postgres-fdw-refactoring-1.patch:
> 1. Just a minor nitpick: wouldn't it be better to call it
> create_foreign_modify_state just like its "finish" counterpart that's
> named finish_foreign_modify?

Good idea! Done.

> * Comments on foreign-routing-fdwapi-1.patch:
> 1. In the following sentence, s/rows/a tuple/g, to be consistent with
> other text added by the patch
> +<para>
> + If the<function>ExecForeignRouting</function> pointer is set to
> +<literal>NULL</literal>, attempts to route rows to the foreign table
> will fail
> + with an error message.
> +</para>

I modified the patch to use the existing API ExecForeignInsert instead
of that API and removed that API including this doc.

> 2. If I understand the description you provided in [1] correctly, the
> point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
> avoid possibly-redundantly performing following two steps in
> ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
> that may not be used for tuple routing after all:
> - create the parent_child_tupconv_maps[] entry for it
> - perform FDW tuple routing initialization.

Sorry, my explanation was not enough, but that was just one of the
reasons why I introduced those; another is to do CheckValidResultRel
against a partition after the partition was chosen for tuple routing
rather than in ExecSetupPartitionTupleRouting, to avoid aborting an
UPDATE of a partition key unnecessarily due to non-routable
foreign-partitions that may not be chosen for tuple routing after all.

Now we have ON CONFLICT for partitioned tables, which requires the
conversion map to be computed in ExecInitPartitionInfo, so I updated the
patch so that we keep the former step in ExecInitPartitionInfo and
ExecSetupPartitionTupleRouting and so that we just init the FDW in
ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting). And I
added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.

> 3. You removed the following from ExecInitPartitionInfo:
> /*
> - * Verify result relation is a valid target for an INSERT. An UPDATE
> of a
> - * partition-key becomes a DELETE+INSERT operation, so this check is
> still
> - * required when the operation is CMD_UPDATE.
> - */
> - CheckValidResultRel(leaf_part_rri, CMD_INSERT);
> but, only added back the following in ExecInsert:
> + /*
> + * Verify the specified partition is a valid target for INSERT if we
> + * didn't yet.
> + */
> + if (!resultRelInfo->ri_PartitionIsValid)
> + {
> + CheckValidResultRel(resultRelInfo, CMD_INSERT);
> Maybe, the new comment should add a "Note: " line the comment saying
> something about this code being invoked as part of an UPDATE.


Also, I fixed a bug reported from you the way you proposed [1], and
added regression tests for that. Good catch! Thanks!

Thank you for the review!

Best regards,
Etsuro Fujita


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2018-03-30 12:57:29 Re: [HACKERS] Add support for tuple routing to foreign partitions
Previous Message Etsuro Fujita 2018-03-30 12:56:15 Re: [HACKERS] Add support for tuple routing to foreign partitions