Feedback-ID: ie0a040ee:Fastmail
Message-ID: <49c07eac-18ec-4aff-929c-e81b87df1092@eisentraut.org>
Date: Fri, 22 Mar 2024 16:49:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: SQL:2011 application time
Content-Language: en-US
To: Paul Jungwirth <pj@illuminatedcomputing.com>,
 jian he <jian.universality@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com>
 <7bd1c8f9-a91a-41a3-990e-0f796ba692ec@eisentraut.org>
 <d1a793bd-d1f1-46d7-90b6-8b89b6feb1cc@illuminatedcomputing.com>
 <6c3edb2c-bc45-4f19-babd-8f4edf39b384@illuminatedcomputing.com>
 <41b57948-f4d3-455f-8880-e0ce7e61ffc1@illuminatedcomputing.com>
 <0d977c63-f4a6-43ba-aad0-35c06c55df11@eisentraut.org>
 <CACJufxFFeUHoBomd2856amDBccNVQZzA=Jrk1_FDdvSZ1+X7og@mail.gmail.com>
 <CACJufxEACgLY7YUvzo8j=iOB-z6-1QHAvMr8cUHRP7b_F_vo3A@mail.gmail.com>
 <CA+renyXHFFyq1n2+4cwcFoLXqzUhb6u6WCL6wjk8ZjP9WxTExw@mail.gmail.com>
 <f22f65c8-865d-4753-94cc-3b954c29ad5c@eisentraut.org>
 <feee527a-6c47-42b9-a947-2422adb45a1c@illuminatedcomputing.com>
 <CACJufxF8HyERm7SKBJ+duxAP9sTgVhaW6b-oWADANGgn-o4wPA@mail.gmail.com>
 <b4bc1ef8-4215-4cf6-933c-5ef2a2827efe@illuminatedcomputing.com>
 <85dd3ad0-fa79-46e8-a08b-c426c76825f3@eisentraut.org>
 <a5d8df78-9ff6-4273-8d06-6c85b5520520@illuminatedcomputing.com>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <a5d8df78-9ff6-4273-8d06-6c85b5520520@illuminatedcomputing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/49c07eac-18ec-4aff-929c-e81b87df1092%40eisentraut.org>
Precedence: bulk

On 22.03.24 01:35, Paul Jungwirth wrote:
>  > 1. In ri_triggers.c ri_KeysEqual, you swap the order of arguments to 
> ri_AttributesEqual():
>  >
>  > -           if (!ri_AttributesEqual(riinfo->ff_eq_oprs[i], 
> RIAttType(rel, attnums[i]),
>  > -                                   oldvalue, newvalue))
>  > +           if (!ri_AttributesEqual(eq_opr, RIAttType(rel, attnums[i]),
>  > +                                   newvalue, oldvalue))
>  >
>  > But the declared arguments of ri_AttributesEqual() are oldvalue and 
> newvalue, so passing them
>  > backwards is really confusing.  And the change does matter in the tests.
>  >
>  > Can we organize this better?
> 
> I renamed the params and actually the whole function. All it's doing is 
> execute `oldvalue op newvalue`, casting if necessary. So I changed it to 
> ri_CompareWithCast and added some documentation. In an earlier version 
> of this patch I had a separate function for the PERIOD comparison, but 
> it's just doing the same thing, so I think the best thing is to give the 
> function a more accurate name and use it.

Ok, I see now, and the new explanation is better.

But after reading the comment in the function about collations, I think 
there could be trouble.  As long as we are only comparing for equality 
(and we don't support nondeterministic global collations), then we can 
use any collation to compare for equality.  But if we are doing 
contained-by, then the collation does matter, so we would need to get 
the actual collation somehow.  So as written, this might not always work 
correctly.

I think it would be safer for now if we just kept using the equality 
operation even for temporal foreign keys.  If we did that, then in the 
case that you update a key to a new value that is contained by the old 
value, this function would say "not equal" and fire all the checks, even 
though it wouldn't need to.  This is kind of similar to the "false 
negatives" that the comment already talks about.

What do you think?