Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Hi Stefan,

On 2021-Oct-03, Stefan Keller wrote:

> Just for my understanding - and perhaps as input for the documentation of 
> this:
> Are Foreign Key Arrays a means to implement "Generic Foreign Keys" as
> in Oracle [1] and Django [2], and of "Polymorphic Associations" as
> they call this in Ruby on Rails?

No -- at least as far as I was able to understand the pages you linked to.

It's intended for array elements of one column to reference values in a
scalar column.  These are always specific columns, not "generic" or
"polymorphic" (which I understand to mean one of several possible
columns).

Thanks,

-- 
Álvaro Herrera   39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Stefan Keller]

Dear all

Just for my understanding - and perhaps as input for the documentation of this:
Are Foreign Key Arrays a means to implement "Generic Foreign Keys" as
in Oracle [1] and Django [2], and of "Polymorphic Associations" as
they call this in Ruby on Rails?

Yours, Stefan

[1] Steven Feuerstein and Debby Russell (1995): Oracle Extension
"PLVfk - Generic Foreign Key Lookups" in: "Advanced Oracle Pl/Sql:
Programming With Packages" (Nutshell Handbook), Oreilly, ISBN
B6AVR6. Webaccess: https://flylib.com/books/en/2.408.1.159/1/
[2] Stackoverflow:
https://stackoverflow.com/questions/14333460/django-generic-foreign-keys-good-or-bad-considering-the-sql-performance

Am Di., 14. Sept. 2021 um 21:00 Uhr schrieb Mark Rofail
:
>
> Dear Alvaro,
>
> We just need to rewrite the scope of the patch so I work on the next 
> generation. I do not know what was "out of scope" in the current version
>
> /Mark
>
> On Tue, 14 Sep 2021, 8:55 pm Alvaro Herrera,  wrote:
>>
>> On 2021-Sep-14, Daniel Gustafsson wrote:
>>
>> > Given the above, and that nothing has happened on this thread since this 
>> > note,
>> > I think we should mark this Returned with Feedback and await a new 
>> > submission.
>> > Does that seem reasonable Alvaro?
>>
>> It seems reasonable to me.
>>
>> --
>> Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Dear Alvaro,

We just need to rewrite the scope of the patch so I work on the next
generation. I do not know what was "out of scope" in the current version

/Mark

On Tue, 14 Sep 2021, 8:55 pm Alvaro Herrera, 
wrote:

> On 2021-Sep-14, Daniel Gustafsson wrote:
>
> > Given the above, and that nothing has happened on this thread since this
> note,
> > I think we should mark this Returned with Feedback and await a new
> submission.
> > Does that seem reasonable Alvaro?
>
> It seems reasonable to me.
>
> --
> Álvaro Herrera  Valdivia, Chile  —
> https://www.EnterpriseDB.com/
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Daniel Gustafsson]

> On 14 Sep 2021, at 20:54, Alvaro Herrera  wrote:
> 
> On 2021-Sep-14, Daniel Gustafsson wrote:
> 
>> Given the above, and that nothing has happened on this thread since this 
>> note,
>> I think we should mark this Returned with Feedback and await a new 
>> submission.
>> Does that seem reasonable Alvaro?
> 
> It seems reasonable to me.

Thanks, done that way now.

--
Daniel Gustafsson   https://vmware.com/

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Sep-14, Daniel Gustafsson wrote:

> Given the above, and that nothing has happened on this thread since this note,
> I think we should mark this Returned with Feedback and await a new submission.
> Does that seem reasonable Alvaro?

It seems reasonable to me.

-- 
Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Daniel Gustafsson]

> On 14 Jul 2021, at 18:07, Alvaro Herrera  wrote:
> 
> On 2021-Jul-14, vignesh C wrote:
> 
>> The patch does not apply on Head anymore, could you rebase and post a
>> patch. I'm changing the status to "Waiting for Author".
> 
> BTW I gave a look at this patch in the March commitfest and concluded it
> still requires some major surgery that I didn't have time for.  I did so
> by re-reading early in the thread to understand what the actual
> requirements were for this feature to work, and it seemed to me that the
> patch started to derail at some point.  I suggest that somebody needs to
> write up exactly what we need, lest the patches end up implementing
> something else.
> 
> I don't have time for this patch during the current commitfest, and I'm
> not sure that I will during the next one.  If somebody else wants to
> spend time with it, ... be my guest.

Given the above, and that nothing has happened on this thread since this note,
I think we should mark this Returned with Feedback and await a new submission.
Does that seem reasonable Alvaro?

--
Daniel Gustafsson   https://vmware.com/

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Jul-14, vignesh C wrote:

> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".

BTW I gave a look at this patch in the March commitfest and concluded it
still requires some major surgery that I didn't have time for.  I did so
by re-reading early in the thread to understand what the actual
requirements were for this feature to work, and it seemed to me that the
patch started to derail at some point.  I suggest that somebody needs to
write up exactly what we need, lest the patches end up implementing
something else.

I don't have time for this patch during the current commitfest, and I'm
not sure that I will during the next one.  If somebody else wants to
spend time with it, ... be my guest.

-- 
Álvaro Herrera  Valdivia, Chile  —  https://www.EnterpriseDB.com/
"Sallah, I said NO camels! That's FIVE camels; can't you count?"
(Indiana Jones)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[vignesh C]

On Tue, Mar 30, 2021 at 2:14 AM Mark Rofail  wrote:
>
> Hey Alvaro
>
>> Yes, we should do that.
>
> I have attached v12 with more tests in “ src/test/regress/sql/gin.sql”
>
> Changelog:
> - v12 (compatible with current master 2021/03/29, commit 
> 6d7a6feac48b1970c4cd127ee65d4c487acbb5e9)
> * add more tests to “ src/test/regress/sql/gin.sql”
> * merge Andreas' edits to the GIN patch
>
> Also, @Andreas Karlsson, waiting for your edits to "pg_constraint"

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

>
> Hey Alvaro,

Well, if it's true that it's translated to the commutator, then I don't
>
think any other code changes are needed.

Great, I will get a patch ready tomorrow. Hopefully we’ll wrap up the GIN
part of the patch soon.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Mar-27, Mark Rofail wrote:

> Hello Alvaro,
> 
> Looking at 0001+0003, I see it claims GIN support for <<@ and @>>, but
> > actually only the former is implemented fully; the latter is missing a
> > strategy number in ginarrayproc.c and pg_amop.dat, and also
> > src/test/regress/sql/gin.sql does not test it.  I suspect
> > ginqueryarrayextract needs to be told about this too.
> 
> GIN/array_ops requires the left operand to be an array, so only @>> can be
> used in GIN.
>
> However, <<@ is defined as @>> commutative counterpart, so
> when for example “5 <<@ index” it will be translated to “index @>> index”
> thus indirectly using the GIN index.

Ah, that makes sense.

Looking at the docs again, I don't see anything that's wrong.  I was
confused about the lack of a new strategy number, but that's explained
by what you say above.

> We can definitely add tests to “ src/test/regress/sql/gin.sql” to test
> this. Do you agree?

Yes, we should do that.

> Also what do you mean by “ ginqueryarrayextract needs to be told about this
> too”?

Well, if it's true that it's translated to the commutator, then I don't
think any other code changes are needed.

-- 
Álvaro Herrera39°49'30"S 73°17'W
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo lógico y coherente. Pero el universo real se halla siempre
un paso más allá de la lógica" (Irulan)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Alvaro,

Looking at 0001+0003, I see it claims GIN support for <<@ and @>>, but
> actually only the former is implemented fully; the latter is missing a
> strategy number in ginarrayproc.c and pg_amop.dat, and also
> src/test/regress/sql/gin.sql does not test it.  I suspect
> ginqueryarrayextract needs to be told about this too.

GIN/array_ops requires the left operand to be an array, so only @>> can be
used in GIN. However, <<@ is defined as @>> commutative counterpart, so
when for example “5 <<@ index” it will be translated to “index @>> index”
thus indirectly using the GIN index.

We can definitely add tests to “ src/test/regress/sql/gin.sql” to test
this. Do you agree?

Also what do you mean by “ ginqueryarrayextract needs to be told about this
too”?

Best Regards,
Mark Rofail

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Looking at 0001+0003, I see it claims GIN support for <<@ and @>>, but
actually only the former is implemented fully; the latter is missing a
strategy number in ginarrayproc.c and pg_amop.dat, and also
src/test/regress/sql/gin.sql does not test it.  I suspect
ginqueryarrayextract needs to be told about this too.

-- 
Álvaro Herrera39°49'30"S 73°17'W
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hi Zhihing,
I think @Andreas ment to mark it as a todo to cleanup later.

On Sun, 21 Mar 2021 at 4:49 AM Zhihong Yu  wrote:

> Hi,
> In v11-0004-fk_arrays_elems_edits.patch :
>
> +   riinfo->fk_reftypes[i] ==
> FKCONSTR_REF_EACH_ELEMENT ? OID_ARRAY_ELEMCONTAINED_OP :
> riinfo->pf_eq_oprs[i], // XXX
>
> Is XXX placeholder for some comment you will fill in later ?
>
> Cheers
>
> On Sat, Mar 20, 2021 at 11:42 AM Mark Rofail 
> wrote:
>
>> Hey Andreas and Joel!
>>
>> Thank you so much for your hard work!
>>
>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>>> anti-join instead. The code is much clearer now IMHO.
>>>
>> It is much cleaner! I
>>
>> 2. That instead of selecting operators at execution time we save which
>>> operators to use in pg_constraint.
>>
>> This is a clever approach. If you have any updates on this please let me
>> know.
>>
>> I am still reviewing your changes. I have split your changes from my work
>> to be able to isolate the changes and review them carefully. And to help
>> others review the changes.
>>
>> Changelist:
>> - v11 (compatible with current master 2021, 03, 20,
>> commit e835e89a0fd267871e7fbddc39ad00ee3d0cb55c)
>> * rebase
>> * split andreas and joel's work
>>
>>
>> On Tue, Mar 16, 2021 at 1:27 AM Andreas Karlsson 
>> wrote:
>>
>>> On 3/15/21 4:29 PM, Mark Rofail wrote:
>>> > Actually, your fix led me in the right way, the issue was how windows
>>> > handle pointers.
>>>
>>> Hi,
>>>
>>> I have started working on a partially new strategy for the second patch.
>>> The ideas are:
>>>
>>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>>> anti-join instead (this was implemented by Joel Jacobson with cleanup
>>> and finishing touches by me). The code is much clearer now IMHO.
>>>
>>> 2. That instead of selecting operators at execution time we save which
>>> operators to use in pg_constraint. This is heavily a work in progress in
>>> my attached patches. I am not 100% convinced this is the right way to go
>>> but I plan to work some on this this weekend to see how ti will work out.
>>>
>>> Another thing I will look into is you gin patch. While you fixed it for
>>> simple scalar types which fit into the Datum type I wonder if we do not
>>> also need to copy types which are too large to fit into a Datum but I
>>> have not investigated yet which memory context the datum passed to
>>> ginqueryarrayextract() is allocated in.
>>>
>>> Andreas
>>>
>>>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Zhihong Yu]

Hi,
In v11-0004-fk_arrays_elems_edits.patch :

+   riinfo->fk_reftypes[i] ==
FKCONSTR_REF_EACH_ELEMENT ? OID_ARRAY_ELEMCONTAINED_OP :
riinfo->pf_eq_oprs[i], // XXX

Is XXX placeholder for some comment you will fill in later ?

Cheers

On Sat, Mar 20, 2021 at 11:42 AM Mark Rofail  wrote:

