Thread: [HACKERS] GSoC 2017: Foreign Key Arrays

On Mon, May 22, 2017 at 7:51 PM, Mark Rofail <markm.rofail@gmail.com> wrote:
> Cloned the git repo found @ https://github.com/postgres/postgres and
> identified the main two files I will be concerned with. (I know I may need
> to edit other files but these seem to where I will spend most of my summer)
>
> src/backend/commands/tablecmds.c
> src/backend/utils/ri_triggers.c
>
> I am yet to identify the files concerned with the GIN opclass. <-- if anyone
> can help with this

There's not only one GIN opclass.  You can get a list like this:

select oid, * from pg_opclass where opcmethod = 2742;

Actually, you probably want to look for GIN opfamilies:

rhaas=# select oid, * from pg_opfamily where opfmethod = 2742;oid  | opfmethod |    opfname     | opfnamespace |
opfowner
------+-----------+----------------+--------------+----------2745 |      2742 | array_ops      |           11 |
103659|      2742 | tsvector_ops   |           11 |       104036 |      2742 | jsonb_ops      |           11 |
104037|      2742 | jsonb_path_ops |           11 |       10
 
(4 rows)

To see which SQL functions are used to implement a particular
opfamily, use the OID from the previous step in a query like this:

rhaas=# select prosrc from pg_amop, pg_operator, pg_proc where
amopfamily = 2745 and amopopr = pg_operator.oid and oprcode =
pg_proc.oid;    prosrc
----------------array_eqarrayoverlaparraycontainsarraycontained
(4 rows)

Then, you can look for those in the source tree.  You can also search
for the associated support functions, e.g.:

rhaas=# select distinct amprocnum, prosrc from pg_amproc, pg_proc
where amprocfamily = 2745 and amproc = pg_proc.oid order by 1, 2;amprocnum |        prosrc
-----------+-----------------------        1 | bitcmp        1 | bpcharcmp        1 | btabstimecmp        1 | btboolcmp
      1 | btcharcmp        1 | btfloat4cmp        1 | btfloat8cmp        1 | btint2cmp        1 | btint4cmp        1 |
btint8cmp       1 | btnamecmp        1 | btoidcmp        1 | btoidvectorcmp        1 | btreltimecmp        1 |
bttextcmp       1 | bttintervalcmp        1 | byteacmp        1 | cash_cmp        1 | date_cmp        1 | interval_cmp
     1 | macaddr_cmp        1 | network_cmp        1 | numeric_cmp        1 | time_cmp        1 | timestamp_cmp
1| timetz_cmp        2 | ginarrayextract        3 | ginqueryarrayextract        4 | ginarrayconsistent        6 |
ginarraytriconsistent
(30 rows)

You might want to read https://www.postgresql.org/docs/devel/static/xindex.html

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

• After finding the arraycontains function, I implemented
arraycontainselem that corresponds to the operator @<(anyarray,
anyelem)
       ◦ Please read the attached patch file to view my progress.

•  In addition to src/backend/utils/adt/arrayfuncs.c where I
implemented arraycontainselem.

       ◦ I also edited pg_amop (src/include/catalog/pg_amop.h) since
it stores information about operators associated with access method
operator families.

+DATA(insert ( 2745   2277 2283 2 s 2753 2742 0 ));
{
2745: Oid amopfamily; (denotes gin array_ops)
277: Oid amoplefttype; (denotes anyaray)
2283: Oid amoprighttype; (denotes anyelem)
5: int16 amopstrategy; /* operator strategy number */ (denotes the new
startegy that is yet to be created)
's': char amoppurpose; (denotes 's' for search)
2753: Oid amopopr; (denotes the new operator Oid)
2742: Oid amopmethod;(denotes gin)
0: Oid amopsortfamily; (0 since search operator)
}

       ◦ And pg_operator (src/include/catalog/pg_operator.h) since it
stores information about operators.
+DATA(insert OID = 2753 (  "@>"   PGNSP PGUID b f f 2277 2283 16 0  0
arraycontainselem 0 0 ));
{
 "@>": NameData oprname; /* name of operator */
Oid oprnamespace; /* OID of namespace containing this oper */
Oid oprowner; /* operator owner */
'b': char oprkind; /* 'l', 'r', or 'b' */ (denotes infix)
'f': bool oprcanmerge; /* can be used in merge join? */
'f': bool oprcanhash; /* can be used in hash join? */
277: Oid oprleft; (denotes anyaray)
2283: Oid oprright; (denotes anyelem)
16: Oid oprresult;  (denotes boolean)
0: Oid oprcom; /* OID of commutator oper, or 0 if none */ (needs to be
revisited)
0: Oid oprnegate; /* OID of negator oper, or 0 if none */ (needs to be
revisited)
arraycontainselem: regproc oprcode; /* OID of underlying function */
0: regproc oprrest; /* OID of restriction estimator, or 0 */
0: regproc oprjoin; /* OID of join estimator, or 0 */
}

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

I was idly following along in GSoC 2017: Foreign Key Arrays
when I noticed this:

=# select * from pg_amproc where amprocfamily = 2745;amprocfamily | amproclefttype | amprocrighttype | amprocnum |
amproc
--------------+----------------+-----------------+-----------+        2745 |           2277 |            2277 |
2|
 
pg_catalog.ginarrayextract        2745 |           2277 |            2277 |         3 |
ginqueryarrayextract
...

where only ginarrayextract is schema-qualified. It seems to be
regproc's output procedure doing it:

=# select 2743::regproc, 2774::regproc;         regproc           |       regproc
----------------------------+----------------------pg_catalog.ginarrayextract | ginqueryarrayextract


The manual says regproc "will display schema-qualified names on output
if the object would not be found in the current search path without
being qualified."

Is regproc displaying the schema in this case because there are two
overloaded flavors of ginarrayextract, though both are in pg_catalog?
Could it be searching for the object by name, ignoring the argument
signature, and just detecting that it hit one with a different OID first?

-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> The manual says regproc "will display schema-qualified names on output
> if the object would not be found in the current search path without
> being qualified."

That's less than the full truth :-(

> Is regproc displaying the schema in this case because there are two
> overloaded flavors of ginarrayextract, though both are in pg_catalog?

Yes, see the test in regprocout:
            * Would this proc be found (uniquely!) by regprocin? If not,            * qualify it.

