Re: SQL:2011 application time

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