> Hey Andreas and Joel!
>
> Thank you so much for your hard work!
>
> 1. I have removed the dependency on count(DISTINCT ...) by using an
>> anti-join instead. The code is much clearer now IMHO.
>>
> It is much cleaner! I
>
> 2. That instead of selecting operators at execution time we save which
>> operators to use in pg_constraint.
>
> This is a clever approach. If you have any updates on this please let me
> know.
>
> I am still reviewing your changes. I have split your changes from my work
> to be able to isolate the changes and review them carefully. And to help
> others review the changes.
>
> Changelist:
> - v11 (compatible with current master 2021, 03, 20,
> commit e835e89a0fd267871e7fbddc39ad00ee3d0cb55c)
> * rebase
> * split andreas and joel's work
>
>
> On Tue, Mar 16, 2021 at 1:27 AM Andreas Karlsson 
> wrote:
>
>> On 3/15/21 4:29 PM, Mark Rofail wrote:
>> > Actually, your fix led me in the right way, the issue was how windows
>> > handle pointers.
>>
>> Hi,
>>
>> I have started working on a partially new strategy for the second patch.
>> The ideas are:
>>
>> 1. I have removed the dependency on count(DISTINCT ...) by using an
>> anti-join instead (this was implemented by Joel Jacobson with cleanup
>> and finishing touches by me). The code is much clearer now IMHO.
>>
>> 2. That instead of selecting operators at execution time we save which
>> operators to use in pg_constraint. This is heavily a work in progress in
>> my attached patches. I am not 100% convinced this is the right way to go
>> but I plan to work some on this this weekend to see how ti will work out.
>>
>> Another thing I will look into is you gin patch. While you fixed it for
>> simple scalar types which fit into the Datum type I wonder if we do not
>> also need to copy types which are too large to fit into a Datum but I
>> have not investigated yet which memory context the datum passed to
>> ginqueryarrayextract() is allocated in.
>>
>> Andreas
>>
>>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

>
>
> This still fails for CI (windows) and me (linux)
>
> Can you provide more information about your Linux platform? So I may test
> the errors for myself?
>
Seems the new tests fails every CI. That’s good honestly, that we found
this problem.

The `arrays` regression test extensively test the new operators. So I think
the problem will be in when the operators are combined with the GIN index.

The problem is that the tests don’t fail on my linux build. Any idea how to
replicate the failed tests so I can debug?

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Justin Pryzby]

On Fri, Mar 12, 2021 at 11:32:27PM +0200, Mark Rofail wrote:
> I have retested the patch on a windows build and it passes the regression
> tests thanks to Justin's recommendations. Hopefully, it will pass CI too.
> 
> Changelog:
>  - v7 (compatible with current master 2021-3-12,
> commit 02b5940dbea17d07a1dbcba3cbe113cc8b70f228)
> * re-add failing regression test with fixes
> * rebase patch

This still fails for CI (windows) and me (linux):

 SELECT ftest1 FROM FKTABLEFORARRAYGIN WHERE ftest1 @>> 5;
-   ftest1
--
- {5}
- {3,5,2,5}
- {3,5,4,1,3}
- {5,1}
- {3,4,5,3}
-(5 rows)
+ ftest1 
+
+(0 rows)

You added enable_seqscan=off, and EXPLAIN to show that it uses an bitmap index
scan, but do you know why it failed ?

I guess the failure is in the first patch, but isn't caught by test cases until
the 2nd patch.

-- 
Justin

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Justin,

It doesn't just rebase: it also removes the test which was failing on
> windows
> CI:
>
My apologies, I didn’t include it in the changelog since this is not a code
change, just wanted to see if any other test would fail on the windows CI

I think the SELECT, when it works, is actually doing a seq scan and not
> using
> the index.  On my PC, the index scan is used until an autovacuum/analyze
> run,
> after which it uses seqscan.  I'm not sure how the other CIs all managed
> to run
> autovacuum between creating a table and running a query on it, though.

This is genius! That explains it. I have been racking my brain for two
weeks now and you figured it out.

I guess you should first run the query with "explain (costs off)" to show
> what
> plan it's using, and add things like "SET enable_seqscan=off" as needed to
> guarantee that everyone will use the same plan, regardless of minor cost
> differences and vacuum timing.

I think that will solve the test discrepancy.

Honestly Justin, hats off!

/Mark

>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Justin Pryzby]

On Wed, Mar 03, 2021 at 11:31:49PM +0200, Mark Rofail wrote:
> This is just a rebase patch since the patch is no longer applicable to the
> current master.

It doesn't just rebase: it also removes the test which was failing on windows
CI:

--- Try using the indexable operator
-SELECT * FROM FKTABLEFORARRAYGIN WHERE ftest1 @>> 5;

> Changelog:
> - v6 (compatible with current master 2020-3-3,
> commit 3769e11a31831fc2f3bd4c4a24b4f45c352fb8fb)
> * rebase to current master

I think the SELECT, when it works, is actually doing a seq scan and not using
the index.  On my PC, the index scan is used until an autovacuum/analyze run,
after which it uses seqscan.  I'm not sure how the other CIs all managed to run
autovacuum between creating a table and running a query on it, though.

I guess you should first run the query with "explain (costs off)" to show what
plan it's using, and add things like "SET enable_seqscan=off" as needed to
guarantee that everyone will use the same plan, regardless of minor cost
differences and vacuum timing.

-- 
Justin

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Justin Pryzby]

On Tue, Feb 16, 2021 at 12:07:10PM +0200, Mark Rofail wrote:
...

There's some errors in the latest patch:

http://cfbot.cputube.org/mark-rofail.html

gram.y:16933:20: error: invalid operands to binary expression ('List' (aka 
'struct List') and 'void *')
Assert(**reftypes != NULL);

Did you mean to write this, before the assignment of NIL ?

Assert(reftypes != NULL);
Assert(names != NULL);

Apparently these Asserts were added last month.

The windows build succeeded, but checks failed like:

 SELECT * FROM FKTABLEFORARRAYGIN WHERE ftest1 @>> 5;
ftest1| ftest2 
--+
- {5} |  1
- {3,5,2,5}   |  3
- {3,5,4,1,3} |  5
- {5,1}   |  7
- {3,4,5,3}   | 10
-(5 rows)
++
+(0 rows)

Would you send an updated patch to address these ?

I suggest to generate the patch series with:
git format-patch -v2 origin.. -o patch/foreign-key-arrays/

That generates patches with a prefix like 0001, indicating which one goes
"first" (and therefore doesn't depend on the others).

-- 
Justin

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hey Joel,

I will now proceed with the review of fk_arrays_elem_v2 as well.
>
Great work!!

/Mark

>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi all,

On Tue, Feb 16, 2021, at 11:07, Mark Rofail wrote:
>Changelog:
>- anyarray_anyelement_operators-v4 (compatible with current master 2021-02-16, 
>>commit f672df5fdd22dac14c98d0a0bf5bbaa6ab17f8a5)
>* revert anycompatiblenonarray in docs to anyelement

Good.

I've marked anyarray_anyelement_operators-v4 "Ready for Committer".

>- fk_arrays_elem_v2:
>* remove coercion support in regression tests
>* update to be compatible with anyarray_anyelement_operators-v4 

I will now proceed with the review of fk_arrays_elem_v2 as well.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi all,

I've reviewed Mark's anyarray_anyelement_operators-v2.patch
and the only remaining issue I've identified is the opr_sanity problem.

Mark seems to be in need of some input here from more experienced hackers, see 
below.

Hopefully someone can guide him in the right direction.

/Joel

On Sat, Feb 13, 2021, at 11:49, Mark Rofail wrote:
>Hey Joel,
>
>test opr_sanity   ... FAILED
>
>AND binary_coercible(p2.opcintype, p1.amoplefttype));
>  amopfamily | amopstrategy | amopopr
>+--+-
>-(0 rows)
>+   2745 |5 |6105
>+(1 row)
>-- Operators that are primary members of opclasses must be immutable (else
>-- it suggests that the index ordering isn't fixed).  Operators that are
>This is due using anycompatiblearray for the left operand in @>>. 
>To solve this problem we need to use @>>(anyarray,anyelement) or introduce a 
>new opclass for gin indices. 
>These are the two approaches that come to mind to solve this. Which one is the 
>right way or is there another solution I am not aware of?
>That’s why I’m asking this on the mailing list, to get the community’s input.

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Sat, Feb 13, 2021, at 12:35, Joel Jacobson wrote:
>psql:type-test.sql:165: WARNING:
>SQL queries produced different results:
>SELECT '285970053'::pg_catalog."numeric" = 
>ANY(ARRAY['285970053']::pg_catalog.float4[])
>false
>SELECT '285970053'::pg_catalog."numeric" <<@ 
>ARRAY['285970053']::pg_catalog.float4[]
>true

I think I've figured this one out.

It looks like the ANY() case converts the float4-value to numeric and then 
compare it with the numeric-value,
while in the <<@ case converts the numeric-value to float4 and then compare it 
with the float4-value.

Since '285970053'::numeric::float4 = '285970053'::float4 is TRUE,
while '285970053'::float4::numeric = '285970053'::numeric is FALSE,
this gives a different result.

Is it documented somewhere which type is picked as the type for the comparison?

"The common type is selected following the same rules as for UNION and related 
constructs (see Section 10.5)."
(https://www.postgresql.org/docs/current/extend-type-system.html#EXTEND-TYPES-POLYMORPHIC)

SELECT (SELECT '285970053'::numeric UNION SELECT '285970053'::float4) = 
'285970053'::float4;
?column?
--
t
(1 row)

Apparently float4 is selected as the common type according to these rules.

So the <<@ operator seems to be doing the right thing. But in the ANY() case, 
it seems to convert the float4 element in the float4[] array to numeric, and 
then compare the numeric values.

I can see how this is normal and expected, but it was a bit surprising to me at 
first.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Fri, Feb 12, 2021, at 20:56, Mark Rofail wrote:
>Attachments:
>anyarray_anyelement_operators-v2.patch

I've created a quite sophisticated test in PL/pgSQL,
that takes advantage of all the test data produced
by the official PostgreSQL regression test suite.

It goes through all tables and columns, and extracts values
for all the different types it can find.

It then uses these values to probe for differences between

some_value::some_type = ANY(ARRAY[some_other_value]::some_other_type[])
and
   some_value::some_type <<@ ARRAY[some_other_value]::some_other_type[]

psql:type-test.sql:165: NOTICE:

144 of 21632 tests failed.


Out of these 144 differences found, this one was really interesting:

psql:type-test.sql:165: WARNING:
SQL queries produced different results:
SELECT '285970053'::pg_catalog."numeric" = 
ANY(ARRAY['285970053']::pg_catalog.float4[])
false
SELECT '285970053'::pg_catalog."numeric" <<@ 
ARRAY['285970053']::pg_catalog.float4[]
true

I don't see why one of them returns false and the other true?

If testing for equality:

SELECT '285970053'::pg_catalog.float4 = '285970053'::numeric;

You get "false".

So it looks like ANY() does the right thing here, and <<@ has a problem.

To run, first run the PostgreSQL regression tests, to produce data in the 
"regression" database.

Then run:

$ psql -f type-test.sql regression

/Joel

type-test.sql
Description: Binary data


type-test.out
Description: Binary data

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hey Joel,

test opr_sanity   ... FAILED
>
> AND binary_coercible(p2.opcintype, p1.amoplefttype));
>   amopfamily | amopstrategy | amopopr
> +--+-
> -(0 rows)
> +   2745 |5 |6105
> +(1 row)
>
> -- Operators that are primary members of opclasses must be immutable (else
> -- it suggests that the index ordering isn't fixed).  Operators that are
>
This is due using anycompatiblearray for the left operand in @>>.
To solve this problem we need to use @>>(anyarray,anyelement) or introduce
a new opclass for gin indices.
These are the two approaches that come to mind to solve this. Which one is
the right way or is there another solution I am not aware of?
That’s why I’m asking this on the mailing list, to get the community’s
input.

>
/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hey Joel,

+ oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
> -+ if (oprresult)
> ++ if (!locfcinfo->isnull && oprresult)
> + return true;
> + }
>
> Is checking !locfcinfo->isnull due to something new in v2,
> or what is just a correction for a problem also in v1?
>
The “!locfcinfo->isnull” is to protect against segmentation faults. I found
it was added to the original function which I adapted the “element” version
from.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi again Mark,

On Fri, Feb 12, 2021, at 20:56, Mark Rofail wrote:
>Attachments:
>anyarray_anyelement_operators-v2.patch

One regression test fails on my machine:

make installcheck
test opr_sanity   ... FAILED 3994 ms

1 of 202 tests failed.


diff -U3 /Users/joel/src/postgresql/src/test/regress/expected/opr_sanity.out 
/Users/joel/src/postgresql/src/test/regress/results/opr_sanity.out
--- /Users/joel/src/postgresql/src/test/regress/expected/opr_sanity.out 
2021-02-13 10:29:50.0 +0100
+++ /Users/joel/src/postgresql/src/test/regress/results/opr_sanity.out 
2021-02-13 11:09:43.0 +0100
@@ -2139,7 +2139,8 @@
AND binary_coercible(p2.opcintype, p1.amoplefttype));
  amopfamily | amopstrategy | amopopr
