[PATCH] Support for Array ELEMENT Foreign Keys

Hi,
please find attached the refreshed v1 patch.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

On Tue, Sep 18, 2012 at 05:52:51PM +0200, Marco Nenciarini wrote:

please find attached the refreshed v1 patch.

I perused this version in comparison to the last version I reviewed, finding
minor problems. First, a warning:

tablecmds.c: In function `ATExecAddConstraint':
tablecmds.c:5898: warning: `fk_element_type' may be used uninitialized in this function
tablecmds.c:5898: note: `fk_element_type' was declared here

I don't see an actual bug; add a dead store to silence the compiler.

*** a/src/test/regress/parallel_schedule
--- b/src/test/regress/parallel_schedule
*************** test: event_trigger
*** 94,100 ****
# ----------
# Another group of parallel tests
# ----------
! test: select_views portals_p2 foreign_key cluster dependency guc bitmapops combocid tsearch tsdicts foreign_data window xmlmap functional_deps advisory_lock json

# ----------
# Another group of parallel tests
--- 94,100 ----
# ----------
# Another group of parallel tests
# ----------
! test: select_views portals_p2 foreign_key cluster dependency guc bitmapops combocid tsearch tsdicts foreign_data window xmlmap functional_deps advisory_lock element_foreign_key

Keep that json test.

+ 				errmsg("array ELEMENT foreign keys only support NO ACTION "
+ 					   "and RESTRICT actions")));

Project style is not to break message literals; instead, let the line run
long. There are a few more examples of this in your patch.

Those problems are isolated and do not impugn design, so committer time would
be just as well-spent on the latest version. As such, I'm marking the patch
Ready for Committer. Thanks to Rafal Pietrak for his helpful review.

nm

Thanks for your review.

Please find the attached refreshed patch (v2) which fixes the loose ends
you found.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

Marco Nenciarini <marco.nenciarini@2ndquadrant.it> writes:

Please find the attached refreshed patch (v2) which fixes the loose ends
you found.

I've started looking at this patch, and the first thing I notice is that
the syntax doesn't work. It's ambiguous, and this:

%left JOIN CROSS LEFT FULL RIGHT INNER_P NATURAL
/* kluge to keep xml_whitespace_option from causing shift/reduce conflicts */
%right PRESERVE STRIP_P
+ %nonassoc ELEMENT

%%

is not in any way an acceptable fix. All that that will do is cause an
arbitrary choice to be made when it's not clear what to do. Half the
time the arbitrary choice will be wrong. Consider for example

regression=# create table t1 (f1 int[] default 4! element references t2);
ERROR: column "element" does not exist

The parser has resolved the ambiguity about whether "!" is an infix or
postfix operator by assuming it's infix. (Yeah, I realize we've "fixed"
some similar cases with precedence hacks, but they are cases that were
forced on us by brain-dead syntax choices in the SQL standard. We don't
need to go there for syntax we're making up ourselves.)

We could get around that by making ELEMENT a fully reserved word, but
I don't think that's a really acceptable solution. ELEMENT is reserved
according to more recent versions of the SQL standard, but only as a
built-in function name, and in any case reserving it is very likely to
break people's existing applications.

Another possibility is to forget about the column constraint ELEMENT
REFERENCES syntax, and only support the table-constraint syntax with
ELEMENT inside the column list --- I've not checked, but I think that
syntax doesn't have any ambiguity problems.

Or we could go back to using ARRAY here --- that should be safe since
ARRAY is already fully reserved.

Or we could choose some other syntax. I'm wondering about dropping the
use of a keyword entirely, and instead using '[]' decoration. This
wouldn't work too badly in the table constraint case:

FOREIGN KEY (foo, bar[]) REFERENCES t (x,y)

but I'm less sure where to put the decoration for the column constraint
case.

Thoughts?

regards, tom lane

I wrote:

Or we could go back to using ARRAY here --- that should be safe since
ARRAY is already fully reserved.

Ugh ... no, that doesn't work, because ARRAY[...] is allowed in c_expr
and hence b_expr. So the ambiguity would still be there. We'd need a
different fully-reserved keyword to go this way.

regards, tom lane

On 10/18/2012 10:26 PM, Tom Lane wrote:

Another possibility is to forget about the column constraint ELEMENT
REFERENCES syntax, and only support the table-constraint syntax with
ELEMENT inside the column list --- I've not checked, but I think that
syntax doesn't have any ambiguity problems.

Or we could go back to using ARRAY here --- that should be safe since
ARRAY is already fully reserved.

Or we could choose some other syntax. I'm wondering about dropping the
use of a keyword entirely, and instead using '[]' decoration. This
wouldn't work too badly in the table constraint case:

FOREIGN KEY (foo, bar[]) REFERENCES t (x,y)

but I'm less sure where to put the decoration for the column constraint
case.

Thoughts?

I'm late to this party, so I apologize in advance if this has already
been considered, but do we actually need a special syntax? Can't we just
infer that we have one of these when the referring column is an array
and the referenced column is of the base type of the array?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

I'm late to this party, so I apologize in advance if this has already
been considered, but do we actually need a special syntax? Can't we just
infer that we have one of these when the referring column is an array
and the referenced column is of the base type of the array?

Yeah, that was suggested before. I for one think it's a seriously bad
idea. It takes away, or at least weakens, a fundamental sanity check
on foreign-key references.

Another point (which is not well handled by my []-syntax idea, I guess)
is that it's not clear that there is one and only one sensible semantics
for the case of an array referencing a scalar. We debated about "all
elements of array must have a match" versus "at least one element of
array must have a match". If we have some special syntax in there then
there's room to change the syntax to select a different semantics,
whereas if we just automatically do something when the column types
are inconsistent, we're not gonna have a lot of wiggle room to support
other behaviors.

This thought also crystallizes something else that had been bothering me,
which is that "ELEMENT" alone is a pretty bad choice of syntax because
it entirely fails to make clear which of these semantics is meant.
I'm tempted to propose that we use

FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES ...

which is certainly more verbose than just "ELEMENT" but I think it
makes it clearer that each array element is required to have a match
separately. If we ever implemented the other behavior it could be
written as "ANY ELEMENT OF".

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

regards, tom lane

On 10/19/2012 03:55 PM, Tom Lane wrote:

This thought also crystallizes something else that had been bothering me,
which is that "ELEMENT" alone is a pretty bad choice of syntax because
it entirely fails to make clear which of these semantics is meant.
I'm tempted to propose that we use

FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES ...

which is certainly more verbose than just "ELEMENT" but I think it
makes it clearer that each array element is required to have a match
separately. If we ever implemented the other behavior it could be
written as "ANY ELEMENT OF".

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

"ALL" is a fully reserved keyword. Could we do something like "ALL
ELEMENTS"?

cheers

andrew

On Fri, Oct 19, 2012 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

This is a little sneaky, but I presume you only get the grammar
conflict if you try to sneak the "each" or "element" or "each element"
or whatever-you-call-it designator in BEFORE the column name. So what
about just putting it afterwards? Something like this:

FOREIGN KEY (a, b BY ELEMENT) REFERENCES ...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/19/2012 03:55 PM, Tom Lane wrote:

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

"ALL" is a fully reserved keyword. Could we do something like "ALL
ELEMENTS"?

[ experiments... ] bison is happy with "ALL ELEMENTS REFERENCES ..."
as a column constraint, but from the standpoint of English grammar
it's kinda sucky. "ANY ELEMENT REFERENCES ..." would be fine but
that's not the case we're implementing now.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

This is a little sneaky, but I presume you only get the grammar
conflict if you try to sneak the "each" or "element" or "each element"
or whatever-you-call-it designator in BEFORE the column name. So what
about just putting it afterwards? Something like this:

FOREIGN KEY (a, b BY ELEMENT) REFERENCES ...

That's not the syntax we're having problems with, it's the column
constraint syntax; that is

CREATE TABLE t1 (c int[] REFERENCES t2);

It looks like we could support

CREATE TABLE t1 (c int[] REFERENCES BY ELEMENT t2);

but (1) this doesn't seem terribly intelligible to me, and
(2) I don't see how we modify that if we want to provide
at-least-one-match semantics later.

regards, tom lane

On Fri, Oct 19, 2012 at 5:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

It looks like we could support

CREATE TABLE t1 (c int[] REFERENCES BY ELEMENT t2);

but (1) this doesn't seem terribly intelligible to me, and
(2) I don't see how we modify that if we want to provide
at-least-one-match semantics later.

What about something more generic?

CREATE TABLE <tname> ( <cname> <type> [(<expr>)] REFERENCES <t2name>
[(<t2expr>)] )

Meaning, if <expr> is missing, it's taken <expr> = <cname>, if not,
it's the result of that expression the one that references the target
table.

Sounds crazy, but with ALL() and ANY() it ought to support lots of subcases.

On Friday, October 19, 2012 09:55:10 PM Tom Lane wrote:

FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES ...

which is certainly more verbose than just "ELEMENT" but I think it
makes it clearer that each array element is required to have a match
separately. If we ever implemented the other behavior it could be
written as "ANY ELEMENT OF".

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

What about sticking a WHERE in there? I.e. FOREIGN KEY (foo, WHERE EACH
ELEMENT OF bar) ...

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Claudio Freire <klaussfreire@gmail.com> writes:

What about something more generic?

CREATE TABLE <tname> ( <cname> <type> [(<expr>)] REFERENCES <t2name>
[(<t2expr>)] )

Meaning, if <expr> is missing, it's taken <expr> = <cname>, if not,
it's the result of that expression the one that references the target
table.

Doesn't seem terribly sensible as a column constraint: a column
constraint ought to just be on the current column. If you want
something more generic, the table-constraint syntax would be the
place for it ... but that's not where we have a syntax problem.

regards, tom lane

Andres Freund <andres@2ndquadrant.com> writes:

What about sticking a WHERE in there? I.e. FOREIGN KEY (foo, WHERE EACH
ELEMENT OF bar) ...

Well, we don't really need it in the table-constraint case. The
column-constraint case is the sticking point.

I tested, and indeed this seems to work:

CREATE TABLE t1 (c int[] WHERE EACH ELEMENT REFERENCES t2);

and it's perfectly sensible from an English-grammar standpoint too.

If we take that, how would we spell the table-constraint case exactly?
Grammatically I'd prefer

FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES

but this seems a bit far afield from the column-constraint syntax.
OTOH, that's a pretty minor quibble. These work according to bison,
and they wouldn't make a grammarian run away screaming, so maybe we
should just be happy with that.

regards, tom lane

On 10/19/2012 04:40 PM, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/19/2012 03:55 PM, Tom Lane wrote:

That doesn't get us any closer to having a working column-constraint
syntax unfortunately, because EACH is not a reserved word either
so "EACH ELEMENT REFERENCES" still isn't gonna work. I'm getting
more willing to give up on having a column-constraint form of this.

"ALL" is a fully reserved keyword. Could we do something like "ALL
ELEMENTS"?

[ experiments... ] bison is happy with "ALL ELEMENTS REFERENCES ..."
as a column constraint, but from the standpoint of English grammar
it's kinda sucky. "ANY ELEMENT REFERENCES ..." would be fine but
that's not the case we're implementing now.

Well, we could add "REFERENCE" as a non-reserved keyword. I agree it's
not ideal.

cheers

andrew

I wrote:

I tested, and indeed this seems to work:
CREATE TABLE t1 (c int[] WHERE EACH ELEMENT REFERENCES t2);
and it's perfectly sensible from an English-grammar standpoint too.
If we take that, how would we spell the table-constraint case exactly?
Grammatically I'd prefer
FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES

Are people happy with these syntax proposals, or do we need some other
color for the bikeshed?

regards, tom lane

2012/10/22 Tom Lane <tgl@sss.pgh.pa.us>:

I wrote:

I tested, and indeed this seems to work:
CREATE TABLE t1 (c int[] WHERE EACH ELEMENT REFERENCES t2);
and it's perfectly sensible from an English-grammar standpoint too.
If we take that, how would we spell the table-constraint case exactly?
Grammatically I'd prefer
FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES

Are people happy with these syntax proposals, or do we need some other
color for the bikeshed?

I am ok

Pavel

On 10/22/2012 12:08 PM, Tom Lane wrote:

I wrote:

I tested, and indeed this seems to work:
CREATE TABLE t1 (c int[] WHERE EACH ELEMENT REFERENCES t2);
and it's perfectly sensible from an English-grammar standpoint too.
If we take that, how would we spell the table-constraint case exactly?
Grammatically I'd prefer
FOREIGN KEY (foo, EACH ELEMENT OF bar) REFERENCES

Are people happy with these syntax proposals, or do we need some other
color for the bikeshed?

I can live with it, although the different spelling is slightly jarring.

cheers

andrew