Re: SQL:2011 application time

I have committed the
v2-0001-Fix-ON-CONFLICT-DO-NOTHING-UPDATE-for-temporal-in.patch from
this (confusingly, there was also a v2 earlier in this thread), and I'll
continue working on the remaining items.

On 09.05.24 06:24, Paul Jungwirth wrote:
> 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,
>