+--+-
-(0 rows)
+   2745 |5 |6105
+(1 row)

-- Operators that are primary members of opclasses must be immutable (else
-- it suggests that the index ordering isn't fixed).  Operators that are

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

>On Fri, Feb 12, 2021, at 20:56, Mark Rofail wrote:
>Attachments:
>anyarray_anyelement_operators-v2.patch

Some more code review comments:

Comparing the v1 and v2 patch, I noticed this change in array_contains_elem():
+ oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
-+ if (oprresult)
++ if (!locfcinfo->isnull && oprresult)
+ return true;
+ }

Is checking !locfcinfo->isnull due to something new in v2,
or what is just a correction for a problem also in v1?

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Fri, Feb 12, 2021, at 20:56, Mark Rofail wrote:
>Indeed you are right, to support the correct behaviour we have to use 
>@>>(anycompatiblearray, anycompatiblenonarry) and >this throws a sanity error 
>in opr_santiy since the left operand doesn't equal the gin opclass which is 
>anyarray. I am thinking >to solve this we need to add a new opclass under gin 
>"compatible_array_ops" beside the already existing "array_ops", >what do you 
>think?

I'm afraid I have no idea. I don't really understand how these 
"anycompatible"-types work, I only knew of "anyarray" and "anyelement" until 
recently. I will study these in detail to get a better understanding. But 
perhaps you could just explain a quick question first:

Why couldn't/shouldn't @>> and <<@ be operating on anyarray and anyelement?
This would seem more natural to me since the Array Operators versions of @> and 
<@ operate on anyarray.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

>On Thu, Feb 11, 2021, at 16:50, Mark Rofail wrote:
>> Here comes a first review of the anyarray_anyelement_operators-v1.patch.
>Great, thanks! I’ll start applying your comments today and release a new patch.

Here comes some more feedback:

I was surprised to see <<@ and @>> don't complain when trying to compare 
incompatible types:

regression=# select '1'::text <<@ ARRAY[1::integer,2::integer];
?column?
--
f
(1 row)

I would expect the same result as if using the <@ and @> operators,
and wrapping the value in an array:

regression=# select ARRAY['1'::text] <@ ARRAY[1::integer,2::integer];
ERROR:  operator does not exist: text[] <@ integer[]
LINE 1: select ARRAY['1'::text] <@ ARRAY[1::integer,2::integer];
^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.

The error above for the existing <@ operator is expected,
and I think the <<@ should give a similar error.

Even worse, when using integer on the left side and text in the array,
the <<@ operator causes a seg fault:

regression=# select 1::integer <<@ ARRAY['1'::text,'2'::text];
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

2021-02-11 22:18:53.083 CET [91680] LOG:  server process (PID 1666) was 
terminated by signal 11: Segmentation fault: 11
2021-02-11 22:18:53.083 CET [91680] DETAIL:  Failed process was running: select 
1::integer <<@ ARRAY['1'::text,'2'::text];
2021-02-11 22:18:53.083 CET [91680] LOG:  terminating any other active server 
processes
2021-02-11 22:18:53.084 CET [1735] FATAL:  the database system is in recovery 
mode
2021-02-11 22:18:53.084 CET [91680] LOG:  all server processes terminated; 
reinitializing
2021-02-11 22:18:53.092 CET [1736] LOG:  database system was interrupted; last 
known up at 2021-02-11 22:14:41 CET
2021-02-11 22:18:53.194 CET [1736] LOG:  database system was not properly shut 
down; automatic recovery in progress
2021-02-11 22:18:53.197 CET [1736] LOG:  redo starts at 0/2BCA5520
2021-02-11 22:18:53.197 CET [1736] LOG:  invalid record length at 0/2BCA5558: 
wanted 24, got 0
2021-02-11 22:18:53.197 CET [1736] LOG:  redo done at 0/2BCA5520 system usage: 
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s
2021-02-11 22:18:53.207 CET [91680] LOG:  database system is ready to accept 
connections

Maybe it's the combination of "anyarray" and "anycompatiblenonarray" that is 
the problem here?

Some more comments on the code:

array_contains_elem(AnyArrayType *array, Datum elem,
+   /*
+* Apply the comparison operator to each pair of array elements.
+*/
This comment has been copy/pasted from array_contain_compare().
Maybe the wording should clarify there is only one array in this function,
the word "pair" seems to imply working with two arrays.


+   for (i = 0; i < nelems; i++)
+   {
+   Datum elt1;

The name `elt1` originates from the array_contain_compare() function.
But since this function, array_contains_elem(), doesn't have a `elt2`,
it would be better to use `elt` as a name here. The same goes for `it1`.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hey Joel,

> Here comes a first review of the anyarray_anyelement_operators-v1.patch.
Great, thanks! I’ll start applying your comments today and release a new
patch.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Mon, Feb 8, 2021, at 09:40, Mark Rofail wrote:
> anyarray_anyelement_operators-v1.patch

Here comes a first review of the anyarray_anyelement_operators-v1.patch.

doc/src/sgml/func.sgml
+Does the array contain specified element ?

* Maybe remove the extra blank space before question mark?

doc/src/sgml/indices.sgml
-@  @  =  
+@ @  @ @  =  

* To me it looks like the pattern is to insert  between each operator, in 
which case this should be written:

@  @  @ @  =  

I.e.,  is missing between @ and @.

src/backend/access/gin/ginarrayproc.c
/* Make copy of array input to ensure it doesn't disappear while in use 
*/
-   ArrayType  *array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+   ArrayType  *array;

I think the comment above should be changed/moved since the copy has been moved 
and isn't always performed due to the if. Since array variable is only used in 
the else block, couldn't both the comment and the declaration of array be moved 
to the else block as well?

src/backend/utils/adt/arrayfuncs.c
+   /*
+* We assume that the comparison operator is strict, so a NULL 
can't
+* match anything.  XXX this diverges from the "NULL=NULL" 
behavior of
+* array_eq, should we act like that?
+*/

The comment above is copy/pasted from array_contain_compare(). It seems 
unnecessary to have this open question, word-by-word, on two different places. 
I think a reference from here to the existing similar code would be better. And 
also to add a comment in array_contain_compare() about the existence of this 
new code where the same question is discussed.

If this would be new code, then this question should probably be answered 
before committing, but since this is old code, maybe the behaviour now can't be 
changed anyway, since the old code in array_contain_compare() has been around 
since 2006, when it was introduced in commit 
f5b4d9a9e08199e6bcdb050ef42ea7ec0f7525ca.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Mon, Feb 8, 2021, at 09:40, Mark Rofail wrote:
>Attachments:
>fk_arrays_elem_v1.patch
>anyarray_anyelement_operators-v1.patch

Nice work!

I have successfully tested both patches against 
e7f42914854926c2afbb89b9cd0e381fd90766be
by cloning all pg_catalog tables, and adding foreign keys
on all columns, including array columns of course.

Here is what e.g. pg_constraint which has quite a few array oid columns looks 
like with foreign keys:

joel=# \d catalog_clone.pg_constraint
 Table "catalog_clone.pg_constraint"
Column |Type| Collation | Nullable | Default
---++---+--+-
oid   | jsonb  |   | not null |
conname   | name   |   |  |
.
.
.
Foreign-key constraints:
"pg_constraint_conexclop_fkey" FOREIGN KEY (EACH ELEMENT OF conexclop) 
REFERENCES catalog_clone.pg_operator(oid)
"pg_constraint_conffeqop_fkey" FOREIGN KEY (EACH ELEMENT OF conffeqop) 
REFERENCES catalog_clone.pg_operator(oid)
"pg_constraint_conpfeqop_fkey" FOREIGN KEY (EACH ELEMENT OF conpfeqop) 
REFERENCES catalog_clone.pg_operator(oid)
"pg_constraint_conppeqop_fkey" FOREIGN KEY (EACH ELEMENT OF conppeqop) 
REFERENCES catalog_clone.pg_operator(oid)
"pg_constraint_conrelid_conkey_fkey" FOREIGN KEY (conrelid, EACH ELEMENT OF 
conkey) REFERENCES catalog_clone.pg_attribute(attrelid, attnum)

Here is my test function that adds foreign keys on catalog tables:

https://github.com/truthly/pg-pit/blob/master/FUNCTIONS/test_referential_integrity.sql

If you want to try it yourself, it is run as part of pit's test suite:

$ git clone https://github.com/truthly/pg-pit.git
$ cd pg-pit
$ make
$ make install
$ make installcheck

== running regression test queries ==
test referential_integrity... ok 1925 ms

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

[Found it :-)]

On 2021-Feb-05, Mark Rofail wrote:

> We will focus on getting the operator patch through first. Should I create
> a separate commitfest ticket? or the current one suffices?
> https://commitfest.postgresql.org/32/2966/

I think the current one is fine.  In fact I would encourage you to post
both patches together as two attachment in the same email; that way, the
CF bot would pick them up correctly.  When you post them in separate
emails, it doesn't know what to do with them.  See here:
http://cfbot.cputube.org/mark-rofail.html

-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Feb-07, Mark Rofail wrote:

> Changelog (operator patch):
> > - v1 (compatible with current master 2021-02-05,
> > commit c72af5c202067a9ecb0ff8df7370fb1ea8f4)
> > * add tests and documentation to array operators and gin index
> >
> Since we agreed to separate @>> and <<@ operators to prerequisite patch to
> the FK Arrays, I have rebased the patch to be applied on
> "anyarray_anyelement_operators-vX.patch"

Um, where is that other patch?


-- 
Álvaro Herrera39°49'30"S 73°17'W

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Stephen,

> Thanks a lot for your persistence, by the way.
> +100
> Hopefully we'll get this in during this cycle and perhaps then you'll work
> on something else? :D

Thank you for your kind words! Yes, hopefully, we'll get this in this time
around. I would definitely love to work on something else once this is done.


> Great to see a GSoC student continue to work
> with the community years after the GSoC they participated in.  If you'd
> be interested in being a mentor for this upcoming GSoC season, please
> let me know!
>
Hmm, not sure. I don't think I am well acquainted with the codebase to
actually mentor someone. Maybe if I get some experience I would be ready
for GSoC 2022.

Thanks again Stephen and Álvaro

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Álvaro,

Well, *I* think it makes sense to do it that way.  I said so three years
> ago :-)
> https://postgr.es/m/20180410135917.odjh5coa4cjatz5v@alvherre.pgsql

