Re: Oddity in tuple routing for foreign partitions

(2018/04/26 15:35), Amit Langote wrote:
> On 2018/04/26 12:43, Etsuro Fujita wrote:
>> (2018/04/25 17:29), Amit Langote wrote:
>>> Hmm, I missed that we do need information from a proper RTE as well. So,
>>> I suppose we should be doing this instead of creating the RTE for foreign
>>> partition from scratch.
>>>
>>> + rte = list_nth(estate->es_range_table, resultRelation - 1);
>>> + rte = copyObject(rte);
>>> + rte->relid = RelationGetRelid(rel);
>>> + rte->relkind = RELKIND_FOREIGN_TABLE;
>>
>> As I said upthread, we can use the RTE in the range table as-is if the
>> foreign table is one of the UPDATE subplan partitions or the target
>> specified in a COPY command. So, I created the patch that way because
>> that would save cycles. Why not just use that RTE in those cases?
>
> Yes, I agree it's better to reuse the RTE in the cases we can. So, how
> does this look:
>
> + /*
> + * If the foreign table is a partition, we need to create a new RTE
> + * describing the foreign table for use by deparseInsertSql and
> + * create_foreign_modify() below, after first copying the parent's
> + * RTE and modifying some fields to describe the foreign partition to
> + * work on. However, if this is invoked by UPDATE, the existing RTE
> + * may already correspond to this partition if it is one of the
> + * UPDATE subplan target rels; in that case, we can just use the
> + * existing RTE as-is.
> + */
> + rte = list_nth(estate->es_range_table, resultRelation - 1);
> + if (rte->relid != RelationGetRelid(rel))
> + {
> + rte = copyObject(rte);
> + rte->relid = RelationGetRelid(rel);
> + rte->relkind = RELKIND_FOREIGN_TABLE;
> +
> + /*
> + * For UPDATE, we must use the RT index of the first subplan
> + * target rel's RTE, because the core code would have built
> + * expressions for the partition, such as RETURNING, using that
> + * RT index as varno of Vars contained in those expressions.
> + */
> + if (plan&& plan->operation == CMD_UPDATE&&
> + resultRelation == plan->nominalRelation)
> + resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
> + }

Seems like a better change than mine; because this simplifies the logic.

> I added the block inside the if because I agree with your comment below
> that the changes to ExecInitPartitionInfo are better kept for later to
> think more carefully about the dependencies it might break.

OK

>>> If we apply the other patch I proposed, resultRelation always points to
>>> the correct RTE.
>>>
>>>>> I tried to do that. So, attached are two patches.
>>>>>
>>>>> 1. Fix for ExecInitPartitionInfo to use the right RT index to pass to
>>>>> InitResultRelInfo
>>>>>
>>>>> 2. v5 of the patch to fix the bug of foreign partitions
>>>>>
>>>>> Thoughts?
>>
>> Actually, I also thought the former when creating the patch, but I left
>> that as-is because I'm not sure that would be really safe; ExecConstraints
>> looks at the RT index via GetInsertedColumns/GetUpdatedColumns, so what I
>> thought was: that might cause unexpected behavior.
>
> OK, I have to agree here that we better leave 1 to be looked at later.
>
> After this change, GetInsertedColumns/GetUpdatedColumns will start
> returning a different set of columns in some cases than it does now.
> Currently, it *always* returns a set containing root table's attribute
> numbers, even for UPDATE. But with this change, for UPDATE, it will start
> returning the set containing the first subplan target rel's attribute
> numbers, which could be any partition with arbitrarily different attribute
> numbers.
>
>> Anyway, I think that
>> the former is more like an improvement rather than a fix, so it would be
>> better to leave that for another patch for PG12?
>
> I agree, so I'm dropping the patch for 1.

OK, let's focus on #2!

> See attached an updated version with changes as described above.

Looks good to me. Thanks for the updated version!

Best regards,
Etsuro Fujita