Of course, in a situation like this, schema-qualification is not enough to
save the day; regprocin will still fail because the name is ambiguous.
You really need to use regprocedure not regproc if you want any guarantees
about the results.

(The fact that we have regproc at all is a bit of a historical accident,
caused by some limitations of the bootstrap mode.)
        regards, tom lane

Mark Rofail wrote:
> Okay, so major breakthrough.
> 
> *Updates:*
> 
>    - The operator @>(anyarray, anyelement) is now functional
>       - The segmentation fault was due to applying PG_FREE_IF_COPY on a
>       datum when it should only be applied on TOASTed inputs
>       - The only problem now is if for example you apply the operator as
>       follows  '{AAAAAAAAAA646'}' @> 'AAAAAAAAAA646' it maps to @>(anyarray,
>       anyarray) since 'AAAAAAAAAA646' is interpreted as char[] instead of Text
>    - Added some regression tests (src/test/regress/sql/arrays.sql) and
>    their results(src/test/regress/expected/arrays.out)
>    - wokred on the new GIN strategy, I don't think it would vary much from
>    GinContainsStrategy.

OK, that's great.

> *What I plan to do:*
> 
>    - I need to start working on the Referential Integrity code but I don't
>    where to start

You need to study the old patch posted by Marco Nenciarini.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:
> On Mon, Jun 26, 2017 at 6:44 PM, Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> 
> > Have you met any particular problem here?  Or is it just a lot of
> > mechanical work?
> >
> 
> Just A LOT of mechanictal work, thankfully. The patch is now rebased and
> all regress tests have passed (even the element_foreign_key). Please find
> the patch below !

Great!

> *What I plan to do next *
> 
>    - study ri_triggers.c (src/backend/utils/adt/ri_triggers.c) since this
>    is where the new RI code will reside

Any news?


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:

>    - now the RI checks utilise the @>(anyarray, anyelement)
>       - however there's a small problem:
>       operator does not exist: integer[] @> smallint
>       I assume that external casting would be required here. But how can I
>       downcast smallint to integer or interger to numeric automatically ?

We have one opclass for each type combination -- int4 to int2, int4 to
int4, int4 to int8, etc.  You just need to add the new strategy to all
the opclasses.

BTW now that we've gone through this a little further, it's starting to
look like a mistake to me to use the same @> operator for (anyarray,
anyelement) than we use for (anyarray, anyarray).  I have the feeling
we'd do better by having some other operator for this purpose -- dunno,
maybe @>> or @>.  ... whatever you think is reasonable and not already
in use.  Unless there is some other reason to pick @> for this purpose.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:
> On Wed, Jul 12, 2017 at 2:30 PM, Mark Rofail <markm.rofail@gmail.com> wrote:
> 
> > On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera <alvherre@2ndquadrant.com
> > > wrote:
> >>
> >> We have one opclass for each type combination -- int4 to int2, int4 to
> >> int4, int4 to int8, etc.  You just need to add the new strategy to all
> >> the opclasses.
> >>
> >
> > Can you clarify this solution ? I think another solution would be external
> > casting
> >
> If external casting is to be used. If for example the two types in
> question are smallint and integer. Would a function get_common_type(Oid
> leftopr, Oid rightopr) be useful ?, that given the two types return the
> "common" type between the two in this case integer.

Do you mean adding cast decorators to the query constructed by
ri_triggers.c?  That looks like an inferior solution.  What problem do
you see with adding more rows to the opclass?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:
> On Tue, 18 Jul 2017 at 7:43 pm, Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> 
> >  separate operators for int4[] @>> int8, int4[] @>> int4, int4[] @>> int2,
> > int4[] @>> numeric.
> >
> 
> My only comment on the separate operators is its high maintenance.  Any new
> datatype introduced a corresponding operator should be created.

Yes.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alexander Korotkov wrote:

> The problem is that you need to have not only opclass entries for the
> operators, but also operators themselves.  I.e. separate operators for
> int4[] @>> int8, int4[] @>> int4, int4[] @>> int2, int4[] @>> numeric.  You
> tried to add multiple pg_amop rows for single operator and consequently get
> unique index violation.
> 
> Alvaro, do you think we need to define all these operators?  I'm not sure.
> If even we need it, I think we shouldn't do this during this GSoC.  What
> particular shortcomings do you see in explicit cast in RI triggers queries?

I'm probably confused.  Why did we add an operator and not a support
procedure?  I think we should have added rows in pg_amproc, not
pg_amproc.  I'm very tired right now so I may be speaking nonsense.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jul 19, 2017 at 8:08 AM, Mark Rofail <markm.rofail@gmail.com> wrote:
> To summarise, the options we have to solve the limitation of the @>(anyarray
> , anyelement) where it produces the following error: operator does not
> exist: integer[] @> smallint

Why do we have to solve that limitation?

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

On Wed, Jul 19, 2017 at 2:29 PM, Mark Rofail <markm.rofail@gmail.com> wrote:
> On Wed, Jul 19, 2017 at 7:28 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> Why do we have to solve that limitation?
>
> Since the regress test labled element_foreing_key fails now that I made the
> RI queries utilise @(anyarray, anyelement), that means it's not functioning
> as it is meant to be.

Well, if this is a new test introduced by the patch, you could also
just change the test.  Off-hand, I'm not sure that it's very important
to make the case work where the types don't match between the
referenced table and the referencing table, which is what you seem to
be talking about here.  But maybe I'm misunderstanding the situation.

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

Mark Rofail wrote:
> On Tue, Jul 18, 2017 at 11:14 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
> >
> > Why did we add an operator and not a support
> > procedure?
> 
> I thought the support procedures were constant within an opclass.

Uhh ... I apologize but I think I was barking at the wrong tree.  I was
thinking that it mattered that the opclass mechanism was able to
determine whether some array @>> some element, but that's not true: it's
the queries in ri_triggers.c, which have no idea about opclasses.