So this makes a lot of sense, let's do that.


> I wonder if it can usefully get cross-type
> operators, i.e., @>>(bigint[],smallint) in some way?  Maybe the
> "anycompatiblearray" thing can be used for that purpose?

It was easy to get @>> and <<@ accept cross-types thanks to your
suggestion, but I opted to having the operators defined as follows to still
be consistent with the GIN index since the index needs the first operant to
be of type "anyarray"
@>>(anyarray, anycompatiblenonearray) and <<@(anycompatiblenonearray,
anyarray)

Thanks a lot for your persistence, by the way.

Thank you for your words of encouragement, it was one of my deepest
regrests to not seeing this though while in GSoC, hopefiully it gets
commited this time around.

We will focus on getting the operator patch through first. Should I create
a separate commitfest ticket? or the current one suffices?
https://commitfest.postgresql.org/32/2966/

Changelog (operator patch):
- v1 (compatible with current master 2021-02-05,
commit c72af5c202067a9ecb0ff8df7370fb1ea8f4)
* add tests and documentation to array operators and gin index

/Mark
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b7150510ab..3d36e88494 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17525,6 +17525,34 @@ SELECT NULLIF(value, '(none)') ...

   
 
+  
+   
+anyarray @ anycompatiblenonarray
+boolean
+   
+   
+Does the array contain specified element ?
+   
+   
+ARRAY[1,4,3] @ 3
+t
+   
+  
+
+  
+   
+anycompatiblenonarray @ anyarray
+boolean
+   
+   
+Is the specified element contained in the array?
+   
+   
+2 @ ARRAY[1,7,4,2,6]
+t
+   
+  
+
   

 anyarray  anyarray
diff --git a/doc/src/sgml/gin.sgml b/doc/src/sgml/gin.sgml
index d68d12d515..a744609a6b 100644
--- a/doc/src/sgml/gin.sgml
+++ b/doc/src/sgml/gin.sgml
@@ -84,7 +84,7 @@
 
 
  
-  array_ops
+  array_ops
(anyarray,anyarray)
  
  
@@ -93,6 +93,12 @@
  
   @ (anyarray,anyarray)
  
+ 
+  @ (anyarray,anycompatiblenonarray)
+ 
+ 
+  @ (anycompatiblenonarray,anyarray)
+ 
  
   = (anyarray,anyarray)
  
diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
index 623962d1d8..3cfc64cfe1 100644
--- a/doc/src/sgml/indices.sgml
+++ b/doc/src/sgml/indices.sgml
@@ -326,7 +326,7 @@ SELECT * FROM places ORDER BY location <-> point '(101,456)' LIMIT 10;
for arrays, which supports indexed queries using these operators:
 
 
-@  @  =  
+@ @  @ @  =  
 
 
(See  for the meaning of
diff --git a/src/backend/access/gin/ginarrayproc.c b/src/backend/access/gin/ginarrayproc.c
index bf73e32932..9b91582021 100644
--- a/src/backend/access/gin/ginarrayproc.c
+++ b/src/backend/access/gin/ginarrayproc.c
@@ -24,6 +24,7 @@
 #define GinContainsStrategy		2
 #define GinContainedStrategy	3
 #define GinEqualStrategy		4
+#define GinContainsElemStrategy	5
 
 
 /*
@@ -79,7 +80,7 @@ Datum
 ginqueryarrayextract(PG_FUNCTION_ARGS)
 {
 	/* Make copy of array input to ensure it doesn't disappear while in use */
-	ArrayType  *array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+	ArrayType  *array;
 	int32	   *nkeys = (int32 *) PG_GETARG_POINTER(1);
 	StrategyNumber strategy = PG_GETARG_UINT16(2);
 
@@ -94,14 +95,31 @@ ginqueryarrayextract(PG_FUNCTION_ARGS)
 	bool	   *nulls;
 	int			nelems;
 
-	get_typlenbyvalalign(ARR_ELEMTYPE(array),
-		 , , );
+	if (strategy == GinContainsElemStrategy)
+	{
+		/*
+		* since this function returns a pointer to a
+		* deconstructed array, there is nothing to do if
+		* operand is a single element except to return it
+		* as is and configure the searchmode
+		*/
 
-	deconstruct_array(array,
-	  ARR_ELEMTYPE(array),
-	  elmlen, elmbyval, elmalign,
-	  , , );
+		nelems = 1;
+		elems = _GETARG_DATUM(0);
+		nulls = _ARGISNULL(0);
+	}
+	else
+	{
+		array = PG_GETARG_ARRAYTYPE_P_COPY(0);
+
+		get_typlenbyvalalign(ARR_ELEMTYPE(array),
+			 , , );
 
+		deconstruct_array(array,
+		  ARR_ELEMTYPE(array),
+		  elmlen, elmbyval, elmalign,
+		  , , );
+	}
 	*nkeys = nelems;
 	*nullFlags = nulls;
 
@@ -126,6 +144,14 @@ ginqueryarrayextract(PG_FUNCTION_ARGS)
 			else
 *searchMode = GIN_SEARCH_MODE_INCLUDE_EMPTY;
 			break;
+		case GinContainsElemStrategy:
+			/*
+			 * only items that match the queried element
+			 * are considered candidate
+			 */
+
+			*searchMode = GIN_SEARCH_MODE_DEFAULT;
+			break;
 		default:
 			elog(ERROR, "ginqueryarrayextract: unknown strategy number: %d",
  strategy);
@@ -185,6 +211,7 @@ ginarrayconsistent(PG_FUNCTION_ARGS)
 }
 			}
 			break;
+		case GinContainsElemStrategy:
 		case 

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Stephen Frost]

Greetings,

* Alvaro Herrera (alvhe...@alvh.no-ip.org) wrote:
> On 2021-Feb-05, Mark Rofail wrote:
> > I disagree -- I think we should get the second patch in, and consider it
> > > a requisite for the other one.
> > 
> > I just want to make sure I got your last message right. We should work on
> > adding the <<@ and @>> operators and their GIN logic as a separate patch
> > then submit the FKARRAY as a future patch?
> 
> Well, *I* think it makes sense to do it that way.  I said so three years
> ago :-)
> https://postgr.es/m/20180410135917.odjh5coa4cjatz5v@alvherre.pgsql
> 
> > So basically reverse the order of the patches.
> 
> Yeah.

I tend to agree with Alvaro on this, that getting the operators in first
makes more sense.

> Thanks a lot for your persistence, by the way.

+100

Hopefully we'll get this in during this cycle and perhaps then you'll
work on something else? :D  Great to see a GSoC student continue to work
with the community years after the GSoC they participated in.  If you'd
be interested in being a mentor for this upcoming GSoC season, please
let me know!

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Hello Mark

On 2021-Feb-05, Mark Rofail wrote:

> I disagree -- I think we should get the second patch in, and consider it
> > a requisite for the other one.
> 
> I just want to make sure I got your last message right. We should work on
> adding the <<@ and @>> operators and their GIN logic as a separate patch
> then submit the FKARRAY as a future patch?

Well, *I* think it makes sense to do it that way.  I said so three years
ago :-)
https://postgr.es/m/20180410135917.odjh5coa4cjatz5v@alvherre.pgsql

> So basically reverse the order of the patches.

Yeah.

Thanks a lot for your persistence, by the way.

-- 
Álvaro Herrera39°49'30"S 73°17'W
"We're here to devour each other alive"(Hobbes)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

 Hello Álvaro,

I disagree -- I think we should get the second patch in, and consider it
> a requisite for the other one.

I just want to make sure I got your last message right. We should work on
adding the <<@ and @>> operators and their GIN logic as a separate patch
then submit the FKARRAY as a future patch?
So basically reverse the order of the patches.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

On 2021-Feb-05, Mark Rofail wrote:

> I believe we should start merging these two patches as one, due to the Elem
> addon's benefits. such as adding Composite Type support.

I disagree -- I think we should get the second patch in, and consider it
a requisite for the other one.  Let's iron it out fully and get it
pushed soon, then we can rebase the array FK patch on top.  I think it's
missing docs, though.  I wonder if it can usefully get cross-type
operators, i.e., @>>(bigint[],smallint) in some way?  Maybe the
"anycompatiblearray" thing can be used for that purpose?

-- 
Álvaro Herrera39°49'30"S 73°17'W
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Zhihong Yu]

Hi,
For Array-containselem-gin-v4.patch , one small comment:

+ * array_contains_elem : checks an array for a spefific element

typo: specific

Cheers

On Thu, Feb 4, 2021 at 4:03 PM Mark Rofail  wrote:

> Hello Joel,
>
> No error, even though bigint[] isn't compatible with smallint.
>>
> I added a check to compart the element type of the fkoperand and the type
> of the pkoperand should be the same
> Please try v18 attached below, you should get the following message
> ```
> ERROR:  foreign key constraint "fktableviolating_ftest1_fkey" cannot be
> implemented
> DETAIL:  Specified columns have element types smallint and bigint which
> are not homogenous.
> ```
>
> Changelog (FK arrays):
> - v18 (compatible with current master 2021-02-54, commit
> c34787f910585f82320f78b0afd53a6a170aa229)
> * add check for operand compatibility at constraint creation
>
> Changelog (FK arrays Elem addon)
> - v4 (compatible with FK arrays v18)
>* re-add Composite Type support
>
> I believe we should start merging these two patches as one, due to the
> Elem addon's benefits. such as adding Composite Type support.
>
> /Mark
>
> On Thu, Feb 4, 2021 at 9:00 AM Joel Jacobson  wrote:
>
>> On Tue, Feb 2, 2021, at 13:51, Mark Rofail wrote:
>> >Array-ELEMENT-foreign-key-v17.patch
>>
>> When working on my pit tool, I found another implicit type casts problem.
>>
>> First an example to show a desired error message:
>>
>> CREATE TABLE a (
>> a_id smallint,
>> PRIMARY KEY (a_id)
>> );
>>
>> CREATE TABLE b (
>> b_id bigint,
>> a_ids text[],
>> PRIMARY KEY (b_id)
>> );
>>
>> ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a;
>>
>> The below error message is good:
>>
>> ERROR:  foreign key constraint "b_a_ids_fkey" cannot be implemented
>> DETAIL:  Key column "a_ids" has element type text which does not have a
>> default btree operator class that's compatible with class "int2_ops".
>>
>> But if we instead make a_ids a bigint[], we don't get any error:
>>
>> DROP TABLE b;
>>
>> CREATE TABLE b (
>> b_id bigint,
>> a_ids bigint[],
>> PRIMARY KEY (b_id)
>> );
>>
>> ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a;
>>
>> No error, even though bigint[] isn't compatible with smallint.
>>
>> We do get an error when trying to insert into the table:
>>
>> INSERT INTO a (a_id) VALUES (1);
>> INSERT INTO b (b_id, a_ids) VALUES (2, ARRAY[1]);
>>
>> ERROR:  operator does not exist: smallint[] pg_catalog.<@ bigint[]
>> LINE 1: ..."."a" x WHERE ARRAY ["a_id"]::pg_catalog.anyarray OPERATOR(p...
>>  ^
>> HINT:  No operator matches the given name and argument types. You might
>> need to add explicit type casts.
>> QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM
>> pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*)
>> FROM (SELECT 1 FROM ONLY "public"."a" x WHERE ARRAY
>> ["a_id"]::pg_catalog.anyarray OPERATOR(pg_catalog.<@)
>> $1::pg_catalog.anyarray FOR KEY SHARE OF x) z)
>>
>> I wonder if we can come up with some general way to detect these
>> problems already at constraint creation time,
>> instead of having to wait for data to get the error,
>> similar to why compile time error are preferred over run time errors.
>>
>> /Joel
>>
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Tue, Feb 2, 2021, at 13:51, Mark Rofail wrote:
>Array-ELEMENT-foreign-key-v17.patch

