Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\))
Subject: Re: [PATCH] Proposal to Enable/Disable Index using ALTER INDEX
From: Shayon Mukherjee <shayonj@gmail.com>
In-Reply-To: <CAApHDvpUNu=iVcdJ74sypvgeaCF+tfpyb8VRhZaF7DTd1xVr7g@mail.gmail.com>
Date: Mon, 23 Sep 2024 07:07:11 -0400
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <7529D17D-6620-466F-A78D-CD2D5346D1B0@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>
To: David Rowley <dgrowleyml@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/7529D17D-6620-466F-A78D-CD2D5346D1B0%40gmail.com>
Precedence: bulk

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

Excited to be learning so much about the internals.=20
Shayon

> On Sep 22, 2024, at 6:44 PM, David Rowley <dgrowleyml@gmail.com> =
wrote:
>=20
> On Mon, 23 Sept 2024 at 05:43, Shayon Mukherjee <shayonj@gmail.com> =
wrote:
>> - Modified get_index_paths() and build_index_paths() to exclude =
disabled
>>  indexes from consideration during query planning.
>=20
> There are quite a large number of other places you also need to =
modify.
>=20
> Here are 2 places where the index should be ignored but isn't:
>=20
> 1. expression indexes seem to still be used for statistical =
estimations:
>=20
> 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=3D23.00..23.12 rows=3D10 width=3D4)
>=20
> alter index b_expr_idx disable;
> explain select distinct b%10 from b;
> -- HashAggregate  (cost=3D23.00..23.12 rows=3D10 width=3D4) <-- should =
be 1000 rows
>=20
> drop index b_expr_idx;
> explain select distinct b%10 from b;
> -- HashAggregate  (cost=3D23.00..35.50 rows=3D1000 width=3D4)
>=20
> 2. Indexes seem to still be used for join removals.
>=20
> create table c (c int primary key);
> explain select c1.* from c c1 left join c c2 on c1.c=3Dc2.c; --
> correctly removes join.
> alter index c_pkey disable;
> explain select c1.* from c c1 left join c c2 on c1.c=3Dc2.c; -- should
> not remove join.
>=20
> 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.
>=20
> The planner should not be making use of disabled indexes for any
> optimisations at all.
>=20
>> - catversion.h is updated with a new CATALOG_VERSION_NO to reflect =
change in pg_index
>>  schema.
>=20
> 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.
>=20
> Also, please get rid of these notices. The command tag serves that
> purpose. It's not interesting that the index is already disabled.
>=20
> # 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
>=20
> 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 !=3D 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.
>=20
> 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)
>=20
> David