Here are a couple new patches, rebased to e305f715, addressing Peter's feedback. I'm still working
on integrating jian he's suggestions for the last patch, so I've omitted that one here.
On 5/8/24 06:51, Peter Eisentraut wrote:
> About v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch, I think the
> ideas are right, but I wonder if we can fine-tune the new conditionals a bit.
>
> --- a/src/backend/executor/execIndexing.c
> +++ b/src/backend/executor/execIndexing.c
> @@ -210,7 +210,7 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative)
> * If the indexes are to be used for speculative insertion, add extra
> * information required by unique index entries.
> */
> - if (speculative && ii->ii_Unique)
> + if (speculative && ii->ii_Unique && !ii->ii_HasWithoutOverlaps)
> BuildSpeculativeIndexInfo(indexDesc, ii);
>
> Here, I think we could check !indexDesc->rd_index->indisexclusion instead. So we
> wouldn't need ii_HasWithoutOverlaps.
Okay.
> Or we could push this into BuildSpeculativeIndexInfo(); it could just skip the rest
> if an exclusion constraint is passed, on the theory that all the speculative index
> info is already present in that case.
I like how BuildSpeculativeIndexInfo starts with an Assert that it's given a unique index, so I've
left the check outside the function. This seems cleaner anyway: the function stays more focused.
> --- a/src/backend/optimizer/util/plancat.c
> +++ b/src/backend/optimizer/util/plancat.c
> @@ -815,7 +815,7 @@ infer_arbiter_indexes(PlannerInfo *root)
> */
> if (indexOidFromConstraint == idxForm->indexrelid)
> {
> - if (!idxForm->indisunique && onconflict->action == ONCONFLICT_UPDATE)
> + if ((!idxForm->indisunique || idxForm->indisexclusion) && onconflict->action ==
> ONCONFLICT_UPDATE)
> ereport(ERROR,
> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("ON CONFLICT DO UPDATE not supported with exclusion constraints")));
>
> Shouldn't this use only idxForm->indisexclusion anyway? Like
>
> + if (idxForm->indisexclusion && onconflict->action == ONCONFLICT_UPDATE)
>
> That matches what the error message is reporting afterwards.
Agreed.
> * constraints), so index under consideration can be immediately
> * skipped if it's not unique
> */
> - if (!idxForm->indisunique)
> + if (!idxForm->indisunique || idxForm->indisexclusion)
> goto next;
>
> Maybe here we need a comment. Or make that a separate statement, like
Yes, that is nice. Done.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com