When working on my pit tool, I found another implicit type casts problem.

First an example to show a desired error message:

CREATE TABLE a (
a_id smallint,
PRIMARY KEY (a_id)
);

CREATE TABLE b (
b_id bigint,
a_ids text[],
PRIMARY KEY (b_id)
);

ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a;

The below error message is good:

ERROR:  foreign key constraint "b_a_ids_fkey" cannot be implemented
DETAIL:  Key column "a_ids" has element type text which does not have a default 
btree operator class that's compatible with class "int2_ops".

But if we instead make a_ids a bigint[], we don't get any error:

DROP TABLE b;

CREATE TABLE b (
b_id bigint,
a_ids bigint[],
PRIMARY KEY (b_id)
);

ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a;

No error, even though bigint[] isn't compatible with smallint.

We do get an error when trying to insert into the table:

INSERT INTO a (a_id) VALUES (1);
INSERT INTO b (b_id, a_ids) VALUES (2, ARRAY[1]);

ERROR:  operator does not exist: smallint[] pg_catalog.<@ bigint[]
LINE 1: ..."."a" x WHERE ARRAY ["a_id"]::pg_catalog.anyarray OPERATOR(p...
 ^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM 
pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) 
FROM (SELECT 1 FROM ONLY "public"."a" x WHERE ARRAY 
["a_id"]::pg_catalog.anyarray OPERATOR(pg_catalog.<@) $1::pg_catalog.anyarray 
FOR KEY SHARE OF x) z)

I wonder if we can come up with some general way to detect these
problems already at constraint creation time,
instead of having to wait for data to get the error,
similar to why compile time error are preferred over run time errors.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Thu, Jan 28, 2021, at 22:41, Mark Rofail wrote:
>Please find v16 with the error message implemented. You can find it in the 
>previous message.

Thanks! It's working fine:
ERROR:  (int2vector) is not an array type, cannot be used as a referencing 
column

I've pushed a first working draft of the schema-diffing-tool I'm working on 
that uses the patch:
https://github.com/truthly/pg-pit

I've added links to the functions that uses the EACH ELEMENT OF syntax.

This project has been my primary source of reviewer insights so far.

I will continue testing the patch over the next couple of days.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Joel,

This means, as a user, I might not be aware of the vector restriction when
> adding the foreign keys
> for my existing schema, and think everything is fine, and not realize
> there is a problem until
> new data arrives.
>
Please find v16 with the error message implemented. You can find it in the
previous message.

> As always thank you for your great insight and your help.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Wed, Jan 27, 2021, at 22:36, Mark Rofail wrote:
> Vectors as refrencing columns are not supported and out of scope of this 
> patch. Try to use arrays.

OK, not a biggie, but I think the user at least should get an error message
immediately when trying to add the foreign key on an incompatible column,
just like we would get if e.g. trying to add a fk on numeric[] -> smallint
or some other incompatible combination.

The first error message looks good:

CREATE TABLE a (
  a_id smallint NOT NULL,
  PRIMARY KEY (a_id)
);

CREATE TABLE b (
b_id integer NOT NULL,
a_ids numeric[] NOT NULL,
PRIMARY KEY (b_id)
);

joel=# ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a(a_id);
ERROR:  foreign key constraint "b_a_ids_fkey" cannot be implemented
DETAIL:  Key column "a_ids" has element type numeric which does not have a 
default btree operator class that's compatible with class "int2_ops".

But you don't get any error message if a_ids is instead int2vector:

CREATE TABLE b (
b_id integer NOT NULL,
a_ids int2vector NOT NULL,
PRIMARY KEY (b_id)
);

ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a(a_id);

It's silently added without any error.

The error first comes when you try to insert data:

INSERT INTO a (a_id) VALUES (1);
INSERT INTO a (a_id) VALUES (2);
INSERT INTO b (b_id, a_ids) VALUES (3, '1 2'::int2vector);

ERROR:  operator does not exist: smallint[] pg_catalog.<@ int2vector
LINE 1: ..."."a" x WHERE ARRAY ["a_id"]::pg_catalog.anyarray OPERATOR(p...
 ^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM 
pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) 
FROM (SELECT 1 FROM ONLY "public"."a" x WHERE ARRAY 
["a_id"]::pg_catalog.anyarray OPERATOR(pg_catalog.<@) $1::pg_catalog.anyarray 
FOR KEY SHARE OF x) z)

This means, as a user, I might not be aware of the vector restriction when 
adding the foreign keys
for my existing schema, and think everything is fine, and not realize there is 
a problem until
new data arrives.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Zhihong Yu]

Hi, Mark:

+   if (ARR_NDIM(arr) != 1 ||
+   ARR_HASNULL(arr) ||
+   ARR_ELEMTYPE(arr) != CHAROID)
+   elog(ERROR, "confreftype is not a 1-D char array");

I think the ARR_HASNULL(arr) condition is not reflected in the error
message.

+* Array foreign keys support only UPDATE/DELETE NO ACTION,
UPDATE/DELETE
+* RESTRICT amd DELETE CASCADE actions

I don't see CASCADE in the if condition that follows the above comment.

+   charreftype;/* FKCONSTR_REF_xxx code */

The code would be FKCONSTR_REF_EACH_ELEMENT and FKCONSTR_REF_PLAIN. I think
you can mention them in the comment.

Cheers

On Wed, Jan 27, 2021 at 11:34 AM Mark Rofail  wrote:

> Hello Joel,
>
>
>> I think you forgot to attach the patch.
>>
> Appears so, sorry about that.
>
> Here it is.
>
> /Mark
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

>
>
> Hey Joel,

I apologize for my rash response, I did not quite understand your example
> at first, it appears the UPDATE statement is neither over the
> referencing nor the referenced columns
>
The v14 fixed the issue where an error would occur by an update statement
that didn't target the refrencing or refrenced columns

Vectors as refrencing columns are not supported and out of scope of this
patch. Try to use arrays.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Wed, Jan 27, 2021, at 20:34, Mark Rofail wrote:
>Here it is. 
>Array-ELEMENT-foreign-key-v15.patch

Thanks.

I've tested it and notice there still seems to be a problem with int2vector?

Reading your previous comment a few messages ago,
it sounds like this was fixed, but perhaps not?

This is the comment that made me think it was fixed:

On Sun, Jan 24, 2021, at 21:46, Mark Rofail wrote:
>Hello again Joel,
>>UPDATE catalog_clone.pg_index SET indkey = '1 2 12345'::int2vector WHERE 
>>indexrelid = 2837;
>>ERROR:  operator does not exist: int2vector pg_catalog.@> smallint[]
>>LINE 1: ...WHERE "attrelid" OPERATOR(pg_catalog.=) $1 AND $2 OPERATOR(p...
>I apologize for my rash response, I did not quite understand your example at 
>first, it appears the UPDATE statement is >neither over the referencing nor 
>the referenced columns, I understand the problem now, please disregard the 
>previous >email. Thank you for this find, please find the fix below
>
>Changelog:
>- v14 (compatible with current master 2021-01-24, commit 
>0c1e8845f28bd07ad381c8b0d6701575d967b88e)

Here is a small test causing that still fails on v15:

CREATE TABLE a (
  a_id smallint NOT NULL,
  PRIMARY KEY (a_id)
);

CREATE TABLE b (
b_id integer NOT NULL,
a_ids int2vector NOT NULL,
PRIMARY KEY (b_id)
);

ALTER TABLE b ADD FOREIGN KEY (EACH ELEMENT OF a_ids) REFERENCES a(a_id);

INSERT INTO a (a_id) VALUES (1);
INSERT INTO a (a_id) VALUES (2);
INSERT INTO b (b_id, a_ids) VALUES (3, '1 2'::int2vector);


ERROR:  operator does not exist: smallint[] pg_catalog.<@ int2vector
LINE 1: ..."."a" x WHERE ARRAY ["a_id"]::pg_catalog.anyarray OPERATOR(p...
 ^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM 
pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) 
FROM (SELECT 1 FROM ONLY "public"."a" x WHERE ARRAY 
["a_id"]::pg_catalog.anyarray OPERATOR(pg_catalog.<@) $1::pg_catalog.anyarray 
FOR KEY SHARE OF x) z)

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Wed, Jan 27, 2021, at 10:58, Mark Rofail wrote:
>Kindly find the updated patch (v15) below

I think you forgot to attach the patch.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hi Joel,

As always, great catch!
Kindly find the updated patch (v15) below

Changelog:
- v15 (compatible with current master 2021-01-27, commit
e42b3c3bd6a9c6233ac4c8a2e9b040367ba2f97c)
* remove "EACH ELEMENT OF" from violation messages
* accommodate tests accordingly

Keep up the good work testing this patch.

/Mark


On Wed, Jan 27, 2021 at 5:11 AM Joel Jacobson  wrote:

> On Tue, Jan 26, 2021, at 12:59, Mark Rofail wrote:
> > Please don't hesitate to give your feedback.
>
> The error message for insert or update violations looks fine:
>
> UPDATE catalog_clone.pg_extension SET extconfig = ARRAY[12345] WHERE oid =
> 10;
> ERROR:  insert or update on table "pg_extension" violates foreign key
> constraint "pg_extension_extconfig_fkey"
> DETAIL:  Key (EACH ELEMENT OF extconfig)=(12345) is not present in table
> "pg_class".
>
> But when trying to delete a still referenced row,
> the column mentioned in the "EACH ELEMENT OF" sentence
> is not the array column, but the column in the referenced table:
>
> DELETE FROM catalog_clone.pg_class WHERE oid = 10;
> ERROR:  update or delete on table "pg_class" violates foreign key
> constraint "pg_extension_extconfig_fkey" on table "pg_extension"
> DETAIL:  Key (EACH ELEMENT OF oid)=(10) is still referenced from table
> "pg_extension".
>
> I think either the "EACH ELEMENT OF" part of the latter error message
> should be dropped,
> or the column name for the array column should be used.
>
> /Joel
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Tue, Jan 26, 2021, at 12:59, Mark Rofail wrote:
> Please don't hesitate to give your feedback.

The error message for insert or update violations looks fine:

UPDATE catalog_clone.pg_extension SET extconfig = ARRAY[12345] WHERE oid = 10;
ERROR:  insert or update on table "pg_extension" violates foreign key 
constraint "pg_extension_extconfig_fkey"
DETAIL:  Key (EACH ELEMENT OF extconfig)=(12345) is not present in table 
"pg_class".

But when trying to delete a still referenced row,
the column mentioned in the "EACH ELEMENT OF" sentence
is not the array column, but the column in the referenced table:

DELETE FROM catalog_clone.pg_class WHERE oid = 10;
ERROR:  update or delete on table "pg_class" violates foreign key constraint 
"pg_extension_extconfig_fkey" on table "pg_extension"
DETAIL:  Key (EACH ELEMENT OF oid)=(10) is still referenced from table 
"pg_extension".

I think either the "EACH ELEMENT OF" part of the latter error message should be 
dropped,
or the column name for the array column should be used.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Joel,

Thank you for your kind words and happy that you benefited from this patch.
We simply assert that the update/delete method used is supported currently
only "NO ACTION" and "RESTRICT", this can be extended in future patches
without rework, just extra logic.
Please don't hesitate to give your feedback.