(I tihnk we would have wanted to use to opclasses in order to find out
what operator to use in the first place, if ri_triggers.c was already
using that general idea; but in reality it's already using hardcoded
operator names, so it doesn't matter.)

I'm not entirely sure what's the best way to deal with the polymorphic
problem, but on the other hand as Robert says downthread maybe we
shouldn't be solving it at this stage anyway.  So let's step back a bit,
get a patch that works for the case where the types match on both sides
of the FK, then we review that patch; if all is well, we can discuss the
other problem as a stretch goal.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Jul 22, 2017 at 5:50 PM, Mark Rofail <markm.rofail@gmail.com> wrote:
> so personally I don't think we should leave creating a GIN index up to the
> user, it should be automatically generated instead.

I can certainly understand why you feel that way, but trying to do
that in your patch is just going to get your patch rejected.  We don't
want array foreign keys to have different behavior than regular
foreign keys, and regular foreign keys don't do this automatically.
We could change that, but I suspect it would cause us some pretty
serious problems with upgrades from older versions with the existing
behavior to newer versions with the revised behavior.

There are other problems, too.  Suppose the user creates the foreign
key and then drops the associated index; then, they run pg_dump.  Will
restoring the dump recreate the index?  If so, then you've broken
dump/restore, because now it doesn't actually recreate the original
state of the database.  You might think of fixing this by not letting
the index be dropped, but that's problematic too, because a
fairly-standard way of removing index bloat is to create a new index
with the "concurrently" flag and then drop the old one.  Another
problem entirely is that the auto-generated index will need to have an
auto-generated name, and that name might happen to conflict with the
name of some other object that already exists in the database, which
doesn't initially seem like a problem because you can just generate a
different name instead; indeed, we already do such things.  But the
thorny point is that you have to preserve whatever name you choose --
and the linkage to the array foreign key that caused it to be created
-- across a dump/restore cycle; otherwise you'll have cases where
conflicting names cause failures.  I doubt this is a comprehensive
list of things that might go wrong; it's intended as an illustrative
list, not an exhaustive one.

This is a jumbo king-sized can of worms, and even a very experienced
contributor would likely find it extremely difficult to sort all of
the problems that would result from a change in this area.

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

On 2017-07-24 23:08, Mark Rofail wrote:
> Here is the new Patch with the bug fixes and the New Patch with the 
> Index
> in place performance results.
> 
> I just want to point this out because I still can't believe the 
> numbers. In
> reference to the old patch:
> The new patch without the index suffers a 41.68% slow down, while the 
> new
> patch with the index has a 95.18% speed up!

> [elemOperatorV4.patch]

This patch doesn't apply to HEAD at the moment ( e2c8100e6072936 ).

Can you have a look?

thanks,

Erik Rijkers




patching file doc/src/sgml/ref/create_table.sgml
Hunk #1 succeeded at 816 with fuzz 3.
patching file src/backend/access/gin/ginarrayproc.c
patching file src/backend/utils/adt/arrayfuncs.c
patching file src/backend/utils/adt/ri_triggers.c
Hunk #1 FAILED at 2650.
Hunk #2 FAILED at 2694.
2 out of 2 hunks FAILED -- saving rejects to file 
src/backend/utils/adt/ri_triggers.c.rej
patching file src/include/catalog/pg_amop.h
patching file src/include/catalog/pg_operator.h
patching file src/include/catalog/pg_proc.h
patching file src/test/regress/expected/arrays.out
patching file src/test/regress/expected/opr_sanity.out
patching file src/test/regress/sql/arrays.sql

On 2017-07-24 23:31, Mark Rofail wrote:
> On Mon, Jul 24, 2017 at 11:25 PM, Erik Rijkers <er@xs4all.nl> wrote:
>> 
>> This patch doesn't apply to HEAD at the moment ( e2c8100e6072936 ).
>> 
> 
> My bad, I should have mentioned that the patch is dependant on the 
> original
> patch.
> Here is a *unified* patch that I just tested.

Thanks.  Apply is now good, but I get this error when compiling:

ELEMENT' not present in UNRESERVED_KEYWORD section of gram.y
make[4]: *** [gram.c] Error 1
make[3]: *** [parser/gram.h] Error 2
make[2]: *** [../../src/include/parser/gram.h] Error 2
make[1]: *** [all-common-recurse] Error 2
make: *** [all-src-recurse] Error 2

On 2017-07-27 02:31, Mark Rofail wrote:
> I have written some benchmark test.
> 

It would help (me at least) if you could be more explicit about what 
exactly each instance is.

Apparently there is an 'original patch': is this the original patch by 
Marco Nenciarini?
Or is it something you posted earlier?

I guess it could be distilled from the earlier posts but when I looked 
those over yesterday evening I still didn't get it.

A link to the post where the 'original patch' is would be ideal...

thanks!

Erik Rijkers


> With two tables a PK table with 5 rows and an FK table with growing row
> count.



> 
> Once triggering an RI check
> at 10 rows,
> 100 rows,
> 1,000 rows,
> 10,000 rows,
> 100,000 rows and
> 1,000,000 rows
> 
> Please find the graph with the findings attached below
> 
> 
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-07-27 21:08, Mark Rofail wrote:
> On Thu, Jul 27, 2017 at 7:15 PM, Erik Rijkers <er@xs4all.nl> wrote:
> 
>> It would help (me at least) if you could be more explicit about what
>> exactly each instance is.
>> 
> 
> I apologize, I thought it was clear through the context.

Thanks a lot.  It's just really easy for testers like me that aren't 
following a thread too closely and just snatch a half hour here and 
there to look into a feature/patch.


One small thing while building docs:

$  cd doc/src/sgml && make html
osx -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -x lower 
postgres.sgml >postgres.xml.tmp
osx:ref/create_table.sgml:960:100:E: document type does not allow 
element "VARLISTENTRY" here
Makefile:147: recipe for target 'postgres.xml' failed
make: *** [postgres.xml] Error 1

(Debian 8/jessie)


thanks,


Erik Rijkers

Mark Rofail wrote:
> These are limitations of the patch ordered by importance:
> 
> ✗ presupposes that count(distinct y) has exactly the same notion of
> equality that the PK unique index has. In reality, count(distinct) will
> fall back to the default btree opclass for the array element type.

Operators are classified in operator classes; each data type may have
more than one operator class for a particular access method.  Exactly
one operator class for some access method can be designated as the
default one for a type.  However, when you create an index, you can
indicate which operator class to use, and it may not be the default one.
If a different one is chosen at index creation time, then a query using
COUNT(distinct) will do the wrong thing, because DISTINCT will select
an equality type using the type's default operator class, not the
equality that belongs to the operator class used to create the index.

That's wrong: DISTINCT should use the equality operator that corresponds
to the index' operator class instead, not the default one.

I hope that made sense.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> ...  However, when you create an index, you can
> indicate which operator class to use, and it may not be the default one.
> If a different one is chosen at index creation time, then a query using
> COUNT(distinct) will do the wrong thing, because DISTINCT will select
> an equality type using the type's default operator class, not the
> equality that belongs to the operator class used to create the index.

> That's wrong: DISTINCT should use the equality operator that corresponds
> to the index' operator class instead, not the default one.

Uh, what?  Surely the semantics of count(distinct x) *must not* vary
depending on what indexes happen to be available.

I think what you meant to say is that the planner may only choose an
optimization of this sort when the index's opclass matches the one
DISTINCT will use, ie the default for the data type.
        regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > ...  However, when you create an index, you can
> > indicate which operator class to use, and it may not be the default one.
> > If a different one is chosen at index creation time, then a query using
> > COUNT(distinct) will do the wrong thing, because DISTINCT will select
> > an equality type using the type's default operator class, not the
> > equality that belongs to the operator class used to create the index.
> 
> > That's wrong: DISTINCT should use the equality operator that corresponds
> > to the index' operator class instead, not the default one.
> 
> Uh, what?  Surely the semantics of count(distinct x) *must not* vary
> depending on what indexes happen to be available.

Err ...

> I think what you meant to say is that the planner may only choose an
> optimization of this sort when the index's opclass matches the one
> DISTINCT will use, ie the default for the data type.

Um, yeah, absolutely.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Mon, Aug 14, 2017 at 2:09 PM, Mark Rofail <markm.rofail@gmail.com> wrote:
>> I think we should cast the operands in the RI queries fired as follows
>> 1. we get the array type from the right operand
>> 2. compare the two array type and see which type is more "general" (as to
>> which should be cast to which, int2 should be cast to int4, since casting
>> int4 to int2 could lead to data loss). This can be done by seeing which Oid
>> is larger numerically since, coincidentally, they are declared in this way
>> in pg_type.h.

> I'm not sure numerical comparison of Oids is a good idea.

I absolutely, positively guarantee that a patch written that way will be
rejected.

> Should we instead use logic similar to select_common_type() and underlying
> functions?

Right.  What we typically do in cases like this is check to see if there
is an implicit coercion available in one direction but not the other.
I don't know if you can use select_common_type() directly, but it would
be worth looking at.

Also, given that the context here is RI constraints, what you're really
worried about is whether the referenced column's uniqueness constraint
is associated with compatible operators, so looking into its operator
class for relevant operators might be the right way to think about it.
I wrote something just very recently that touches on that ... ah,
here it is:
https://www.postgresql.org/message-id/13220.1502376894@sss.pgh.pa.us
        regards, tom lane

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]       rightOid;       ^~~~~~~~

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

