I have not looked at the issue with the btree_gin tests yet, but here is the first part of my review.

= Review

This is my first quick review where I just read the documentation and quickly tested the feature. I will review it more in-depth later.

This is a very useful feature, one which I have a long time wished for.

The patch applies, compiles and passes the test suite with just one warning.

parse_coerce.c: In function ‘select_common_type_2args’:
parse_coerce.c:1379:7: warning: statement with no effect [-Wunused-value]

= Functional

The documentation does not agree with the code on the syntax. The documentation claims it is "FOREIGN KEY (ELEMENT xs) REFERENCES t1 (x)" when it actually is "FOREIGN KEY (EACH ELEMENT OF xs) REFERENCES t1 (x)".

Likewise I can't get the "final_positions integer[] ELEMENT REFERENCES drivers" syntax to work, but here I cannot see any change in the syntax to support it.

Related to the above: I am not sure if it is a good idea to make ELEMENT a reserved word in column definitions. What if the SQL standard wants to use it for something?

The documentation claims ON CASCADE DELETE is not supported by array element foreign keys, but I do not think that is actually the case.

I think I prefer (EACH ELEMENT OF xs) over (ELEMENT xs) given how the former is more in what I feel is the spirit of SQL. And if so we should match it as "xs integer[] EACH ELEMENT REFERENCES t1 (x)", assuming we want that syntax.

Once I have created an array element foreign key the basic features seem to work as expected.

The error message below fails to mention that it is an array element foreign key, but I do not think that is not a blocker for getting this feature merged. Right now I cannot think of how to improve it either.

$ INSERT INTO t3 VALUES ('{1,3}');
ERROR: insert or update on table "t3" violates foreign key constraint "t3_xs_fkey"
DETAIL:  Key (xs)=({1,3}) is not present in table "t1".

= Nitpicking/style comments

In doc/src/sgml/catalogs.sgml the "<entry><structfield>conpfeqop</structfield></entry>" line is incorrectly indented.

I am not fan of calling it "array-vs-scalar". What about array to scalar?

In ddl.sgml date should be lower case like the other types in "race_day DATE,".

In ddl.sgml I suggest removing the "..." from the examples to make it possible to copy paste them easily.

Your text wrapping in ddl.sqml and create_table.sgqml is quite arbitrary. I suggest wrapping all paragraphs at 80 characters (except for code which should not be wrapped). Your text editor probably has tools for wrapping paragraphs.

Please be consistent about how you write table names and SQL in general. I think almost all places use lower case for table names, while your examples in create_table.sgml are FKTABLEFORARRAY.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to