I would love some help with some performance comparisons if you are up to
it, between Many-to-Many, Foreign Key Arrays and Gin Indexed Foreign Key
Arrays.

/Mark

On Tue, Jan 26, 2021 at 1:51 PM Joel Jacobson  wrote:

> Hi Mark,
>
> On Mon, Jan 25, 2021, at 09:14, Joel Jacobson wrote:
>
> I'll continue testing over the next couple of days and report if I find
> any more oddities.
>
>
> I've continued testing by trying to make full use of this feature in the
> catalog-diff-tool I'm working on.
>
> Today I became aware of a SQL feature that I must confess I've never used
> before,
> that turned out to be useful in my tool, as I then wouldn't need to follow
> FKs
> to do updates manually, but let the database do them automatically.
>
> I'm talking about "ON CASCADE UPDATE", which I see is not supported in the
> patch.
>
> In my tool, I could simplify the code for normal FKs by using ON CASCADE
> UPDATE,
> but will continue to need my hand-written traverse-FKs-recursively code
> for Foreign Key Arrays.
>
> I lived a long SQL life without ever needing ON CASCADE UPDATE,
> so I would definitively vote for Foreign Key Arrays to be added even
> without support for this.
>
> Would you say the patch is written in a way which would allow adding
> support for ON CASCADE UPDATE
> later on mostly by adding code, or would it require a major rewrite?
>
> I hesitated if I should share this with you, since I'm really grateful for
> this feature even without ON CASCADE UPDATE,
> but since this was discovered when testing real-life scenario and not some
> hypothetical example,
> I felt it should be noted that I stumbled upon this during testing.
>
> Again, thank you so much for working on this.
>
> /Joel
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark,

On Mon, Jan 25, 2021, at 09:14, Joel Jacobson wrote:
> I'll continue testing over the next couple of days and report if I find any 
> more oddities.

I've continued testing by trying to make full use of this feature in the 
catalog-diff-tool I'm working on.

Today I became aware of a SQL feature that I must confess I've never used 
before,
that turned out to be useful in my tool, as I then wouldn't need to follow FKs
to do updates manually, but let the database do them automatically.

I'm talking about "ON CASCADE UPDATE", which I see is not supported in the 
patch.

In my tool, I could simplify the code for normal FKs by using ON CASCADE UPDATE,
but will continue to need my hand-written traverse-FKs-recursively code for 
Foreign Key Arrays.

I lived a long SQL life without ever needing ON CASCADE UPDATE,
so I would definitively vote for Foreign Key Arrays to be added even without 
support for this.

Would you say the patch is written in a way which would allow adding support 
for ON CASCADE UPDATE
later on mostly by adding code, or would it require a major rewrite?

I hesitated if I should share this with you, since I'm really grateful for this 
feature even without ON CASCADE UPDATE,
but since this was discovered when testing real-life scenario and not some 
hypothetical example,
I felt it should be noted that I stumbled upon this during testing.

Again, thank you so much for working on this.

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Sun, Jan 24, 2021, at 21:46, Mark Rofail wrote:
>Thank you for this find, please find the fix below

Many thanks for fixing. I can confirm it now works like expected for the 
reported case:

CREATE SCHEMA catalog_clone;
CREATE TABLE catalog_clone.pg_proc AS SELECT * FROM pg_catalog.pg_proc;
CREATE TABLE catalog_clone.pg_type AS SELECT * FROM pg_catalog.pg_type;
ALTER TABLE catalog_clone.pg_type ADD PRIMARY KEY (oid);
ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF proargtypes) 
REFERENCES catalog_clone.pg_type (oid);
UPDATE catalog_clone.pg_proc SET proargtypes = '19 25 12345'::oidvector WHERE 
oid = 79;
ERROR:  insert or update on table "pg_proc" violates foreign key constraint 
"pg_proc_proargtypes_fkey"
DETAIL:  Key (EACH ELEMENT OF proargtypes)=(19 25 12345) is not present in 
table "pg_type".

Excellent!

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

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

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 Herrera39°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)

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Joel,

UPDATE catalog_clone.pg_index SET indkey = '1 2 12345'::int2vector WHERE
> indexrelid = 2837;
> ERROR:  operator does not exist: int2vector pg_catalog.@> smallint[]
> LINE 1: ...WHERE "attrelid" OPERATOR(pg_catalog.=) $1 AND $2 OPERATOR(p...
>
 In your example, you are using the notation  '1 2 12345'::int2vector which
signifies a vector as you have mentioned, however in this patch we use the
array operator @> to help with the indexing, the incompatibility stems from
here.
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.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi again,

I found a similar problem with int2vector columns:

CREATE TABLE catalog_clone.pg_index AS SELECT * FROM pg_catalog.pg_index;
CREATE TABLE catalog_clone.pg_attribute AS SELECT attrelid,attnum FROM 
pg_catalog.pg_attribute;
ALTER TABLE catalog_clone.pg_attribute ADD UNIQUE (attrelid, attnum);
ALTER TABLE catalog_clone.pg_index ADD FOREIGN KEY (indrelid, EACH ELEMENT OF 
indkey) REFERENCES catalog_clone.pg_attribute (attrelid, attnum);

SELECT indexrelid,indrelid,indkey FROM catalog_clone.pg_index WHERE 
cardinality(indkey) > 1 LIMIT 1;
indexrelid | indrelid | indkey
+--+
   2837 | 2836 | 1 2
(1 row)

UPDATE catalog_clone.pg_index SET indkey = '1 2 12345'::int2vector WHERE 
indexrelid = 2837;
ERROR:  operator does not exist: int2vector pg_catalog.@> smallint[]
LINE 1: ...WHERE "attrelid" OPERATOR(pg_catalog.=) $1 AND $2 OPERATOR(p...
 ^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM 
pg_catalog.unnest($2) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) 
FROM (SELECT 1 FROM ONLY "catalog_clone"."pg_attribute" x WHERE "attrelid" 
OPERATOR(pg_catalog.=) $1 AND $2 OPERATOR(pg_catalog. @>) ARRAY["attnum"] FOR 
KEY SHARE OF x) z)

This made me wonder if there are any more of these "vector" columns than the 
oidvector and int2vector.

It appears they are the only two:

SELECT DISTINCT data_type, udt_name from information_schema.columns WHERE 
udt_name LIKE '%vector' ORDER BY 1,2;
data_type |  udt_name
---+
ARRAY | int2vector
ARRAY | oidvector
(2 rows)

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello Joel,

On Sun, Jan 24, 2021 at 12:11 PM Joel Jacobson  wrote:

> HINT:  No operator matches the given name and argument types. You might
> need to add explicit type casts.
> QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM
> pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*)
> FROM (SELECT 1 FROM ONLY "catalog_clone"."pg_type" x WHERE $1
> OPERATOR(pg_catalog. @>) ARRAY["oid"] FOR KEY SHARE OF x) z)
>
> It seems to me there is some type conversion between oidvector and oid[]
> that isn't working properly?
>

This seems to be a type casting problem indeed. The coercion part of the
patch found in "ruleutils.c:11438-11491" is the culprit, is there a cleaner
way to achieve this?
I am aware of the problem and will try to fix it, but I assume this is
because of "Array-containselem-gin-v1.patch" can you give the v13 patch a
try alone?

I will work on a fix and send it soon.

/Mark

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

On Sun, Jan 24, 2021, at 11:21, Mark Rofail wrote:
>This seems to be a type casting problem indeed. The coercion part of the patch 
>found in "ruleutils.c:11438-11491" is the culprit, is there a cleaner way to 
>achieve this?
>I am aware of the problem and will try to fix it, but I assume this is because 
>of "Array-containselem-gin-v1.patch" can you give the v13 patch a try alone?
>I will work on a fix and send it soon.

Actually, the error occurred without the "Array-containselem-gin-v1.patch".

However, I also had the 
"v3-0001-Add-primary-keys-and-unique-constraints-to-system.patch", to get PKs 
on the oids.

So, I've retested to ensure it didn't cause this problem,
this time only 7e57255f6189380d545e1df6a6b38827b213e3da + 
"Array-ELEMENT-foreign-key-v13.patch",
but I still get the same error though.

Exact steps to reproduce error from a clean installation:

$ cd postgresql
$ git checkout 7e57255f6189380d545e1df6a6b38827b213e3da
$ patch -p1 < ~/Array-ELEMENT-foreign-key-v13.patch
$ ./configure --prefix=~/pg-head
$ make -j16
$ make install
$ initdb -D ~/pghead
$ pg_ctl -D ~/pghead -l /tmp/logfile start
$ createdb $USER
$ psql

CREATE SCHEMA catalog_clone;
CREATE TABLE catalog_clone.pg_proc AS SELECT * FROM pg_catalog.pg_proc;
CREATE TABLE catalog_clone.pg_type AS SELECT * FROM pg_catalog.pg_type;
ALTER TABLE catalog_clone.pg_type ADD PRIMARY KEY (oid);
ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF proargtypes) 
REFERENCES catalog_clone.pg_type (oid);
UPDATE catalog_clone.pg_proc SET proargtypes = '19 25 12345'::oidvector WHERE 
oid = 79;
ERROR:  operator does not exist: oidvector pg_catalog.@> oid[]
LINE 1: ... 1 FROM ONLY "catalog_clone"."pg_type" x WHERE $1 OPERATOR(p...
 ^
HINT:  No operator matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:  SELECT 1 WHERE (SELECT pg_catalog.count(DISTINCT y) FROM 
pg_catalog.unnest($1) y) OPERATOR(pg_catalog.=) (SELECT pg_catalog.count(*) 
FROM (SELECT 1 FROM ONLY "catalog_clone"."pg_type" x WHERE $1 
OPERATOR(pg_catalog. @>) ARRAY["oid"] FOR KEY SHARE OF x) z)

/Joel

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Joel Jacobson]

Hi Mark and others,

On Sun, Jan 24, 2021, at 09:22, Mark Rofail wrote:
> Changelog:
> - v13 (compatible with current master 2021-01-24, commit 
> 7e57255f6189380d545e1df6a6b38827b213e3da)
...
> I encourage everyone to take review this patch. After considerable reviews 
> and performance testing, it will be ready for a commitfest.
...
> Array-ELEMENT-foreign-key-v13.patch

This is awesome, a million thanks for this!

I've tested the patch and tried to use it in the pg_catalog-diff-tool I'm 
working on.

I found one problem, described in Test #3 below.

*** Test #1 OK: Multi-key FK on (oid, smallint[])

Find a suitable row to do testing on:

joel=# SELECT oid,conrelid,conkey FROM catalog_clone.pg_constraint WHERE 
cardinality(conkey) > 1 LIMIT 1;
  oid  | conrelid |  conkey
---+--+--
12112 | 1255 | {2,20,3}
(1 row)

Corrupting the row will not be detected since no FK yet:

joel=# UPDATE catalog_clone.pg_constraint SET conkey = '{2,20,3,1234}' WHERE 
oid = 12112;
UPDATE 1

Trying to add a FK now will detect the corrupted row:

joel=# ALTER TABLE catalog_clone.pg_constraint ADD FOREIGN KEY (conrelid, EACH 
ELEMENT OF conkey) REFERENCES catalog_clone.pg_attribute (attrelid, attnum);
ERROR:  insert or update on table "pg_constraint" violates foreign key 
constraint "pg_constraint_conrelid_conkey_fkey"
DETAIL:  Key (conrelid, EACH ELEMENT OF conkey)=(1255, {2,20,3,1234}) is not 
present in table "pg_attribute".

OK, good, we got an error.