Andreas


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

Sorry for the very late review.

I like this feature and have needed it myself in the past, and the 
current syntax seems pretty good. One could argue for if the syntax 
could be generalized to support other things like json and hstore, but I 
do not think it would be fair to block this patch due to that.

== Limitations of the current design

1) Array element foreign keys can only be specified at the table level 
(not at columns): I think this limitation is fine. Other PostgreSQL 
specific features like exclusion contraints can also only be specified 
at the table level.

2) Lack of support for SET NULL and SET DEFAULT: these do not seem very 
useful for arrays.

3) Lack of support for specifiying multiple arrays in the foreign key: 
seems like a good thing to me since it is not obvious what such a thing 
even would do.

4) That you need to add a cast to the index if you have different types: 
due to there not being a int4[] <@ int2[] operator you need to add an 
index on (col::int4[]) to speed up deletes and updates. This one i 
annoying since EXPLAIN wont give you the query plans for the foreign key 
queries, but I do not think fixing this should be within the scope of 
the patch and that having a smaller interger in the referring table is rare.

5) The use of count(DISTINCT) requiring types to support btree equality: 
this has been discussed a lot up-thread and I think the current state is 
good enough.

== Functional review

I have played around some with it and things seem to work and the test 
suite passes, but I noticed a couple of strange behaviors.

1) MATCH FULL does not seem to care about NULLS in arrays. In the 
example below I expected both inserts into the referring table to fail.

CREATE TABLE t (x int, y int, PRIMARY KEY (x, y));
CREATE TABLE fk (x int, ys int[], FOREIGN KEY (x, EACH ELEMENT OF ys) 
REFERENCES t MATCH FULL);
INSERT INTO t VALUES (10, 1);
INSERT INTO fk VALUES (10, '{1,NULL}');
INSERT INTO fk VALUES (NULL, '{1}');

CREATE TABLE
CREATE TABLE
INSERT 0 1
INSERT 0 1
ERROR:  insert or update on table "fk" violates foreign key constraint 
"fk_x_fkey"
DETAIL:  MATCH FULL does not allow mixing of null and nonnull key values.

2) To me it was not obvious that ON DELETE CASCADE would delete the 
whole rows rather than delete the members from the array, and this kind 
of misunderstanding can lead to pretty bad surprises in production. I am 
leaning towards not supporting CASCADE.

== The @>> operator

A previous version of your patch added the "anyelement <<@ anyarray" 
operator to avoid having to build arrays, but that part was reverted due 
to a bug.

I am not expert on the gin code, but as far as I can tell it would be 
relatively simple to fix that bug. Just allocate an array of Datums of 
length one where you put the element you are searching for (or maybe a 
copy of it).

Potential issues with adding the operators:

1) Do we really want to add an operator just for array element foreign 
keys? I think this is not an issue since it seems like it should be 
useful in general. I know I have wanted it myself.

2) I am not sure, but the committers might prefer if adding the 
operators is done in a separate patch.

3) Bikeshedding about operator names. I personally think @>> is clear 
enough and as far as I know it is not used for anything else.

== Code review

The patch no longer applies to HEAD, but the conflicts are small.

I think we should be more consistent in the naming, both in code and in 
the documentation. Right now we have "array foreign keys", "element 
foreign keys", "ELEMENT foreign keys", etc.

+       /*
+        * If this is an array foreign key, we must look up the 
operators for
+        * the array element type, not the array type itself.
+        */
+       if (fkreftypes[i] != FKCONSTR_REF_PLAIN)

