Reply-To: jesper.pedersen@redhat.com
Subject: Re: Index Skip Scan
To: David Rowley <david.rowley@2ndquadrant.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>,
 Floris Van Nee <florisvannee@optiver.com>,
 Thomas Munro <thomas.munro@gmail.com>, James Coleman <jtc331@gmail.com>,
 Rafia Sabih <rafia.pghackers@gmail.com>, Jeff Janes <jeff.janes@gmail.com>,
 Peter Geoghegan <pg@bowt.ie>, Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Thomas Munro <thomas.munro@enterprisedb.com>,
 Bhushan Uparkar <bhushan.uparkar@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>
References: <2685f020-f302-9bf6-1b5c-ebc2bcebc635@2ndquadrant.com>
 <CA+q6zcVnRRqktw4cg2Vddy3S+VY3Tsmit0kausYtctvsTum7tA@mail.gmail.com>
 <1559763453006.85615@Optiver.com> <1559763570329.85227@Optiver.com>
 <4d469bf2-49ff-bc3d-a428-6c7b5d87d526@redhat.com>
 <CAKJS1f86FgODuUnHiQ25RKeuES4qTqeNxm1QbqJWrBoZxVGLiQ@mail.gmail.com>
 <CA+q6zcXoYjCtyU-jFU_=p2DL_K3BHbBEEbJHaJTxdvRX3cU-Sg@mail.gmail.com>
 <CA+q6zcVVigO_1KRBxvzKfnR593CEE7FKR-bwP-XcH6AVHOo4+g@mail.gmail.com>
 <a8b5c2d9-7b23-e12b-fc6b-eee63dc8dacf@redhat.com>
 <CA+hUKGKo30N5VNuRWhDuMGVZ3hTcv4J5RGXe286GRJZLk_jBYQ@mail.gmail.com>
 <CAKJS1f_v5X1Sb42ShOhsLzuMA6iX52PcBwpf4P-L9H_GDYA3-Q@mail.gmail.com>
 <CA+hUKG+oY0u1YON4KHikmX0aO6tFBOF_LzZQOTX8bVUbe1j5mA@mail.gmail.com>
 <CA+q6zcWC2BgJwFf31xQUZ+FbP+J0zR8fM0sGVMuCYJDbAW_GAw@mail.gmail.com>
 <1562769595280.16860@Optiver.com>
 <CA+q6zcU66Gx954SjOT5nZRRbYG-gVVNJx5NSbn3Gn2aKwtaX1Q@mail.gmail.com>
 <CAKJS1f9xBUCdqj4hT3gc0Vp=BimUnRoQe2ogDEGBhuLv+txz_g@mail.gmail.com>
From: Jesper Pedersen <jesper.pedersen@redhat.com>
Organization: Red Hat
Message-ID: <994cbf82-333c-56d7-2860-9c8c85aba790@redhat.com>
Date: Thu, 11 Jul 2019 12:53:54 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: 
 <CAKJS1f9xBUCdqj4hT3gc0Vp=BimUnRoQe2ogDEGBhuLv+txz_g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi David,

On 7/11/19 7:38 AM, David Rowley wrote:
> The UniqueKeys idea is quite separate from pathkeys.  Currently, a
> Path can have a List of PathKeys which define the order that the
> tuples will be read from the Plan node that's created from that Path.
> The idea with UniqueKeys is that a Path can also have a non-empty List
> of UniqueKeys to define that there will be no more than 1 row with the
> same value for the Paths UniqueKey column/exprs.
> 
> As of now, if you look at standard_qp_callback() sets
> root->query_pathkeys, the idea here would be that the callback would
> also set a new List field named "query_uniquekeys" based on the
> group_pathkeys when non-empty and !root->query->hasAggs, or by using
> the distinct clause if it's non-empty. Then in build_index_paths()
> around the call to match_pathkeys_to_index() we'll probably also want
> to check if the index can support UniqueKeys that would suit the
> query_uniquekeys that were set during standard_qp_callback().
> 
> As for setting query_uniquekeys in standard_qp_callback(), this should
> be simply a matter of looping over either group_pathkeys or
> distinct_pathkeys and grabbing the pk_eclass from each key and making
> a canonical UniqueKey from that. To have these canonical you'll need
> to have a new field in PlannerInfo named canon_uniquekeys which will
> do for UniqueKeys what canon_pathkeys does for PathKeys. So you'll
> need an equivalent of make_canonical_pathkey() in uniquekey.c
> 
> With this implementation, the code that the patch adds in
> create_distinct_paths() can mostly disappear. You'd only need to look
> for a path that uniquekeys_contained_in() matches the
> root->query_uniquekeys and then just leave it to
> set_cheapest(distinct_rel); to pick the cheapest path.
> 
> It would be wasted effort to create paths with UniqueKeys if there's
> multiple non-dead base rels in the query as the final rel in
> create_distinct_paths would be a join rel, so it might be worth
> checking that before creating such index paths in build_index_paths().
> However, down the line, we could carry the uniquekeys forward into the
> join paths if the join does not duplicate rows from the other side of
> the join... That's future stuff though, not for this patch, I don't
> think.
> 
> I think a UniqueKey can just be a struct similar to PathKey, e.g be
> located in pathnodes.h around where PathKey is defined.  Likely we'll
> need a uniquekeys.c file that has the equivalent to
> pathkeys_contained_in() ... uniquekeys_contained_in(), which return
> true if arg1 is a superset of arg2 rather than check for one set being
> a prefix of another. As you mention above: UniqueKeys { x, y } ==
> UniqueKeys { y, x }.  That superset check could perhaps be optimized
> by sorting UniqueKey lists in memory address order, that'll save
> having a nested loop, but likely that's not going to be required for a
> first cut version.  This would work since you'd want UniqueKeys to be
> canonical the same as PathKeys are (Notice that compare_pathkeys()
> only checks memory addresses of pathkeys and not equals()).
> 
> I think the UniqueKey struct would only need to contain an
> EquivalenceClass field. I think all the other stuff that's in PathKey
> is irrelevant to UniqueKey.
> 

Thanks for the feedback ! I'll work on these changes for the next 
uniquekey patch.

Best regards,
  Jesper