Re: SQL:2011 application time

Hi All,

A few more changes here:

On 3/17/24 16:30, jian he wrote:
 > Hi, minor issues from 00001 to 0005.
 > +      <row>
 > +       <entry><function>referencedagg</function></entry>
 > +       <entry>aggregates referenced rows' <literal>WITHOUT OVERLAPS</literal>
 > +        part</entry>
 > +       <entry>13</entry>
 > +      </row>
 > comparing with surrounding items, maybe need to add `(optional)`?

We do say this function is optional above, in the list of support functions. That seems to be the 
normal approach. The only other support function that mentions being optional elsewhere is sortsupport.

 > I think the explanation is not good as explained in referencedagg entry below:
 >        <para>
 >         An aggregate function. Given values of this opclass,
 >         it returns a value combining them all. The return value
 >         need not be the same type as the input, but it must be a
 >         type that can appear on the right hand side of the "contained by"
 >         operator. For example the built-in <literal>range_ops</literal>
 >         opclass uses <literal>range_agg</literal> here, so that foreign
 >         keys can check <literal>fkperiod @> range_agg(pkperiod)</literal>.
 >        </para>

Can you explain what you'd like to see improved here?

 > +      In other words, the reference must have a referent for its
 > entire duration.
 > +      This column must be a column with a range type.
 > +      In addition the referenced table must have a primary key
 > +      or unique constraint declared with <literal>WITHOUT PORTION</literal>.
 > +     </para>
 > seems you missed replacing this one.

I'm not sure what this is referring to. Replaced what?

 > in v28-0002, the function name is FindFKPeriodOpers,
 > then in v28-0005 rename it to FindFKPeriodOpersAndProcs?
 > renaming the function name in a set of patches seems not a good idea?

We'll only apply part 5 if we support more than range types (though I think that would be great). It 
doesn't make sense to name this function FindFKPeriodOpersAndProcs when it isn't yet finding a proc. 
If it's a problem to rename it in part 5 perhaps the commits should be squashed by the committer? 
But I don't see the problem really.

 > +      <para>
 > +       This is used for temporal foreign key constraints.
 > +       If you omit this support function, your type cannot be used
 > +       as the <literal>PERIOD</literal> part of a foreign key.
 > +      </para>
 > in v28-0004, I think here "your type"  should change to "your opclass"?

I think "your type" addresses what the user is more likely to care about, but I added some 
clarification here.

 > +bool
 > +check_amproc_is_aggregate(Oid funcid)
 > +{
 > + bool result;
 > + HeapTuple tp;
 > + Form_pg_proc procform;
 > +
 > + tp = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
 > + if (!HeapTupleIsValid(tp))
 > + elog(ERROR, "cache lookup failed for function %u", funcid);
 > + procform = (Form_pg_proc) GETSTRUCT(tp);
 > + result = procform->prokind == 'a';
 > + ReleaseSysCache(tp);
 > + return result;
 > +}
 > maybe
 > `
 > change procform->prokind == 'a';
 > `
 > to
 > `
 > procform->prokind == PROKIND_AGGREGATE;
 > `
 > or we can put the whole function to cache/lsyscache.c
 > name it just as proc_is_aggregate.

Added the constant reference. Since lsyscache.c already has get_func_prokind, I changed the gist 
validation function to call that directly.

 > -  Added pg_dump support.
 > - Show the correct syntax in psql \d output for foreign keys.
 > in 28-0002, seems there is no work to correspond to these 2 items in
 > the commit message?

The changes to psql and pg_dump happen in pg_get_constraintdef_worker and 
decompile_column_index_array (both in ruleutils.c).

 > @@ -12335,7 +12448,8 @@ validateForeignKeyConstraint(char *conname,
 >    Relation rel,
 >    Relation pkrel,
 >    Oid pkindOid,
 > - Oid constraintOid)
 > + Oid constraintOid,
 > + bool temporal)
 > do you need to change the last argument of this function to "is_period"?

Changed to hasperiod.

 > + sprintf(paramname, "$%d", riinfo->nkeys);
 > + sprintf(paramname, "$%d", riinfo->nkeys);
 > do you think it worth the trouble to change to snprintf, I found
 > related post on [1].
 >
 > [1] https://stackoverflow.com/a/7316500/15603477

paramname holds 16 chars so I don't think there is any risk of an int overflowing here. The existing 
foreign key code already uses sprintf, so I don't think it makes sense to be inconsistent here. And 
if we want to change it it should probably be in a separate commit, not buried in a commit about 
adding temporal foreign keys.

On 3/17/24 21:47, jian he wrote:
 > one more minor issue related to error reporting.
 > I've only applied v28, 0001 to 0005.
 >
 > -- (parent_id, valid_at) REFERENCES [implicit]
 > -- FOREIGN KEY part should specify PERIOD
 > CREATE TABLE temporal_fk_rng2rng (
 > id int4range,
 > valid_at daterange,
 > parent_id int4range,
 > CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
 > CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, valid_at)
 > REFERENCES temporal_rng
 > );
 > ERROR:  number of referencing and referenced columns for foreign key disagree
 >
 > -- (parent_id, PERIOD valid_at) REFERENCES (id)
 > CREATE TABLE temporal_fk_rng2rng (
 > id int4range,
 > valid_at daterange,
 > parent_id int4range,
 > CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
 > CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
 > REFERENCES temporal_rng (id)
 > );
 > ERROR:  foreign key uses PERIOD on the referencing table but not the
 > referenced table
 >
 > these error messages seem somehow inconsistent with the comments above?

Clarified the comments.

 > + else
 > + {
 > + /*
 > + * Check it's a btree; currently this can never fail since no other
 > + * index AMs support unique indexes.  If we ever did have other types
 > + * of unique indexes, we'd need a way to determine which operator
 > + * strategy number is equality.  (Is it reasonable to insist that
 > + * every such index AM use btree's number for equality?)
 > + */
 > + if (amid != BTREE_AM_OID)
 > + elog(ERROR, "only b-tree indexes are supported for foreign keys");
 > + eqstrategy = BTEqualStrategyNumber;
 > + }
 >
 > the comments say never fail.
 > but it actually failed. see:
 >
 > +-- (parent_id) REFERENCES [implicit]
 > +-- This finds the PK (omitting the WITHOUT OVERLAPS element),
 > +-- but it's not a b-tree index, so it fails anyway.
 > +-- Anyway it must fail because the two sides have a different
 > definition of "unique".
 > +CREATE TABLE temporal_fk_rng2rng (
 > + id int4range,
 > + valid_at daterange,
 > + parent_id int4range,
 > + CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
 > + CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id)
 > + REFERENCES temporal_rng
 > +);
 > +ERROR:  only b-tree indexes are supported for foreign keys

You're right, now that we have temporal primary keys the comment is out-of-date.
You can reach that error message by creating a regular foreign key against a temporal primary key.

Perhaps we should update the comment separately, although I haven't added a new patch for that here.
I did update the comment as part of this FK patch. I also added "non-PERIOD" to the error message
(which only makes sense in the FK patch). Since the error message was impossible before, I assume 
that is no problem. I think this is a simpler fix than what you have in your attached patch. In 
addition your patch doesn't work if we include part 3 here: see Peter's feedback about the SQL 
standard and my reply.

Rebased to 846311051e.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com