+           if (fkreftypes[i] != FKCONSTR_REF_PLAIN)
+           {
+               old_fktype = get_base_element_type(old_fktype);
+               /* this shouldn't happen ... */
+               if (!OidIsValid(old_fktype))
+                   elog(ERROR, "old foreign key column is not an array");
+           }

+       if (riinfo->fk_reftypes[i] != FKCONSTR_REF_PLAIN)
+       {
+           riinfo->has_array = true;
+           riinfo->ff_eq_oprs[i] = ARRAY_EQ_OP;
+       }

In the three diffs above it would be much cleaner to check for "== 
FKCONSTR_REF_EACH_ELEMENT" since that better conveys the intent and is 
safer for adding new types in the future.

+           /* We look through any domain here */
+           fktype = get_base_element_type(fktype);

What does the comment above mean?
        if (!(OidIsValid(pfeqop) && OidIsValid(ffeqop)))            ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
-                    errmsg("foreign key constraint \"%s\" "
-                           "cannot be implemented",
-                           fkconstraint->conname),
-                    errdetail("Key columns \"%s\" and \"%s\" "
-                              "are of incompatible types: %s and %s.",
+               errmsg("foreign key constraint \"%s\" cannot be 
implemented",
+                      fkconstraint->conname),
+                    errdetail("Key columns \"%s\" and \"%s\" are of 
incompatible types: %s and %s.",                               strVal(list_nth(fkconstraint->fk_attrs, i)),
                 strVal(list_nth(fkconstraint->pk_attrs, i)),
 
-                              format_type_be(fktype),
+                              format_type_be(fktypoid[i]),                               format_type_be(pktype))));

The above diff looks like pointless code churn which possibly introduces 
a bug in how it changed fktype to fktypoid[i].

I think the code in RI_Initial_Check() would be cleaner if you used 
"CROSS JOIN LATERAL unnest(col)" rather than having unnest() in the 
target list. This way you would not need to rename all columns and the 
code paths for the array case could look more like the code path for the 
normal case.

== Minor things in the code

+           {
+               pk_attrs = lappend(pk_attrs, arg);
+               fk_reftypes = lappend_int(fk_reftypes, FKCONSTR_REF_PLAIN);
+           }

This is incorrectly indented.

I suggest that you should only allocate countbuf in RI_FKey_check() if 
has_array is set.

I think the code would be more readable if both branches of 
ri_GenerateQual() used the same pattern for whitespace so the 
differences are easier to spot.

You can use DROP TABLE IF EXISTS to silence the "ERROR:  table 
"fktableforarray" does not exist" errors.

I am not sure that you need to test all combinations of ON UPDATE and ON 
DELETE. I think it should be enough to test one of each ON UPDATE and 
one of each ON DELETE should be enough.

+-- Create the foreign table

It is probably a bad idea to call the referencing table the foreign table.

+-- Repeat a similar test using INT4 keys coerced from INT2

This comment is repeated twice in the test suite.

== Documentation

Remove the ELEMENT REFERENCES form from 
doc/src/sgml/ref/create_table.sgml since we no longer support it.

-  FOREIGN KEY ( <replaceable 
class="parameter">column_name</replaceable> [, ... ] ) REFERENCES 
<replaceable class="parameter">reftable</replaceable> [ ( <replaceable 
class="parameter">refcolumn</replaceable> [, ... ] ) ]
+  FOREIGN KEY ( [ELEMENT] <replaceable 
class="parameter">column_name</replaceable> [, ... ] ) REFERENCES 
<replaceable class="parameter">reftable</replaceable> [ ( <replaceable 
class="parameter">refcolumn</replaceable> [, ... ] ) ]

Change this to "FOREIGN KEY ( [EACH ELEMENT OF] ...".

-   <term><literal>FOREIGN KEY ( <replaceable 
class="parameter">column_name</replaceable> [, ... ] )
+   <term><literal>FOREIGN KEY ( [ELEMENT] <replaceable 
class="parameter">column_name</replaceable> [, ... ] )

Change this to "... FOREIGN KEY ( [EACH ELEMENT OF] ...".

+        <literal>ELEMENT</literal> keyword and <replaceable

Change this to "... <literal>EACH ELEMENT OF</literal> ...". Maybe you 
should also fix other instances of ELEMENT in the same paragraph but 
these are less obvious.

You should remove the "ELEMENT REFERENCES" section of 
doc/src/sgml/ref/create_table.sgml, and move any still relevant bits 
elsewhere since we do not support this syntax.

The sql-createtable-foreign-key-arrays section need to be updated to 
remove references to "ELEMENT REFERENCES".

Your indentation in doc/src/sgml/catalogs.sgml is two spaces but the 
rest of the file looks like it uses 1 space indentation. Additionally 
you seem to have accidentally reindented something which was correctly 
indented.

Same with the idnentation in doc/src/sgml/ddl.sgml and 
doc/src/sgml/ref/create_table.sgml.

-   <varlistentry>
+  <varlistentry id="sql-createtable-foreign-key" xreflabel="FOREIGN KEY">

You have accidentally reindented this in doc/src/sgml/ref/create_table.sgml.

The paragraph in doc/src/sgml/ref/create_table.sgml which starts with 
"In case the column name" seems to actually be multiple paragraphs. Is 
that intentional or a mistake?

The documentation in doc/src/sgml/ddl.sgml mentions that "it must be 
written in table constraint form" for when you have multiple columns, 
but I feel this is just redundant and confusing since this is always 
true, both for array foreign keys and for when you have multiple columns.

The documentation in doc/src/sgml/ddl.sgml should mention that we only 
support one array reference per foreign key.

Maybe the documentation in doc/src/sgml/ddl.sgml should mention that we 
only support the table constraint form.

Andreas


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

On 11/10/2017 01:47 AM, Mark Rofail wrote:
> I am sorry for the late reply

There is no reason for you to be. It did not take you 6 weeks to do a 
review. :) Thanks for this new version.

> == Functional review
> 
>      >1) MATCH FULL does not seem to care about NULLS in arrays. In the
>     example below I expected both inserts into the referring table to fail.
> 
> 
> It seems in your example the only failed case was: INSERT INTO fk VALUES 
> (NULL, '{1}');
> which shouldn't work, can you clarify this?

