Re: SQL:2011 application time

On Mon, Jan 29, 2024 at 8:00 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> I fixed your tests, some of your tests can be simplified, (mainly
> primary key constraint is unnecessary for the failed tests)
> also your foreign key patch test table, temporal_rng is created at
> line 141, and we use it at around line 320.
> it's hard to get the definition of temporal_rng. I drop the table
> and recreate it.
> So people can view the patch with tests more easily.
>
I've attached a new patch that further simplified the tests. (scope
v24 patch's 0002 and 0003)
Please ignore previous email attachments.

I've only applied the v24, 0002, 0003.
seems in doc/src/sgml/ref/create_table.sgml
lack the explanation of `<replaceable
class="parameter">temporal_interval</replaceable>`

since foreign key ON {UPDATE | DELETE} {CASCADE,SET NULL,SET DEFAULT}
not yet supported,
v24-0003 create_table.sgml should reflect that.

+ /*
+ * For FKs with PERIOD we need an operator and aggregate function
+ * to check whether the referencing row's range is contained
+ * by the aggregated ranges of the referenced row(s).
+ * For rangetypes this is fk.periodatt <@ range_agg(pk.periodatt).
+ * FKs will look these up at "runtime", but we should make sure
+ * the lookup works here.
+ */
+ if (is_temporal)
+ FindFKPeriodOpersAndProcs(opclasses[numpks - 1], &periodoperoid,
&periodprocoid);

within the function ATAddForeignKeyConstraint, you called
FindFKPeriodOpersAndProcs,
but never used the computed outputs: periodoperoid, periodprocoid,
opclasses.
We validate these(periodoperoid, periodprocoid) at
lookupTRIOperAndProc, FindFKPeriodOpersAndProcs.
I'm not sure whether FindFKPeriodOpersAndProcs in
ATAddForeignKeyConstraint is necessary.

+ * Check if all key values in OLD and NEW are "equivalent":
+ * For normal FKs we check for equality.
+ * For temporal FKs we check that the PK side is a superset of its old
value,
+ * or the FK side is a subset.
"or the FK side is a subset." is misleading, should it be something
like "or the FK side is a subset of X"?

+ if (indexStruct->indisexclusion) return i - 1;
+ else return i;

I believe our style should be (with proper indent)
if (indexStruct->indisexclusion)
return i - 1;
else
return i;

in transformFkeyCheckAttrs
+ if (found && is_temporal)
+ {
+ found = false;
+ for (j = 0; j < numattrs + 1; j++)
+ {
+ if (periodattnum == indexStruct->indkey.values[j])
+ {
+ opclasses[numattrs] = indclass->values[j];
+ found = true;
+ break;
+ }
+ }
+ }

can be simplified:
{
found = false;
if (periodattnum == indexStruct->indkey.values[numattrs])
{
opclasses[numattrs] = indclass->values[numattrs];
found = true;
}
}

Also wondering, at the end of the function transformFkeyCheckAttrs `if
(!found)` part:
do we need another error message handle is_temporal is true?

@@ -212,8 +213,11 @@ typedef struct NewConstraint
ConstrType contype; /* CHECK or FOREIGN */
Oid refrelid; /* PK rel, if FOREIGN */
Oid refindid; /* OID of PK's index, if FOREIGN */
+ bool conwithperiod; /* Whether the new FOREIGN KEY uses PERIOD */
Oid conid; /* OID of pg_constraint entry, if FOREIGN */
Node *qual; /* Check expr or CONSTR_FOREIGN Constraint */
+ Oid *operoids; /* oper oids for FOREIGN KEY with PERIOD */
+ Oid *procoids; /* proc oids for FOREIGN KEY with PERIOD */
ExprState *qualstate; /* Execution state for CHECK expr */
} NewConstraint;
primary key can only one WITHOUT OVERLAPS,
so *operoids and *procoids
can be replaced with just
`operoids, procoids`.
Also these two elements in struct NewConstraint not used in v24, 0002, 0003.