Re: SQL:2011 application time - Mailing list pgsql-hackers

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)`?
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>


+      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.


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?


+      <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"?

+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 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?


@@ -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"?


+ 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