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