I think that if you use MATH FULL the query should fail if you have a 
NULL in the array.

>      >2) To me it was not obvious that ON DELETE CASCADE would delete
>     the whole rows rather than delete the members from the array, and
>     this kind of misunderstanding can lead to pretty bad surprises in
>     production. I am leaning towards not supporting CASCADE.
> 
> 
> I would say so too, maybe we should remove ON DELETE CASCADE until we 
> have supported all remaining actions.

I am leaning towards this too. I would personally be fine with a first 
version without support for CASCADE since it is not obvious to me what 
CASCADE should do.

> == The @>> operator
> I would argue that allocating an array of datums and building an array 
> would have the same complexity

I am not sure what you mean here. Just because something has the same 
complexity does not mean there can't be major performance differences.

> == Code review
> 
>      >I think the code in RI_Initial_Check() would be cleaner if you
>     used "CROSS JOIN LATERAL unnest(col)" rather than having unnest() in
>     the target list. This way you would not need to rename all columns
>     and the code paths for the array case could look more like the code
>     path for the normal case.
> 
> Can you clarify what you mean a bit more?

I think the code would look cleaner if you generate the following query:

SELECT fk.x, fk.ys FROM ONLY t2 fk CROSS JOIN LATERAL 
pg_catalog.unnest(ys) a2 (v) LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.x 
AND pk.y = a2.v WHERE [...]

rather than:

SELECT fk.k1, fk.ak2 FROM (SELECT x k1, pg_catalog.unnest(ys) k2, ys ak2 
FROM ONLY t2) fk LEFT OUTER JOIN ONLY t1 pk ON pk.x = fk.k1 AND pk.y = 
fk.k2 WHERE [...]

= New stuff

When applying the patch I got some white space warnings:

Array-ELEMENT-foreign-key-v5.3.patch:1343: space before tab in indent, 
indent with spaces.                 format_type_be(oprleft), format_type_be(oprright))));
Array-ELEMENT-foreign-key-v5.3.patch:1345: trailing whitespace.

When compiling I got an error:

ri_triggers.c: In function ‘ri_GenerateQual’:
ri_triggers.c:2693:19: error: unknown type name ‘d’   Oid   oprcommon;d                   ^
ri_triggers.c:2700:3: error: conflicting types for ‘oprright’   oprright = get_array_type(operform->oprleft);
^~~~~~~~
ri_triggers.c:2691:9: note: previous declaration of ‘oprright’ was here   Oid   oprright;         ^~~~~~~~
<builtin>: recipe for target 'ri_triggers.o' failed

When building the documentation I got two warnings:

/usr/bin/osx:catalogs.sgml:2349:17:W: empty end-tag
/usr/bin/osx:catalogs.sgml:2350:17:W: empty end-tag

When running the tests I got a failure in element_foreign_key.

Andreas


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

On 11/13/2017 12:32 PM, Mark Rofail wrote:
>         == The @>> operator
>         I would argue that allocating an array of datums and building an
>         array would have the same complexity
> 
> 
>     I am not sure what you mean here. Just because something has the
>     same complexity does not mean there can't be major performance
>     differences.
> 
> I have spend a lot of time working on this operator and would like to 
> benefit from it. How should I go about this ? Start a new patch ?

I am still not sure what you mean here. Feel free to add @>> to this 
patch if you like. You may want to submit it as two patch files (but 
please keep them as the same commitfest entry) but I leave that decision 
all up to you.
> So the two main issues we remain to resolve are MATCH FULL and the 
> RI_Initial_Check() query refactoring. The problem is that I am not one 
> of the original authors and have not touched this part of the code.
> I understand the problem but it will take some time for me to understand 
> how to resolve everything.

Cool, feel free to ask if you need some assistance. I want this patch.

Andreas

On Mon, Nov 27, 2017 at 10:47 AM, Andreas Karlsson <andreas@proxel.se> wrote:
> Cool, feel free to ask if you need some assistance. I want this patch.

The last patch submitted did not get a review, and it does not apply
as well. So I am moving it to next CF with waiting on author as
status. Please rebase the patch.
-- 
Michael

Here's a rebased version of this patch.  I have done nothing other than
fix the conflicts and verify that it passes existing regression tests.
I intend to go over the reviews sometime later and hopefully get it all
fixed for inclusion in pg11.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:

> > 1) MATCH FULL does not seem to care about NULLS in arrays. In the example
> > below I expected both inserts into the referring table to fail.
> >
> > CREATE TABLE t (x int, y int, PRIMARY KEY (x, y));
> > CREATE TABLE fk (x int, ys int[], FOREIGN KEY (x, EACH ELEMENT OF ys) REFERENCES t MATCH FULL);
> > INSERT INTO t VALUES (10, 1);
> > INSERT INTO fk VALUES (10, '{1,NULL}');
> > INSERT INTO fk VALUES (NULL, '{1}');
> >
> I understand that Match full should contain nulls in the results. However,
> I don't think that it's semantically correct, so I suggest we don't use
> Match full. What would be the consequences of that ?

Well, I think you could get away with not supporting MATCH FULL with
array FK references (meaning you ought to raise an error if you see it) ...
clearly EACH ELEMENT OF is an extension of the spec so we're not forced
to comply with all the clauses.  On the other hand, it would be better if it
can be made to work.

If I understand correctly, you would need a new operator similar to @>
but which rejects NULLs in order to implement MATCH FULL, right?

> > 2) I think the code in RI_Initial_Check() would be cleaner if you used
> > "CROSS JOIN LATERAL unnest(col)" rather than having unnest() in the target
> > list. This way you would not need to rename all columns and the code paths
> > for the array case could look more like the code path for the normal case.
> >
> I have repeatedly tried to generate the suggested query using C code and I
> failed. I would like some help with it

Well, the way to go about it would be to first figure out what is the
correct SQL query, and only later try to implement it in C.  Is SQL the
problem, or is it C?  I'm sure we can get in touch with somebody that
knows a little bit of SQL.  Can you do a write-up of the query requirements?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/21/2018 10:36 PM, Mark Rofail wrote:
>     == The @>> operator
> 
>     A previous version of your patch added the "anyelement <<@ anyarray"
>     operator to avoid having to build arrays, but that part was reverted
>     due to a bug.
> 
>     I am not expert on the gin code, but as far as I can tell it would
>     be relatively simple to fix that bug. Just allocate an array of
>     Datums of length one where you put the element you are searching for
>     (or maybe a copy of it).
> 
> The @>> is now restored and functioning correctly, all issues with 
> contrib libraries has been resolved