Fix row and try again:

joel=# UPDATE catalog_clone.pg_constraint SET conkey = '{2,20,3}' WHERE oid = 
12112;
UPDATE 1
joel=# ALTER TABLE catalog_clone.pg_constraint ADD FOREIGN KEY (conrelid, EACH 
ELEMENT OF conkey) REFERENCES catalog_clone.pg_attribute (attrelid, attnum);
ALTER TABLE

OK, good, FK added.

Thanks to the FK, trying to corrupt the column again will now give an error:

joel=# UPDATE catalog_clone.pg_constraint SET conkey = '{2,20,3,1234}' WHERE 
oid = 12112;
ERROR:  insert or update on table "pg_constraint" violates foreign key 
constraint "pg_constraint_conrelid_conkey_fkey"
DETAIL:  Key (conrelid, EACH ELEMENT OF conkey)=(1255, {2,20,3,1234}) is not 
present in table "pg_attribute".

OK, good, we got an error.

*** Test #2 OK: FK on oid[]

Find a suitable row to do testing on:

joel=# \d catalog_clone.pg_proc
proallargtypes  | oid[] |   |  |

joel=# SELECT oid,proallargtypes FROM catalog_clone.pg_proc WHERE 
cardinality(proallargtypes) > 1 LIMIT 1;
oid  | proallargtypes
--+
3059 | {25,2276}
(1 row)

Corrupting the row will not be detected since no FK yet:

joel=# UPDATE catalog_clone.pg_proc SET proallargtypes = '{25,2276,1234}' WHERE 
oid = 3059;
UPDATE 1

Trying to add a FK now will detect the corrupted row:

joel=# ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF 
proallargtypes) REFERENCES catalog_clone.pg_type (oid);
ERROR:  insert or update on table "pg_proc" violates foreign key constraint 
"pg_proc_proallargtypes_fkey"
DETAIL:  Key (EACH ELEMENT OF proallargtypes)=({25,2276,1234}) is not present 
in table "pg_type".

OK, good, we got an error.

Fix row and try again:

joel=# UPDATE catalog_clone.pg_proc SET proallargtypes = '{25,2276}' WHERE oid 
= 3059;
UPDATE 1
joel=# ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF 
proallargtypes) REFERENCES catalog_clone.pg_type (oid);
ALTER TABLE

OK, good, FK added.

Thanks to the FK, trying to corrupt the column again will now give an error:

joel=# UPDATE catalog_clone.pg_proc SET proallargtypes = '{25,2276,1234}' WHERE 
oid = 3059;
ERROR:  insert or update on table "pg_proc" violates foreign key constraint 
"pg_proc_proallargtypes_fkey"
DETAIL:  Key (EACH ELEMENT OF proallargtypes)=({25,2276,1234}) is not present 
in table "pg_type".

OK, good, we got an error.

*** Test 3 NOT OK: FK on oidvector

Find a suitable row to do testing on:

joel=# \d catalog_clone.pg_proc
proargtypes | oidvector |   |  |

joel=# SELECT oid,proargtypes FROM catalog_clone.pg_proc WHERE 
cardinality(proargtypes) > 1 LIMIT 1;
oid | proargtypes
-+-
  79 | 19 25
(1 row)

Corrupting the row will not be detected since no FK yet:

joel=# UPDATE catalog_clone.pg_proc SET proargtypes = '19 25 12345'::oidvector 
WHERE oid = 79;
UPDATE 1

Trying to add a FK now will detect the corrupted row:

joel=# ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF 
proargtypes) REFERENCES catalog_clone.pg_type (oid);
ERROR:  insert or update on table "pg_proc" violates foreign key constraint 
"pg_proc_proargtypes_fkey"
DETAIL:  Key (EACH ELEMENT OF proargtypes)=(19 25 12345) is not present in 
table "pg_type".

OK, good, we got an error.

Fix row and try again:

joel=# UPDATE catalog_clone.pg_proc SET proargtypes = '19 25'::oidvector WHERE 
oid = 79;
UPDATE 1
joel=# ALTER TABLE catalog_clone.pg_proc ADD FOREIGN KEY (EACH ELEMENT OF 
proargtypes) REFERENCES catalog_clone.pg_type (oid);
ALTER TABLE

OK, good, 

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Zhihong Yu]

Hi, Mark:

+ CREATE TABLE races (

Maybe 'racings' is a better name for the table (signifying the activities).

+   if (ARR_NDIM(arr) != 1 ||
+   ARR_DIMS(arr)[0] != numkeys ||
+   ARR_HASNULL(arr) ||
+   ARR_ELEMTYPE(arr) != CHAROID)
+   elog(ERROR, "confreftype is not a 1-D char array");

For the case of ARR_DIMS(arr)[0] != numkeys (while other conditions are
satisfied), maybe refine the error message mentioning numkeys so that the
user would get better idea.

+* XXX this restriction is pretty annoying, considering the
effort
+* that's been put into the rest of the RI mechanisms to make
them

Is the above going to be addressed in subsequent patches ?

+SplitFKColElems(List *fkcolelems, List **names, List **reftypes)

Maybe add assertion that names and reftypes are not NULL.

+* If a foreign key, the reference semantics for each column
+*/
+   charconfreftype[1];

It would be nice to expand 'semantics' a bit to make the comment clearer.
e.g. mention 'Foreign key column reference semantics codes'

Thanks

On Sat, Jan 23, 2021 at 5:37 AM Mark Rofail  wrote:

> > Changelog (since the last version, v8):
> Below are the versions mentioned in the changelog. v12 is the latest
> version.
>
> /Mark
>
> On Sat, Jan 23, 2021 at 2:34 PM Mark Rofail 
> wrote:
>
>> Greetings,
>>
>> I am trying to revive this patch, Foreign Key Arrays. The original
>> proposal from my GSoC 2017 days can be found here:
>>
>> https://www.postgresql.org/message-id/CAJvoCut7zELHnBSC8HrM6p-R6q-NiBN1STKhqnK5fPE-9%3DGq3g%40mail.gmail.com
>>
>> Disclaimer, I am not the original author of this patch, I picked up this
>> patch in 2017 to migrate the original patch from 2012 and add a GIN index
>> to make it usable as the performance without a GIN index is not usable
>> after 100 rows.
>> The original authors, Tom Lane and Marco Nenciarini, are the ones who did
>> most of the heavy lifting. The original discussion can be found here:
>>
>> https://www.postgresql.org/message-id/flat/1343842863.5162.4.camel%40greygoo.devise-it.lan#1343842863.5162.4.ca...@greygoo.devise-it.lan
>>
>> *In brief, it would be used as follows:*
>> ```sql
>>CREATE TABLE A ( atest1 int PRIMARY KEY, atest2 text );
>>CREATE TABLE B ( btest1 int[], btest2 int );
>> ALTER TABLE B ADD CONSTRAINT FKARRAY FOREIGN KEY (EACH ELEMENT OF
>> btest1) REFERENCES A;
>> ```
>> and now table B references table A as follows:
>> ```sql
>> INSERT INTO B VALUES ('{10,1}', 2);
>> ```
>> where this row references rows 1 and 10 from A without the need of a
>> many-to-many table
>>
>> *Changelog (since the last version, v8):*
>> - v9 (made compatible with Postgresql 11)
>> support `DeconstructFkConstraintRow`
>> support `CloneFkReferencing`
>> support `generate_operator_clause`
>>
>> - v10 (made compatible with Postgresql v12)
>> support `addFkRecurseReferenced` and `addFkRecurseReferencing`
>> support `CloneFkReferenced` and `CloneFkReferencing`
>> migrate tests
>>
>> - v11(make compatible with Postgresql v13)
>> drop `ConvertTriggerToFK`
>> drop `select_common_type_2args` in favor of
>> `select_common_type_from_oids`
>> migrate tests
>>
>> - v12(made compatible with current master, 2021-01-23,
>> commit a8ed6bb8f4cf259b95c1bff5da09a8f4c79dca46)
>> add ELEMENT to `bare_label_keyword`
>> migrate docs
>>
>> *Todo:*
>> - re-add @>> operator which allows comparison of between array and
>> element and returns true iff the element is within the array
>> to allow easier select statements and lower overhead of explicitly
>> creating an array within the SELECT statement.
>>
>> ```diff
>> - SELECT * FROM B WHERE btest1 @> ARRAY[5];
>> + SELECT * FROM B WHERE btest1 @>> 5;
>> ```
>>
>> I apologize it took so long to get a new version here (years). However,
>> this is not the first time I tried to migrate the patch, every time a
>> different issue blocked me from doing so.
>> Reviews and suggestions are most welcome, @Joel Jacobson
>>  please review and test as previously agreed.
>>
>> /Mark
>>
>> On Tue, Oct 2, 2018 at 7:13 AM Michael Paquier 
>> wrote:
>>
>>> 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
>>>
>>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Greetings,

I am trying to revive this patch, Foreign Key Arrays. The original proposal
from my GSoC 2017 days can be found here:
https://www.postgresql.org/message-id/CAJvoCut7zELHnBSC8HrM6p-R6q-NiBN1STKhqnK5fPE-9%3DGq3g%40mail.gmail.com

Disclaimer, I am not the original author of this patch, I picked up this
patch in 2017 to migrate the original patch from 2012 and add a GIN index
to make it usable as the performance without a GIN index is not usable
after 100 rows.
The original authors, Tom Lane and Marco Nenciarini, are the ones who did
most of the heavy lifting. The original discussion can be found here:
https://www.postgresql.org/message-id/flat/1343842863.5162.4.camel%40greygoo.devise-it.lan#1343842863.5162.4.ca...@greygoo.devise-it.lan

*In brief, it would be used as follows:*
```sql
   CREATE TABLE A ( atest1 int PRIMARY KEY, atest2 text );
   CREATE TABLE B ( btest1 int[], btest2 int );
ALTER TABLE B ADD CONSTRAINT FKARRAY FOREIGN KEY (EACH ELEMENT OF
btest1) REFERENCES A;
```
and now table B references table A as follows:
```sql
INSERT INTO B VALUES ('{10,1}', 2);
```
where this row references rows 1 and 10 from A without the need of a
many-to-many table

*Changelog (since the last version, v8):*
- v9 (made compatible with Postgresql 11)
support `DeconstructFkConstraintRow`
support `CloneFkReferencing`
support `generate_operator_clause`

- v10 (made compatible with Postgresql v12)
support `addFkRecurseReferenced` and `addFkRecurseReferencing`
support `CloneFkReferenced` and `CloneFkReferencing`
migrate tests

- v11(make compatible with Postgresql v13)
drop `ConvertTriggerToFK`
drop `select_common_type_2args` in favor of `select_common_type_from_oids`
migrate tests

- v12(made compatible with current master, 2021-01-23,
commit a8ed6bb8f4cf259b95c1bff5da09a8f4c79dca46)
add ELEMENT to `bare_label_keyword`
migrate docs

*Todo:*
- re-add @>> operator which allows comparison of between array and element
and returns true iff the element is within the array
to allow easier select statements and lower overhead of explicitly creating
an array within the SELECT statement.

```diff
- SELECT * FROM B WHERE btest1 @> ARRAY[5];
+ SELECT * FROM B WHERE btest1 @>> 5;
```

I apologize it took so long to get a new version here (years). However,
this is not the first time I tried to migrate the patch, every time a
different issue blocked me from doing so.
Reviews and suggestions are most welcome, @Joel Jacobson
 please
review and test as previously agreed.

/Mark

On Tue, Oct 2, 2018 at 7:13 AM Michael Paquier  wrote:

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

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

I am still having problems rebasing this patch. I can not figure it out on
my own.

On Sun, 27 May 2018 at 5:31 pm, Mark Rofail  wrote:

> issue 1: `pg_constraint.c:564`
> I need to check that `conppeqop` is not null and copy it but I don't know
> how to check its type since its a char*
>
> issue 2: `matview.c:768`
> I need to pass `fkreftype` but I don't have it in the rest of the functIon
>
> On Wed, May 16, 2018 at 10:31 AM, Mark Rofail 
> wrote:
>
>> I was wondering if anyone knows the proper way to write a benchmarking
>> test for the @>> operator. I used the below script in my previous attempt
>> https://gist.github.com/markrofail/174ed370a2f2ac24800fde2fc27e2d38
>> The script does the following steps:
>>
>> 1. Generate Table A with 5 rows
>> 2. Generate Table B with n rows such as:
>> every row of Table B is an array of IDs referencing rows in Table A.
>>
>> The tests we ran previously had Table B up to 1 million rows and the
>> results can be seen here :
>>
>> https://www.postgresql.org/message-id/CAJvoCusMuLnYZUbwTBKt%2Bp6bB9GwiTqF95OsQFHXixJj3LkxVQ%40mail.gmail.com
>>
>> How would we change this so it would be more exhaustive and accurate?
>>
>> Regards,
>> Mark Rofail
>>
>>
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

I was wondering if anyone knows the proper way to write a benchmarking test
for the @>> operator. I used the below script in my previous attempt
https://gist.github.com/markrofail/174ed370a2f2ac24800fde2fc27e2d38
The script does the following steps:

1. Generate Table A with 5 rows
2. Generate Table B with n rows such as:
every row of Table B is an array of IDs referencing rows in Table A.

The tests we ran previously had Table B up to 1 million rows and the
results can be seen here :
https://www.postgresql.org/message-id/CAJvoCusMuLnYZUbwTBKt%2Bp6bB9GwiTqF95OsQFHXixJj3LkxVQ%40mail.gmail.com

How would we change this so it would be more exhaustive and accurate?

Regards,
Mark Rofail

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

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.1351094...@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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Tue, 10 Apr 2018 at 4:17 pm, Alvaro Herrera 
wrote:

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

Yes, you are right. I have been dealing with the code as a single patch
that I almost forgot.

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.

I'd rather just throw away the previous results and start over with new
performance tests. However, like I showed you, it was my first time to
write performance tests. If there's something I could use as a reference
that would help me so much.

>
> 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 apologise, however, I needed more evidence to support my claims.

Regards

On Tue, Apr 10, 2018 at 4:17 PM, Alvaro Herrera 
wrote:

> Mark Rofail wrote:
> > On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera  >
> > 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 Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Mark Rofail wrote:
> On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera 
> 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera 
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?

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

Regards

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Mark Rofail wrote:
> Hello David,
> 
> On Tue, Apr 10, 2018 at 3:47 PM, David Steele  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

Hello David,

On Tue, Apr 10, 2018 at 3:47 PM, David Steele  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.

On 3/7/18 5:43 AM, Alvaro Herrera wrote:

> 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
>
 Any ideas how to perform more exhaustive tests ?

On 3/26/18 4:50 PM, Mark Rofail wrote:

> > On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson
>  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.


as I said before we need a way to obtain a copy of a datum to comply with
the context of  ginqueryarrayextract()

Best Regards

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[David Steele]

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
> *`
> 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  
> 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
da...@pgmasters.net

Re: Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

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
*`
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
 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.

Best.
Mark Rofail

Re: Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[David Steele]

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
da...@pgmasters.net

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index a0e6d7062b..d6ad238cd4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2342,6 +2342,16 @@ SCRAM-SHA-256$iteration 
count:
  
 
  
+  confreftype
+  char[]
+  
+  If a foreign key, the reference semantics for each column:
+   p = plain (simple equality),
+   e = each element of referencing array must have a 
match
+  
+ 
+ 
+   
   conpfeqop
   oid[]
   pg_operator.oid
@@ -2387,6 +2397,12 @@ SCRAM-SHA-256$iteration 
count:
   
 
   
+When confreftype indicates array to scalar
+foreign key reference semantics, the equality operators listed in
+conpfeqop etc are for the array's element type.
+  
+ 
+  
In the case of an exclusion constraint, conkey
is only useful for constraint elements that are simple column references.
For other cases, a zero appears in conkey
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 2b879ead4b..8b95352256 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -882,6 +882,116 @@ CREATE TABLE order_items (

   
 
+  
+   Array Element Foreign Keys
+
+   
+Array Element Foreign Keys
+   
+
+   
+ELEMENT foreign key
+   
+
+   
+constraint
+Array ELEMENT foreign key
+   
+
+   
+constraint
+ELEMENT foreign key
+   
+
+   
+referential integrity
+   
+
+   
+Another option you have with foreign keys is to use a referencing column
+which is an array of elements with the same type (or a compatible one) as
+the referenced column in the related table. This feature is called
+Array Element Foreign Keys and is implemented in 
PostgreSQL
+with EACH ELEMENT OF foreign key constraints, as
+described in the following example:
+
+
+ CREATE TABLE drivers (
+ driver_id integer PRIMARY KEY,
+ first_name text,
+ last_name text
+ );
+
+ CREATE TABLE races (
+ race_id integer PRIMARY KEY,
+ title text,
+ race_day date,
+ final_positions integer[],
+ FOREIGN KEY  (EACH ELEMENT OF final_positions) REFERENCES 
drivers 
+ );
+
+
+The above example uses an array (final_positions) to
+store the results of a race: for each of its elements a referential
+integrity check is enforced on the drivers table. Note
+that (EACH ELEMENT OF ...) REFERENCES is an extension of
+PostgreSQL and it is not included in the SQL standard.
+   
+
+   
+We currently only support the table constraint form.
+   
+
+   
+Even though the most common use case for Array Element Foreign Keys 
constraint is on
+a single column key, you can define a Array Element Foreign Keys 
constraint on a
+group of columns.
+
+
+ CREATE TABLE available_moves (
+ kind text,
+ move text,
+ description text,
+ PRIMARY KEY (kind, move)
+ );
+
+ CREATE TABLE paths (
+ description text,
+ kind text,
+ moves text[],
+ FOREIGN KEY (kind, EACH ELEMENT OF moves) REFERENCES 
available_moves (kind, move)
+ );
+
+ INSERT INTO available_moves VALUES ('relative', 'LN', 'look north');
+ INSERT 

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Andreas Karlsson]

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

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Sat, 3 Feb 2018 at 5:14 pm, Andreas Karlsson  wrote:

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


We really need an initial patch to get accepted and postpone anything that
won't affect the core mechanics of the patch.

So yes, I move towards delaying the introduction of @>> to the patch.

A new patch will be ready by tomorrow.
Besr Regards,
Mark Rofail

>
>

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Wed, Jan 31, 2018 at 1:52 AM, Andreas Karlsson  wrote:

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

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

Best Regards,
Mark Rofail

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Mon, Nov 13, 2017 at 2:41 AM, Andreas Karlsson  wrote:

> 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 [...]
>

Andreas has kindly written the SQL query for us. My problem is generating
it with C code

Best Regards,
Mark Rofail

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

According to the last review, there are still two main issues with the
patch.

On Mon, Oct 30, 2017 at 3:24 AM, Andreas Karlsson  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}');
>
> 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.
>
> 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 ?

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

