On 11/10/2017 01:47 AM, Mark Rofail wrote:
I am sorry for the late reply
There is no reason for you to be. It did not take you 6 weeks to do a
review. :) Thanks for this new version.
== Functional review
>1) MATCH FULL does not seem to care about NULLS in arrays. In the
example be
Sorry for the very late review.
I like this feature and have needed it myself in the past, and the
current syntax seems pretty good. One could argue for if the syntax
could be generalized to support other things like json and hstore, but I
do not think it would be fair to block this patch due
I have not looked at the issue with the btree_gin tests yet, but here is
the first part of my review.
= Review
This is my first quick review where I just read the documentation and
quickly tested the feature. I will review it more in-depth later.
This is a very useful feature, one which I ha
I have a concern that after supporting UPDATE/DELETE CASCADE, the
performance would drop.
On Thu, Jul 27, 2017 at 12:54 PM, Alexander Korotkov
wrote:
>
> I wonder how may RI trigger work so fast if it has to do some job besides
> index search with no results?
>
Best Regards,
Mark Rofail
Alexander Korotkov writes:
> On Mon, Aug 14, 2017 at 2:09 PM, Mark Rofail wrote:
>> I think we should cast the operands in the RI queries fired as follows
>> 1. we get the array type from the right operand
>> 2. compare the two array type and see which type is more "general" (as to
>> which shoul
On Mon, Aug 14, 2017 at 2:09 PM, Mark Rofail wrote:
> On Tue, Aug 8, 2017 at 3:24 PM, Alexander Korotkov
> wrote:
>
>> On Tue, Aug 8, 2017 at 4:12 PM, Mark Rofail
>> wrote:
>>
>>> On Tue, Aug 8, 2017 at 2:25 PM, Alexander Korotkov >> > wrote:
>>>
>> GROUP BY would also use default btree/hash op
On Tue, Aug 8, 2017 at 4:12 PM, Mark Rofail wrote:
> On Tue, Aug 8, 2017 at 2:25 PM, Alexander Korotkov
> wrote:
>>
>> Do we already assume that default btree opclass for array element type
>> matches PK opclass when using @>> operator on UPDATE/DELETE of referenced
>> table?
>>
> I believe so,
On Tue, Aug 8, 2017 at 2:25 PM, Alexander Korotkov
wrote:
>
> Do we already assume that default btree opclass for array element type
> matches PK opclass when using @>> operator on UPDATE/DELETE of referenced
> table?
>
I believe so, since it's a polymorphic function.
> If so, we don't introduce
On Sat, Aug 5, 2017 at 11:36 PM, Mark Rofail wrote:
> This is the query fired upon any UPDATE/DELETE for RI checks:
>
> SELECT 1 FROM ONLY x WHERE pkatt1 = $1 [AND ...] FOR KEY SHARE
> OF x
>
> in the case of foreign key arrays, it's wrapped in this query:
>
> SELECT 1 WHERE
> (SELECT count
This is the query fired upon any UPDATE/DELETE for RI checks:
SELECT 1 FROM ONLY x WHERE pkatt1 = $1 [AND ...] FOR KEY SHARE OF
x
in the case of foreign key arrays, it's wrapped in this query:
SELECT 1 WHERE
(SELECT count(DISTINCT y) FROM unnest($1) y)
= (SELECT count(*) FROM () z)
Th
To better understand a limitation I ask 5 questions
What is the limitation?
Why is there a limitation?
Why is it a limitation?
What can we do?
Is it feasible?
Through some reading:
*What is the limitation?*
presupposes that count(distinct y) has exactly the same notion of equality
that the PK un
On Mon, Jul 31, 2017 at 5:18 PM, Alvaro Herrera
wrote:
> Tom Lane wrote:
> > Alvaro Herrera writes:
> > > ... However, when you create an index, you can
> > > indicate which operator class to use, and it may not be the default
> one.
> > > If a different one is chosen at index creation time, th
Tom Lane wrote:
> Alvaro Herrera writes:
> > ... However, when you create an index, you can
> > indicate which operator class to use, and it may not be the default one.
> > If a different one is chosen at index creation time, then a query using
> > COUNT(distinct) will do the wrong thing, because
Alvaro Herrera writes:
> ... However, when you create an index, you can
> indicate which operator class to use, and it may not be the default one.
> If a different one is chosen at index creation time, then a query using
> COUNT(distinct) will do the wrong thing, because DISTINCT will select
> an
Mark Rofail wrote:
> These are limitations of the patch ordered by importance:
>
> ✗ presupposes that count(distinct y) has exactly the same notion of
> equality that the PK unique index has. In reality, count(distinct) will
> fall back to the default btree opclass for the array element type.
Ope
These are limitations of the patch ordered by importance:
✗ presupposes that count(distinct y) has exactly the same notion of
equality that the PK unique index has. In reality, count(distinct) will
fall back to the default btree opclass for the array element type.
- Supported actions:
✔ NO ACTIO
On Fri, Jul 28, 2017 at 1:19 PM, Erik Rijkers wrote:
> One small thing while building docs:
>
> $ cd doc/src/sgml && make html
> osx -wall -wno-unused-param -wno-empty -wfully-tagged -D . -D . -x lower
> postgres.sgml >postgres.xml.tmp
> osx:ref/create_table.sgml:960:100:E: document type does no
On 2017-07-27 21:08, Mark Rofail wrote:
On Thu, Jul 27, 2017 at 7:15 PM, Erik Rijkers wrote:
It would help (me at least) if you could be more explicit about what
exactly each instance is.
I apologize, I thought it was clear through the context.
Thanks a lot. It's just really easy for tes
On Thu, Jul 27, 2017 at 7:30 PM, Alexander Korotkov
wrote:
> Oh, ok. I missed that.
>>
> Could you remind me why don't we have DELETE CASCADE? I understand that
> UPDATE CASCADE is problematic because it's unclear which way should we
> delete elements from array. But what about DELETE CASCADE?
On Thu, Jul 27, 2017 at 7:15 PM, Erik Rijkers wrote:
> It would help (me at least) if you could be more explicit about what
> exactly each instance is.
>
I apologize, I thought it was clear through the context.
I meant by the original patch is all the work done before my GSoC project.
The lates
On Thu, Jul 27, 2017 at 3:07 PM, Mark Rofail wrote:
> On Thu, Jul 27, 2017 at 12:54 PM, Alexander Korotkov > wrote:
>>
>> How many rows of FK table were referencing the PK table row you're
>> updating/deleting.
>> I wonder how may RI trigger work so fast if it has to do some job besides
>> index
On 2017-07-27 02:31, Mark Rofail wrote:
I have written some benchmark test.
It would help (me at least) if you could be more explicit about what
exactly each instance is.
Apparently there is an 'original patch': is this the original patch by
Marco Nenciarini?
Or is it something you posted
On Thu, Jul 27, 2017 at 12:54 PM, Alexander Korotkov
wrote:
>
> How many rows of FK table were referencing the PK table row you're
> updating/deleting.
> I wonder how may RI trigger work so fast if it has to do some job besides
> index search with no results?
>
The problem here is that the only to
On Thu, Jul 27, 2017 at 3:31 AM, Mark Rofail wrote:
> I have written some benchmark test.
>
> With two tables a PK table with 5 rows and an FK table with growing row
> count.
>
> Once triggering an RI check
> at 10 rows,
> 100 rows,
> 1,000 rows,
> 10,000 rows,
> 100,000 rows and
> 1,000,000 rows
On 2017-07-24 23:31, Mark Rofail wrote:
On Mon, Jul 24, 2017 at 11:25 PM, Erik Rijkers wrote:
This patch doesn't apply to HEAD at the moment ( e2c8100e6072936 ).
My bad, I should have mentioned that the patch is dependant on the
original
patch.
Here is a *unified* patch that I just tested
On 2017-07-24 23:08, Mark Rofail wrote:
Here is the new Patch with the bug fixes and the New Patch with the
Index
in place performance results.
I just want to point this out because I still can't believe the
numbers. In
reference to the old patch:
The new patch without the index suffers a 41.
It certainly is, thank you for the heads up. I included a note to encourage
the user to index the referencing column instead.
On Sun, Jul 23, 2017 at 4:41 AM, Robert Haas wrote:
>
> This is a jumbo king-sized can of worms, and even a very experienced
> contributor would likely find it extremely d
>
> However, there is a bug that prevented me from testing the third scenario,
> I assume there's an issue of incompatible types problem since the right
> operand type is anyelement and the supporting procedures expect anyarray.
> I am working on debugging it right now.
>
I have also solved the bu
On Sat, Jul 22, 2017 at 5:50 PM, Mark Rofail wrote:
> so personally I don't think we should leave creating a GIN index up to the
> user, it should be automatically generated instead.
I can certainly understand why you feel that way, but trying to do
that in your patch is just going to get your pa
On Wed, Jul 19, 2017 at 11:08 PM, Alvaro Herrera
wrote:
> I'm not entirely sure what's the best way to deal with the polymorphic
> problem, but on the other hand as Robert says downthread maybe we
> shouldn't be solving it at this stage anyway. So let's step back a bit,
> get a patch that works
On Wed, Jul 19, 2017 at 10:08 PM, Alvaro Herrera
wrote:
> So let's step back a bit,
> get a patch that works for the case where the types match on both sides
> of the FK, then we review that patch; if all is well, we can discuss the
> other problem as a stretch goal.
Agreed. This should be a fu
Mark Rofail wrote:
> On Tue, Jul 18, 2017 at 11:14 PM, Alvaro Herrera
> wrote:
> >
> > Why did we add an operator and not a support
> > procedure?
>
> I thought the support procedures were constant within an opclass.
Uhh ... I apologize but I think I was barking at the wrong tree. I was
thinkin
On Wed, Jul 19, 2017 at 2:29 PM, Mark Rofail wrote:
> On Wed, Jul 19, 2017 at 7:28 PM, Robert Haas wrote:
>>
>> Why do we have to solve that limitation?
>
> Since the regress test labled element_foreing_key fails now that I made the
> RI queries utilise @(anyarray, anyelement), that means it's no
On Wed, Jul 19, 2017 at 7:28 PM, Robert Haas wrote:
> Why do we have to solve that limitation?
Since the regress test labled element_foreing_key fails now that I made the
RI queries utilise @(anyarray, anyelement), that means it's not functioning
as it is meant to be.
On Wed, Jul 19, 2017 at 8:08 AM, Mark Rofail wrote:
> To summarise, the options we have to solve the limitation of the @>(anyarray
> , anyelement) where it produces the following error: operator does not
> exist: integer[] @> smallint
Why do we have to solve that limitation?
--
Robert Haas
Ente
*To summarise,* the options we have to solve the limitation of the
@>(anyarray , anyelement) where it produces the following error: operator
does not exist: integer[] @> smallint
*Option 1: *Multiple Operators
Have separate operators for every combination of datatypes instead of a
single polymorph
On Tue, Jul 18, 2017 at 11:14 PM, Alvaro Herrera
wrote:
>
> Why did we add an operator and not a support
> procedure?
I thought the support procedures were constant within an opclass. They
implement the mandotary function required of an opclass. I don't see why we
would need to implement new one
Alexander Korotkov wrote:
> The problem is that you need to have not only opclass entries for the
> operators, but also operators themselves. I.e. separate operators for
> int4[] @>> int8, int4[] @>> int4, int4[] @>> int2, int4[] @>> numeric. You
> tried to add multiple pg_amop rows for single o
Mark Rofail wrote:
> On Tue, 18 Jul 2017 at 7:43 pm, Alexander Korotkov
> wrote:
>
> > separate operators for int4[] @>> int8, int4[] @>> int4, int4[] @>> int2,
> > int4[] @>> numeric.
> >
>
> My only comment on the separate operators is its high maintenance. Any new
> datatype introduced a co
On Tue, 18 Jul 2017 at 7:43 pm, Alexander Korotkov
wrote:
> separate operators for int4[] @>> int8, int4[] @>> int4, int4[] @>> int2,
> int4[] @>> numeric.
>
My only comment on the separate operators is its high maintenance. Any new
datatype introduced a corresponding operator should be create
On Tue, 18 Jul 2017 at 7:43 pm, Alexander Korotkov
wrote:
> On T upue, Jul 18, 2017 at 2:24 AM, Mark Rofail
> wrote:
>
>> On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera <
>> alvhe...@2ndquadrant.com> wrote:
>>>
>>> We have one opclass for each type combination -- int4 to int2, int4 to
>>> int4
On Tue, Jul 18, 2017 at 2:24 AM, Mark Rofail wrote:
> On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera > wrote:
>>
>> We have one opclass for each type combination -- int4 to int2, int4 to
>> int4, int4 to int8, etc. You just need to add the new strategy to all
>> the opclasses.
>
>
> I tried
There is a generic definition for any array added as part of
https://commitfest.postgresql.org/10/708/ (it may be the reason for the
duplicate error). I am not sure what your change is but I would review the
above just in case. There is also a defect with a misleading error that is
still being trig
On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera
wrote:
>
> We have one opclass for each type combination -- int4 to int2, int4 to
> int4, int4 to int8, etc. You just need to add the new strategy to all
> the opclasses.
I tried this approach by manually declaring the operator multiple of time
Mark Rofail wrote:
> On Wed, Jul 12, 2017 at 2:30 PM, Mark Rofail wrote:
>
> > On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera > > wrote:
> >>
> >> We have one opclass for each type combination -- int4 to int2, int4 to
> >> int4, int4 to int8, etc. You just need to add the new strategy to all
On Wed, Jul 12, 2017 at 2:30 PM, Mark Rofail wrote:
> On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera > wrote:
>>
>> We have one opclass for each type combination -- int4 to int2, int4 to
>> int4, int4 to int8, etc. You just need to add the new strategy to all
>> the opclasses.
>>
>
> Can you
On Wed, Jul 12, 2017 at 12:53 AM, Alvaro Herrera
wrote:
>
> We have one opclass for each type combination -- int4 to int2, int4 to
> int4, int4 to int8, etc. You just need to add the new strategy to all
> the opclasses.
>
Can you clarify this solution ? I think another solution would be external
Mark Rofail wrote:
>- now the RI checks utilise the @>(anyarray, anyelement)
> - however there's a small problem:
> operator does not exist: integer[] @> smallint
> I assume that external casting would be required here. But how can I
> downcast smallint to integer or in
here are the modifications to ri_triggers.c
On Wed, Jul 12, 2017 at 12:26 AM, Mark Rofail
wrote:
>
> *What I did *
>
>- now the RI checks utilise the @>(anyarray, anyelement)
>
> Best Regards,
> Mark Rofail
>
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers
On Sun, Jul 9, 2017 at 7:42 PM, Alexander Korotkov
wrote:
> We may document that GIN index is required to accelerate RI queries for
> array FKs. And users are encouraged to manually define them.
> It's also possible to define new option when index on referencing
> column(s) would be created auto
On Sun, Jul 9, 2017 at 1:11 PM, Mark Rofail wrote:
> On Sun, Jul 9, 2017 at 2:38 AM, Alexander Korotkov
> wrote:
>
>> Could you, please, specify idea of what you're implementing in more
>> detail?
>>
>
> Ultimatley we would like an indexed scan instead of a sequential scan, so
> I thought we nee
On Sun, Jul 9, 2017 at 2:38 AM, Alexander Korotkov
wrote:
> Could you, please, specify idea of what you're implementing in more
> detail?
>
Ultimatley we would like an indexed scan instead of a sequential scan, so I
thought we needed to index the FK array columns first.
On Sun, Jul 9, 2017 at 2:35 AM, Mark Rofail wrote:
> * What I am working on*
>
>- since we want to create an index on the referencing column, I am
>working on firing a 'CREATE INDEX' query programatically right after
>the 'CREATE TABLE' query
>- The problem I ran into is how to sp
* What I am working on*
- since we want to create an index on the referencing column, I am
working on firing a 'CREATE INDEX' query programatically right after the
'CREATE TABLE' query
- The problem I ran into is how to specify my Strategy (
GinContainsElemStrategy) within the CR
To make the queries fired by the RI triggers GIN indexed. We need to ‒ as
Tom Lane has previously suggested[1] ‒ to replace the query
SELECT 1 FROM ONLY fktable x WHERE $1 = ANY (fkcol) FOR SHARE OF x;
with
SELECT 1 FROM ONLY fktable x WHERE ARRAY[$1] <@ fkcol FOR SHARE OF x;
but since we have
Mark Rofail wrote:
> On Mon, Jun 26, 2017 at 6:44 PM, Alexander Korotkov
> wrote:
>
> > Have you met any particular problem here? Or is it just a lot of
> > mechanical work?
> >
>
> Just A LOT of mechanictal work, thankfully. The patch is now rebased and
> all regress tests have passed (even th
On Mon, Jun 26, 2017 at 2:26 AM, Mark Rofail wrote:
> *What I did:*
>
>
>
>- read into the old patch but couldn't apply it since it's quite old.
>It needs to be rebased and that's what I am working on. It's a lot of
>work.
> - incomplete patch can be found attached here
>
> Hav
*What I did:*
- read into the old patch but couldn't apply it since it's quite old. It
needs to be rebased and that's what I am working on. It's a lot of work.
- incomplete patch can be found attached here
*Bugs*
- problem with the @>(anyarray, anyelement) opertator: if for exa
Mark Rofail wrote:
> Okay, so major breakthrough.
>
> *Updates:*
>
>- The operator @>(anyarray, anyelement) is now functional
> - The segmentation fault was due to applying PG_FREE_IF_COPY on a
> datum when it should only be applied on TOASTed inputs
> - The only problem now
Okay, so major breakthrough.
*Updates:*
- The operator @>(anyarray, anyelement) is now functional
- The segmentation fault was due to applying PG_FREE_IF_COPY on a
datum when it should only be applied on TOASTed inputs
- The only problem now is if for example you apply the op
On Sun, Jun 18, 2017 at 12:41 AM, Mark Rofail
wrote:
> *Questions:*
>
>- I'd like to check that anyelem and anyarray have the same element
>type. but anyelem is obtained from PG_FUNCTION_ARGS as a Datum. How
>can I make such a check?
>
>
As I know, it's implicitly checked during query
*Updates till now:*
- added a record to pg_proc (src/include/catalog/pg_proc.h)
- modified opr_sanity regression check expected results
- implemented a low-level function called `array_contains_elem` as an
equivalent to `array_contain_compare` but accepts anyelement instead of
anya
• After finding the arraycontains function, I implemented
arraycontainselem that corresponds to the operator @<(anyarray,
anyelem)
◦ Please read the attached patch file to view my progress.
• In addition to src/backend/utils/adt/arrayfuncs.c where I
implemented arraycontainselem.
◦
Hi, Mark!
On Tue, May 30, 2017 at 2:18 AM, Mark Rofail wrote:
> rhaas=# select oid, * from pg_opfamily where opfmethod = 2742;
>> oid | opfmethod |opfname | opfnamespace | opfowner
>> --+---++--+--
>> 2745 | 2742 | array_ops |
>
> rhaas=# select oid, * from pg_opfamily where opfmethod = 2742;
> oid | opfmethod |opfname | opfnamespace | opfowner
> --+---++--+--
> 2745 | 2742 | array_ops | 11 | 10
> 3659 | 2742 | tsvector_ops |
On Mon, May 22, 2017 at 7:51 PM, Mark Rofail wrote:
> Cloned the git repo found @ https://github.com/postgres/postgres and
> identified the main two files I will be concerned with. (I know I may need
> to edit other files but these seem to where I will spend most of my summer)
>
> src/backend/comm
66 matches
Mail list logo