Hi,

I looked some at your anyarray @>> anyelement code and sadly it does not 
look like the index code could work. The issue I see is that 
ginqueryarrayextract() needs to make a copy of the search key but to do 
so it needs to know the type of anyelement (to know if it needs to 
detoast, etc). But there is as far as I can tell no way to check the 
type of anyelement in this context.

Am I missing something?

Andreas

On 02/01/2018 04:17 PM, Mark Rofail wrote:
> since its a polymorphic function it only passes if the `anyarray` is the 
> same type of the `anyelement` so we are sure they are the same type. 
> Can't we get the type from the anyarray ? the type is already stored in 
> ` arr_type`.

In theory, yes, but I saw no obvious way to get it without major changes 
to the code. Unless I am missing something, of course.

If I am right my recommendation for getting this patch in is to 
initially skip the new operators and go back to the version of the patch 
which uses @>.

Andreas

The patch looks good to me now other than some smaller issues, mostly in 
the documentation. If you fix these I will set it as ready for 
committer, and let a committer chime in on the casting logic which we 
both find a bit ugly.

== Comments

The only a bit bigger issue I see is the error messages. Can they be 
improved?

For example:

CREATE TABLE t1 (a int, b int, PrIMARY KEY (a, b));
CREATE TABLE t2 (a int, bs int8[], FOREIGN KEY (a, EACH ELEMENT OF bs) 
REFERENCES t1 (a, b));
INSERT INTO t2 VALUES (0, ARRAY[1, 2]);

Results in:

ERROR:  insert or update on table "t2" violates foreign key constraint 
"t2_a_fkey"
DETAIL:  Key (a, bs)=(0, {1,2}) is not present in table "t1".

Would it be possible to make the DETAIL something like the below 
instead? Do you think my suggested error message is clear?

I am imaging something like the below as a patch. Does it look sane? The 
test cases need to be updated at least.

diff --git a/src/backend/utils/adt/ri_triggers.c 
b/src/backend/utils/adt/ri_triggers.c
index 402bde19d4..9dc7c9812c 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3041,6 +3041,10 @@ ri_ReportViolation(const RI_ConstraintInfo *riinfo,
                 appendStringInfoString(&key_names, ", ");
                 appendStringInfoString(&key_values, ", ");
             }
+
+           if (riinfo->fk_reftypes[idx] == FKCONSTR_REF_EACH_ELEMENT)
+               appendStringInfoString(&key_names, "EACH ELEMENT OF ");
+
             appendStringInfoString(&key_names, name);
             appendStringInfoString(&key_values, val);
         }


DETAIL:  Key (a, EACH ELEMENT OF bs)=(0, {1,2}) is not present in table 
"t1".

-  REFERENCES <replaceable class="parameter">reftable</replaceable> [ ( 
<replaceable class="parameter">refcolumn</replaceable> ) ] [ MATCH FULL 
| MATCH PARTIAL | MATCH SIMPLE ]
+  [EACH ELEMENT OF] REFERENCES <replaceable 
class="parameter">reftable</replaceable> [ ( <replaceable 
class="parameter">refcolumn</replaceable> ) ] [ MATCH FULL | MATCH 
PARTIAL | MATCH SIMPLE ]

I think this documentation in doc/src/sgml/ref/create_table.sgml should 
be removed since it is no longer correct.

+     <para>
+      Foreign Key Arrays are an extension
+      of PostgreSQL and are not included in the SQL standard.
+     </para>

This pargraph and some of the others you added to 
doc/src/sgml/ref/create_table.sgml are strangely line wrapped.

+       <varlistentry>
+        <term><literal>ON DELETE CASCADE</literal></term>
+        <listitem>
+         <para>
+          Same as standard foreign key constraints. Deletes the entire 
array.
+         </para>
+        </listitem>
+       </varlistentry>
+      </variablelist>

I thought this was no longer supported.

+       however, this syntax will cast ftest1 to int4 upon RI checks, 
thus defeating the
+      purpose of the index.

There is a minor indentation error on these lines.

+
+   <para>
+     Array <literal>ELEMENT</literal> foreign keys and the <literal>ELEMENT
+     REFERENCES</literal> clause are a 
<productname>PostgreSQL</productname>
+     extension.
+   </para>

We do not have any ELEMENT REFERENCES clause.

-           ereport(ERROR,
-                   (errcode(ERRCODE_DATATYPE_MISMATCH),
-                    errmsg("foreign key constraint \"%s\" "
-                           "cannot be implemented",
-                           fkconstraint->conname),
-                    errdetail("Key columns \"%s\" and \"%s\" "
-                              "are of incompatible types: %s and %s.",
-                              strVal(list_nth(fkconstraint->fk_attrs, i)),
-                              strVal(list_nth(fkconstraint->pk_attrs, i)),
-                              format_type_be(fktype),
-                              format_type_be(pktype))));
+       ereport(ERROR,
+               (errcode(ERRCODE_DATATYPE_MISMATCH),
+                errmsg("foreign key constraint \"%s\" "
+                       "cannot be implemented",
+                       fkconstraint->conname),
+                errdetail("Key columns \"%s\" and \"%s\" "
+                          "are of incompatible types: %s and %s.",
+                          strVal(list_nth(fkconstraint->fk_attrs, i)),
+                          strVal(list_nth(fkconstraint->pk_attrs, i)),
+                          format_type_be(fktype),
+                          format_type_be(pktype))));

It seems like you accidentally change the indentation here,

Andreas

On 02/06/2018 11:15 AM, Mark Rofail wrote:
> A new patch including all the fixes is ready.
> 
> Can you give the docs another look. I re-wrapped, re-indented  and 
> changed all `Foreign Key Arrays` to `Array Element Foreign Keys` for 
> consistency.

Looks good to me so set it to ready for committer. I still feel the type 
casting logic is a bit ugly but I am not sure if it can be improved much.

Andreas

