public inbox for [email protected]  

help / color / mirror / Atom feed

From: Shayon Mukherjee <[email protected]>
To: David Rowley <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH] Proposal to Enable/Disable Index using ALTER INDEX
Date: Mon, 23 Sep 2024 07:07:11 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAApHDvpUNu=iVcdJ74sypvgeaCF+tfpyb8VRhZaF7DTd1xVr7g@mail.gmail.com>
References: <CANqtF-oXKe0M=0QOih6H+sZRjE2BWAbkW_1+9nMEAMLxUJg5jA@mail.gmail.com>
	<CAApHDvoVL57C-pWtBO_1bvzTHRvSGN3BY_5us5F9mDbKdqG=YA@mail.gmail.com>
	<ZuBh1pSlyeAnRJYp@nathan>
	<CAApHDvo5SNiqaai4Xbg1nWn3gKxBcX_8=B1BJFfsLrE9SNyjUw@mail.gmail.com>
	<CANqtF-oBaBtRfw9O7GAoHN3nNEZQYsW3oaGfD+wJfG8R29nZYw@mail.gmail.com>
	<CAApHDvpUNu=iVcdJ74sypvgeaCF+tfpyb8VRhZaF7DTd1xVr7g@mail.gmail.com>

Hi David,

Thank you so much for the review and pointers. I totally missed expression indexes. I am going to do another proper pass along with your feedback and follow up with an updated patch and any questions. 

Excited to be learning so much about the internals. 
Shayon

> On Sep 22, 2024, at 6:44 PM, David Rowley <[email protected]> wrote:
> 
> On Mon, 23 Sept 2024 at 05:43, Shayon Mukherjee <[email protected]> wrote:
>> - Modified get_index_paths() and build_index_paths() to exclude disabled
>>  indexes from consideration during query planning.
> 
> There are quite a large number of other places you also need to modify.
> 
> Here are 2 places where the index should be ignored but isn't:
> 
> 1. expression indexes seem to still be used for statistical estimations:
> 
> create table b as select generate_series(1,1000)b;
> create index on b((b%10));
> analyze b;
> explain select distinct b%10 from b;
> -- HashAggregate  (cost=23.00..23.12 rows=10 width=4)
> 
> alter index b_expr_idx disable;
> explain select distinct b%10 from b;
> -- HashAggregate  (cost=23.00..23.12 rows=10 width=4) <-- should be 1000 rows
> 
> drop index b_expr_idx;
> explain select distinct b%10 from b;
> -- HashAggregate  (cost=23.00..35.50 rows=1000 width=4)
> 
> 2. Indexes seem to still be used for join removals.
> 
> create table c (c int primary key);
> explain select c1.* from c c1 left join c c2 on c1.c=c2.c; --
> correctly removes join.
> alter index c_pkey disable;
> explain select c1.* from c c1 left join c c2 on c1.c=c2.c; -- should
> not remove join.
> 
> Please carefully look over all places that RelOptInfo.indexlist is
> looked at and consider skipping disabled indexes. Please also take
> time to find SQL that exercises each of those places so you can verify
> that the behaviour is correct after your change. This is also a good
> way to learn exactly all cases where indexes are used. Using this
> method would have led you to find places like
> rel_supports_distinctness(), where you should be skipping disabled
> indexes.
> 
> The planner should not be making use of disabled indexes for any
> optimisations at all.
> 
>> - catversion.h is updated with a new CATALOG_VERSION_NO to reflect change in pg_index
>>  schema.
> 
> Please leave that up to the committer. Patch authors doing this just
> results in the patch no longer applying as soon as someone commits a
> version bump.
> 
> Also, please get rid of these notices. The command tag serves that
> purpose. It's not interesting that the index is already disabled.
> 
> # alter index a_pkey disable;
> NOTICE:  index "a_pkey" is now disabled
> ALTER INDEX
> # alter index a_pkey disable;
> NOTICE:  index "a_pkey" is already disabled
> ALTER INDEX
> 
> I've only given the code a very quick glance. I don't quite understand
> why you're checking the index is enabled in create_index_paths() and
> get_index_paths(). I think the check should be done only in
> create_index_paths().  Primarily, you'll find code such as "if
> (index->indpred != NIL && !index->predOK)" in the locations you need
> to consider skipping the disabled index. I think your new code should
> be located very close to those places or perhaps within the same if
> condition unless it makes it overly complex for the human reader.
> 
> I think the documents should also mention that disabling an index is a
> useful way to verify an index is not being used before dropping it as
> the index can be enabled again at the first sign that performance has
> been effected. (It might also be good to mention that checking
> pg_stat_user_indexes.idx_scan should be the first port of call when
> checking for unused indexes)
> 
> David

view thread (6+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Proposal to Enable/Disable Index using ALTER INDEX
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox