Re: SQL:2011 application time

On 3/19/24 04:02, Peter Eisentraut wrote:
 > On 16.03.24 22:37, Paul A Jungwirth wrote:
 >> Here is a new patch series addressing the last few feedback emails
 >> from Peter & Jian He. It mostly focuses on the FKs patch, trying to
 >> get it really ready to commit,
 >
 > I have committed the test changes (range and date format etc.).
 >
 > The FOREIGN KEY patch looks okay to me now.  Maybe check if any of the subsequent comments from jian
 > should be applied.

Okay, specifics below.

 > I think we could also handle multiranges in a hardcoded way?  Ranges and multiranges are hardcoded
 > concepts anyway.  It's just when we move to arbitrary types supporting containment, then it gets a
 > bit more complicated.
 >
 > What would a patch that adds just multiranges on the FK side, but without the full pluggable gist
 > support, look like?

Attached a separate patch extending FKs to multiranges only. I'd still love to support arbitrary 
types eventually but it's not part of the patches here now.

 >> I don't see any drawbacks from supporting inferred REFERENCES with
 >> temporal tables, so my vote is to break from the standard here, and
 >> *not* apply that follow-up patch. Should I add some docs about that?
 >> Also skipping the patch will cause some annoying merge conflicts, so
 >> let me know if that's what you choose and I'll handle them right away.
 >
 > I agree we can allow this.

Great, thanks! Took out those changes.

On 3/19/24 02:01, jian he wrote:
 > + * types matching the PERIOD element. periodprocoid is a GiST support
 > function to
 > + * aggregate multiple PERIOD element values into a single value
 > + * (whose return type need not match its inputs,
 > + * e.g. many ranges can be aggregated into a multirange).
 >    * And aggedperiodoperoid is also a ContainedBy operator,
 > - * but one whose rhs is anymultirange.
 > + * but one whose rhs matches the type returned by aggedperiodoperoid.
 >    * That way foreign keys can compare fkattr <@ range_agg(pkattr).
 >    */
 >   void
 > -FindFKPeriodOpers(Oid opclass,
 > -  Oid *periodoperoid,
 > -  Oid *aggedperiodoperoid)
 > +FindFKPeriodOpersAndProcs(Oid opclass,
 > +  Oid *periodoperoid,
 > +  Oid *aggedperiodoperoid,
 > +  Oid *periodprocoid)
 >
 > I think, aggedperiodoperoid is more descriptive than periodprocoid, in
 > 0005, you don't need to rename it.
 > aslo do we need to squash v29 0001 to 0005 together?

I changed the operator names to {,agged}containedbyoperoid. The proc names are not included now 
because we only need them for supporting more than ranges + multiranges.

 > --- a/doc/src/sgml/ref/create_table.sgml
 > +++ b/doc/src/sgml/ref/create_table.sgml
 > @@ -1167,7 +1167,8 @@ WITH ( MODULUS <replaceable
 > class="parameter">numeric_literal</replaceable>, REM
 >         column(s) of some row of the referenced table.  If the <replaceable
 >         class="parameter">refcolumn</replaceable> list is omitted, the
 >         primary key of the <replaceable class="parameter">reftable</replaceable>
 > -      is used.  Otherwise, the <replaceable
 > class="parameter">refcolumn</replaceable>
 > +      is used (omitting any part declared with <literal>WITHOUT
 > OVERLAPS</literal>).
 > +      Otherwise, the <replaceable class="parameter">refcolumn</replaceable>
 >         list must refer to the columns of a non-deferrable unique or primary key
 >         constraint or be the columns of a non-partial unique index.
 >        </para>
 > I think this does not express that
 > foreign key is PERIOD, then the last column of refcolumn must specify PERIOD?

Okay, added a sentence about that (and adjusted some other things re allowing implicit REFERENCES 
and only supporting ranges + multiranges).

 > +     <para>
 > +      If the last column is marked with <literal>PERIOD</literal>,
 > +      it is treated in a special way.
 > +      While the non-<literal>PERIOD</literal> columns are compared for equality
 > +      (and there must be at least one of them),
 > +      the <literal>PERIOD</literal> column is not.
 > +      Instead the constraint is considered satisfied
 > +      if the referenced table has matching records
 > +      (based on the non-<literal>PERIOD</literal> parts of the key)
 > +      whose combined <literal>PERIOD</literal> values completely cover
 > +      the referencing record's.
 > +      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>
 > you forgot to change  <literal>WITHOUT PORTION</literal> to
 > <literal>WITHOUT OVERLAPS</literal>

Oh! Thanks, I guess I was just blind.

 > Oid pf_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = FK) */
 > Oid pp_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (PK = PK) */
 > Oid ff_eq_oprs[RI_MAX_NUMKEYS]; /* equality operators (FK = FK) */
 > in struct RI_ConstraintInfo, these comments need to be updated?

In earlier feedback Peter advised not changing the "equals" language (e.g. in KeysEqual). But I 
added a comment at the top of the struct to clarify.

Rebased to 605721f819.

Yours,

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