Re: SQL:2011 application time

I have done a review of the temporal foreign key patches in this patch
series (0002 and 0003, v24).

The patch set needs a rebase across c85977d8fef.  I was able to do it
manually, but it's a bit tricky, so perhaps you can post a new set to
help future reviews.

(Also, the last (0007) patch has some compiler warnings and also
causes the pg_upgrade test to fail.  I didn't check this further, but
that's why the cfbot is all red.)

In summary, in principle, this all looks more or less correct to me.

As a general comment, we need to figure out the right terminology
"period" vs. "temporal", especially if we are going to commit these
features incrementally.  But I didn't look at this too hard here yet.


* v24-0002-Add-GiST-referencedagg-support-func.patch

Do we really need this level of generality?  Are there examples not
using ranges that would need a different aggregate function?  Maybe
something with geometry (points and lines)?  But it seems to me that
then we'd also need some equivalent to "without portion" support for
those types and a multirange equivalent (basically another gist
support function wrapped around the 0004 patch).


* v24-0003-Add-temporal-FOREIGN-KEYs.patch

- contrib/btree_gist/expected/without_overlaps.out
- contrib/btree_gist/sql/without_overlaps.sql

typo "exusts"


- doc/src/sgml/ref/create_table.sgml

This mentions FOR PORTION OF from a later patch.

It is not documented that SET NULL and SET DEFAULT are not supported,
even though that is added in a later patch.  (So this patch should say
that it's not supported, and then the later patch should remove that.)


- src/backend/commands/indexcmds.c

The changes to GetOperatorFromWellKnownStrategy() don't work for
message translations.  We had discussed a similar issue for this
function previously.  I think it's ok to leave the function as it was.
The additional context could be added with location pointers or
errcontext() maybe, but it doesn't seem that important for now.


- src/backend/commands/tablecmds.c

The changes in ATAddForeignKeyConstraint(), which are the meat of the
changes in this file, are very difficult to review in detail.  I tried
different git-diff options to get a sensible view, but it wasn't
helpful.  Do we need to do some separate refactoring here first?

The error message "action not supported for temporal foreign keys"
could be more detailed, mention the action.  Look for example how the
error for the generated columns is phrased.  (But note that for
generated columns, the actions are impossible to support, whereas here
it is just something not done yet.  So there should probably still be
different error codes.)


- src/backend/nodes/outfuncs.c
- src/backend/nodes/readfuncs.c

Perhaps you would like to review my patch 0001 in
<https://www.postgresql.org/message-id/859d6155-e361-4a05-8db3-4aa1f007ff28@eisentraut.org>,
which removes the custom out/read functions for the Constraint node.
Then you could get rid of these changes.


- src/backend/utils/adt/ri_triggers.c

The added #include "catalog/pg_range.h" doesn't appear to be used for
anything.

Maybe we can avoid the added #include "commands/tablecmds.h" by
putting the common function in some appropriate lower-level module.

typo "PEROID"

Renaming of ri_KeysEqual() to ri_KeysStable() doesn't improve clarity,
I think.  I think we can leave the old name and add a comment (as you
have done).  There is a general understanding around this feature set
that "equal" sometimes means "contained" or something like that.

The function ri_RangeAttributeNeedsCheck() could be documented better.
It's bit terse and unclear.  From the code, it looks like it is used
instead of row equality checks.  Maybe a different function name would
be suitable.

Various unnecessary reformatting in RI_FKey_check().

When assembling the SQL commands, you need to be very careful about
fully quoting and schema-qualifying everything.  See for example
ri_GenerateQual().

Have you checked that the generated queries can use indexes and have
suitable performance?  Do you have example execution plans maybe?


- src/backend/utils/adt/ruleutils.c

This seems ok in principle, but it's kind of weird that the new
argument of decompile_column_index_array() is called "withPeriod"
(which seems appropriate seeing what it does), but what we are passing
in is conwithoutoverlaps.  Maybe we need to reconsider the naming of
the constraint column?  Sorry, I made you change it from "contemporal"
or something, didn't I?  Maybe "conperiod" would cover both meanings
better?


- src/backend/utils/cache/lsyscache.c

get_func_name_and_namespace(): This function would at least need some
identifier quoting.  There is only one caller (lookupTRIOperAndProc),
so let's just put this code inline there; it's not worth a separate
global function.  (Also, you could use psprintf() here to simplify
palloc() + snprintf().)


- src/include/catalog/pg_constraint.h

You are changing in several comments "equality" to "comparison".  I
suspect you effectively mean "equality or containment"?  Maybe
"comparison" is too subtle to convey that meaning?  Maybe be more
explicit.

You are changing a foreign key from DECLARE_ARRAY_FOREIGN_KEY to
DECLARE_ARRAY_FOREIGN_KEY_OPT.  Add a comment about it, like the one
just above has.


- src/include/catalog/pg_proc.dat

For the names of the trigger functions, maybe instead of

     TRI_FKey_check_ins

something like

     RI_FKey_period_check_ins

so that all RI trigger functions group under a common prefix.

On second thought, do we even need separate functions for this?
Looking at ri_triggers.c, the temporal and non-temporal functions are
the same, and all the differences are handled in the underlying
implementation functions.


- src/include/nodes/parsenodes.h

The constants FKCONSTR_PERIOD_OP_CONTAINED_BY and
FKCONSTR_PERIOD_PROC_REFERENCED_AGG could use more documentation here.

For the Constraint struct, don't we just need a bool field saying
"this is a period FK", and then we'd know that the last column is the
period?  Like we did for the primary keys (bool without_overlaps).


- src/include/parser/kwlist.h

For this patch, the keyword PERIOD can be unreserved.  But it
apparently will need to be reserved later for the patch that
introduces PERIOD columns.  Maybe it would make sense to leave it
unreserved for this patch and upgrade it in the later one.