public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Eisentraut <[email protected]>
To: Paul Jungwirth <[email protected]>
To: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: SQL:2011 application time
Date: Fri, 22 Mar 2024 16:49:29 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CA+renyUApHgSZF9-nd-a0+OPGharLQLO=mDHcY4_qQ0+noCUVg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CACJufxFFeUHoBomd2856amDBccNVQZzA=Jrk1_FDdvSZ1+X7og@mail.gmail.com>
	<CACJufxEACgLY7YUvzo8j=iOB-z6-1QHAvMr8cUHRP7b_F_vo3A@mail.gmail.com>
	<CA+renyXHFFyq1n2+4cwcFoLXqzUhb6u6WCL6wjk8ZjP9WxTExw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CACJufxF8HyERm7SKBJ+duxAP9sTgVhaW6b-oWADANGgn-o4wPA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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?

view thread (24+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: SQL:2011 application time
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox