Feedback-ID: ie0a040ee:Fastmail
Message-ID: <c9d548bc-8b0d-45e0-907f-937b3471d0c5@eisentraut.org>
Date: Fri, 10 May 2024 15:25:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: SQL:2011 application time
To: Paul Jungwirth <pj@illuminatedcomputing.com>,
 Robert Haas <robertmhaas@gmail.com>
Cc: jian he <jian.universality@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com>
 <b4bc1ef8-4215-4cf6-933c-5ef2a2827efe@illuminatedcomputing.com>
 <85dd3ad0-fa79-46e8-a08b-c426c76825f3@eisentraut.org>
 <a5d8df78-9ff6-4273-8d06-6c85b5520520@illuminatedcomputing.com>
 <49c07eac-18ec-4aff-929c-e81b87df1092@eisentraut.org>
 <CACJufxHsYDfHK62FTHdLsgDenvJd_bEuxCiyH62Lacj7NvzX4w@mail.gmail.com>
 <d579a67c-e8d7-4eb7-ae14-f6fca01489cf@illuminatedcomputing.com>
 <1edb3b79-a4bc-43b1-80aa-2e090472fe32@eisentraut.org>
 <47550967-260b-4180-9791-b224859fe63e@illuminatedcomputing.com>
 <CA+TgmoaH-2fAttd24pA3p45TUHbM9hRU6ud=brhKsp9ENHXWDg@mail.gmail.com>
 <1b2ab118-82c1-4e79-a094-04dab382e2c4@illuminatedcomputing.com>
 <CA+TgmobfDJuXJATNUco+g6AiUb3WS2Pi9FrJe9hKAHGw37RX-g@mail.gmail.com>
 <3775839b-3f0f-4c8a-ac03-a253222e6a4b@illuminatedcomputing.com>
 <731ef81e-d4c3-41c6-ba8f-f6131f4f6407@eisentraut.org>
 <1426589a-83cb-4a89-bf40-713970c07e63@illuminatedcomputing.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <1426589a-83cb-4a89-bf40-713970c07e63@illuminatedcomputing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/c9d548bc-8b0d-45e0-907f-937b3471d0c5%40eisentraut.org>
Precedence: bulk

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