(2018/04/26 20:06), Kyotaro HORIGUCHI wrote:
It seems almost fine for me, but just one point..

At Thu, 26 Apr 2018 17:36:44 +0900, Etsuro Fujita<fujita.ets...@lab.ntt.co.jp>  wrote 
in<5ae18f9c.6080...@lab.ntt.co.jp>
(2018/04/26 15:35), Amit Langote wrote:
On 2018/04/26 12:43, Etsuro Fujita wrote:
+            resultRelation == plan->nominalRelation)
+ resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
+    }

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

Please rewrite it to use not array reference, but pointer
reference if one mtstate logically holds just one resultRelInfo.

Maybe I don't understand your words correctky, but in that UPDATE case, I think that mtstate can have multiple ResultRelInfo.

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!

Agreed on all points above.

Thanks for reviewing!

Best regards,
Etsuro Fujita

Reply via email to