Re: SQL:2011 application time
От | Peter Eisentraut |
---|---|
Тема | Re: SQL:2011 application time |
Дата | |
Msg-id | 7bd1c8f9-a91a-41a3-990e-0f796ba692ec@eisentraut.org обсуждение исходный текст |
Ответ на | Re: SQL:2011 application time (Paul Jungwirth <pj@illuminatedcomputing.com>) |
Ответы |
Re: SQL:2011 application time
(jian he <jian.universality@gmail.com>)
Re: SQL:2011 application time (Paul Jungwirth <pj@illuminatedcomputing.com>) |
Список | pgsql-hackers |
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.
В списке pgsql-hackers по дате отправления:
Следующее
От: Heikki LinnakangasДата:
Сообщение: Re: Remove WIN32 conditional compilation from win32common.c