Thanks for the patch.
Amit Langote wrote:
> I mentioned this case at [1] and had a WIP patch to address that. Please
> find it attached here. It is to be applied on top of both of your patches.
In this bit:
> + /*
> + * When specific arbiter indexes requested, only examine them. If
> + * this is a partition (after a tuple is routed to it from the
> + * parent into which the original tuple has been inserted), we must
> + * check the parent index id, instead of our own id, because that's
> + * the one that appears in the arbiterIndexes list.
> + */
> if (arbiterIndexes != NIL &&
> - !list_member_oid(arbiterIndexes,
> - indexRelation->rd_index->indexrelid))
> + !(list_member_oid(arbiterIndexes,
> + indexRelation->rd_index->indexrelid) ||
> + list_member_oid(arbiterIndexes,
> + indexRelation->rd_index->indparentidx)))
I think this would fail if there is two-level partitioning (or more),
because the index mentioned in the arbiter indexes list would be the
grand-parent index and would not appear in indparentidx. Maybe what we
need is to map the parent index ids to partition indexes, all the way up
in ExecInsert before calling ExecCheckIndexConstraints, which looks
pretty annoying.
Another I'm mildly worried about is the use of ModifyState->nominalRelation,
which is said to be "for the use of EXPLAIN"; in this new code, we're
extending its charter so that we're actually relying on it for
execution. Maybe this is not a problem and we just need to update the
comments (if we believe that we maintain it reliably enough, which is
probably true), but I'm not certain.
I also wonder about short-circuiting the build of the on-conflict stuff
for the parent table in the partitioned case, because surely we don't
need it. But that seems fairly minor.
Thanks again,
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