Andreas Karlsson wrote:
> On 02/06/2018 11:15 AM, Mark Rofail wrote:
> > A new patch including all the fixes is ready.
> > 
> > Can you give the docs another look. I re-wrapped, re-indented  and
> > changed all `Foreign Key Arrays` to `Array Element Foreign Keys` for
> > consistency.
> 
> Looks good to me so set it to ready for committer. I still feel the type
> casting logic is a bit ugly but I am not sure if it can be improved much.

I gave this a quick look.  I searched for the new GIN operator so that I
could brush it up for commit ahead of the rest -- only to find out that
it was eviscerated from the patch between v5 and v5.1.  The explanation
for doing so is that the GIN code had some sort of bug that made it
crash; so the performance was measured to see if the GIN operator was
worth it, and the numbers are pretty confusing (the test don't seem
terribly exhaustive; some numbers go up, some go down, it doesn't appear
that the tests were run more than once for each case therefore the
numbers are pretty noisy) so the decision was to ditch all the GIN
support code anyway ..??  I didn't go any further since ISTM the GIN
operator prerequisite was there for good reasons, so we'll need to see
much more evidence that it's really unneeded before deciding to omit it.

At this point I'm not sure what to do with this patch.  It needs a lot
of further work, for which I don't have time now, and given the pressure
we're under I think we should punt it to the next dev cycle.

Here's a rebased pgindented version.  I renamed the regression test.  I
didn't touch anything otherwise.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 3/7/18 5:43 AM, Alvaro Herrera wrote:
> 
> At this point I'm not sure what to do with this patch.  It needs a lot
> of further work, for which I don't have time now, and given the pressure
> we're under I think we should punt it to the next dev cycle.
> 
> Here's a rebased pgindented version.  I renamed the regression test.  I
> didn't touch anything otherwise.

I have changed this entry to Waiting on Author in case Mark wants to
comment.  At the end of the CF it will be Returned with Feedback.

Regards,
-- 
-David
david@pgmasters.net

Hi Mark,

On 3/26/18 4:50 PM, Mark Rofail wrote:
> On 3/7/18 5:43 AM, Alvaro Herrera wrote:
> 
>     I searched for the new GIN operator so that I
>     could brush it up for commit ahead of the rest -- only to find out that
>     it was eviscerated from the patch between v5 and v5.1.
> 
> The latest version of the patch which contained the new GIN operator is
> version  `*Array-ELEMENT-foreign-key-v6.patch
> <https://www.postgresql.org/message-id/attachment/58007/Array-ELEMENT-foreign-key-v6.patch>*`
> which works fine and passed all the regression tests at the time
> (2018-01-21). We abandoned the GIN operator since it couldn't follow the
> same logic as the rest of GIN operators use since it operates on a Datum
> not an array. Not because of any error.
> 
> just as Andreas said
> 
> On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> 
>      The issue I see is that 
>     ginqueryarrayextract() needs to make a copy of the search key but to do 
>     so it needs to know the type of anyelement (to know if it needs to 
>     detoast, etc). But there is as far as I can tell no way to check the 
>     type of anyelement in this context.
> 
>  
> If there is any way to  obtain a copy of the datum I would be more than
> happy to integrate the GIN operator to the patch.

Since you have expressed a willingness to continue work on this patch I
have moved it to the next CF in Waiting on Author state.

You should produce a new version by then that addresses Alvaro's
concerns and I imagine that will require quite a bit of discussion and
work.  Everyone is a bit fatigued at the moment so it would best to hold
off on that for a while.

Regards,
-- 
-David
david@pgmasters.net

Mark Rofail wrote:
> Hello David,
> 
> On Tue, Apr 10, 2018 at 3:47 PM, David Steele <david@pgmasters.net> wrote:
> 
> > You should produce a new version by then that addresses Alvaro's
> > concerns and I imagine that will require quite a bit of discussion and
> > work.
> 
> I'll get working.
> I'll produce a patch with two alternate versions, one with and one without
> the GIN operators.

I'd rather see one patch with just the GIN operator (you may think it's
a very small patch, but that's only because you forget to add
documentation to it and a few extensive tests to ensure it works well);
then the array-fk stuff as a follow-on patch.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:
> On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> >
> > documentation to it and a few extensive tests to ensure it works well);
> 
> I think the existing regression tests verify that the patch works as
> expectations, correct?

I meant for the GIN operator. (Remember, these are two patches, and each
of them needs its own tests.)

> We need more *exhaustive* tests to test performance, not functionality.

True.  So my impression from the numbers you posted last time is that
you need to run each measurement case several times, and provide
averages/ stddevs/etc for the resulting numbers, and see about outliers
(maybe throw them away, or maybe they indicate some problem in the test
or in the code); then we can make an informed decision about whether the
variations between the several different scenarios are real improvements
(or pessimizations) or just measurement noise.

In particular: it seemed to me that you decided to throw away the idea
of the new GIN operator without sufficient evidence that it was
unnecessary.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Mark Rofail wrote:

> > In particular: it seemed to me that you decided to throw away the idea
> > of the new GIN operator without sufficient evidence that it was
> > unnecessary.
> 
> I have to admit to that. But in my defence @> is also GIN indexable so the
> only difference in performance between 'anyarray @>> anyelement' and
> 'anyarray @> ARRAY [anyelement]' is the delay caused by the ARRAY[]
> operation theoretically.

I think I need to review Tom's bounce-for-rework email
https://postgr.es/m/28389.1351094795@sss.pgh.pa.us
to respond to this intelligently.  Tom mentioned @> there but there was
a comment about the comparison semantics used by that operator, so I'm
unclear on whether or not that issue has been fixed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Aug 11, 2018 at 05:20:57AM +0200, Mark Rofail wrote:
> I am still having problems rebasing this patch. I can not figure it out on
> my own.

Okay, it's been a couple of months since this last email, and nothing
has happened, so I am marking it as returned with feedback.
--
Michael

On 2021-Jan-24, Mark Rofail wrote:

> I do not think that postgres contains vector operators, correct me if I am
> wrong. I feel that supporting vectors is our of the scope of this patch, if
> you have an idea how to support it please let me know.

I do not think that this patch needs to support oidvector and int2vector
types as if they were oid[] and int2[] respectively.  If you really want
an element FK there, you can alter the column types to the real array
types in your catalog clone.  oidvector and int2vector are internal
implementation artifacts.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."                              (Brian Kernighan)

I'll continue testing over the next couple of days and report if I find any more oddities.