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,
>