Best Regards,
Mark Rofail

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From d513b449722b6e848b958584d0c81ca19b289d22 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 12 Jan 2018 16:28:51 -0300
Subject: [PATCH v6] Support foreign keys for array elements

---
 doc/src/sgml/catalogs.sgml|  16 +
 doc/src/sgml/ddl.sgml | 110 
 doc/src/sgml/ref/create_table.sgml| 116 +++-
 src/backend/catalog/heap.c|   1 +
 src/backend/catalog/index.c   |   1 +
 src/backend/catalog/pg_constraint.c   |  14 +-
 src/backend/commands/tablecmds.c  | 138 -
 src/backend/commands/trigger.c|   6 +
 src/backend/commands/typecmds.c   |   1 +
 src/backend/nodes/copyfuncs.c |   1 +
 src/backend/nodes/equalfuncs.c|   1 +
 src/backend/nodes/outfuncs.c  |   1 +
 src/backend/parser/gram.y |  66 ++-
 src/backend/parser/parse_coerce.c |  84 +++
 src/backend/parser/parse_utilcmd.c|   2 +
 src/backend/utils/adt/ri_triggers.c   | 245 +++--
 src/backend/utils/adt/ruleutils.c |  88 ++-
 src/include/catalog/pg_constraint.h   |  27 +-
 src/include/catalog/pg_constraint_fn.h|   1 +
 src/include/nodes/parsenodes.h|   5 +
 src/include/parser/kwlist.h   |   1 +
 src/include/parser/parse_coerce.h |   1 +
 src/test/regress/expected/element_foreign_key.out | 639 ++
 src/test/regress/parallel_schedule|   7 +-
 src/test/regress/serial_schedule  |   1 +
 src/test/regress/sql/element_foreign_key.sql  | 503 +
 26 files changed, 2000 insertions(+), 76 deletions(-)
 create mode 100644 src/test/regress/expected/element_foreign_key.out
 create mode 100644 src/test/regress/sql/element_foreign_key.sql

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 3f02202caf..aa5f9fe21f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2342,6 +2342,16 @@ SCRAM-SHA-256$iteration 
count:
  
 
  
+  confreftype
+  char[]
+  
+  If a foreign key, the reference semantics for each column:
+   p = plain (simple equality),
+   e = each element of referencing array must have a 
match
+  
+ 
+ 
+   
   conpfeqop
   oid[]
   pg_operator.oid
@@ -2387,6 +2397,12 @@ SCRAM-SHA-256$iteration 
count:
   
 
   
+When confreftype indicates array to scalar
+foreign key reference semantics, the equality operators listed in
+conpfeqop etc are for the array's element type.
+  
+ 
+  
In the case of an exclusion constraint, conkey
is only useful for constraint elements that are simple column references.
For other cases, a zero appears in conkey
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index b1167a40e6..5ca685b991 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -882,6 +882,116 @@ CREATE TABLE order_items (

   
 
+  
+   Foreign Key Arrays
+
+   
+Foreign Key Arrays
+   
+
+   
+ELEMENT foreign key
+   
+
+   
+constraint
+Array ELEMENT foreign key
+   
+
+   
+constraint
+ELEMENT foreign key
+   
+
+   
+referential integrity
+   
+
+   
+Another option you have with foreign keys is to use a referencing column
+which is an array of elements with the same type (or a compatible one) as
+the referenced column in the related table. This feature is called
+Foreign Key Arrays and is implemented in PostgreSQL
+with EACH ELEMENT OF foreign key constraints, as
+described in the following example:
+
+
+ CREATE TABLE drivers (
+ driver_id integer PRIMARY KEY,
+ first_name text,
+ last_name text
+ );
+
+ CREATE TABLE races (
+ race_id integer PRIMARY KEY,
+ title text,
+ race_day date,
+ final_positions integer[],
+ FOREIGN KEY  (EACH ELEMENT OF final_positions) REFERENCES 
drivers 
+ );
+
+
+The above example uses an array (final_positions) to
+store the results of a race: for each of its elements a referential
+integrity check is enforced on the drivers table. Note
+that (EACH ELEMENT OF ...) REFERENCES is an extension of
+PostgreSQL and it is not included in the SQL standard.
+   
+
+   
+We currently only support the table constraint form.
+   
+
+   
+Even though the 

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Michael Paquier]

On Mon, Nov 27, 2017 at 10:47 AM, Andreas Karlsson  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