Re: Proposal to Enable/Disable Index using ALTER INDEX (with patch)

On Mon, Dec 30, 2024 at 2:56 PM Sami Imseih <samimseih(at)gmail(dot)com> wrote:

> > Rebased with the latest master as well.
>
> Hi,
>
> This is a great, long needed feature. Thanks for doing this.
>
> I am late to this thread, but I took a look at the current patch
> and have some comments as I continue to look.
>
>
Thank you so much for such detailed and useful feedback.

> 1/
> instead of
>
> + If true, the index is currently enabled and should be used for
> queries.
> + If false, the index is disabled and should not be used for queries,
>
> how about?
> ....
>

It all makes sense to me. Thank you for the pointers to the docs and
existing practices as well. I have updated the patch accordingly.

>
> "indisvalid" does not control constraint enforcement in this case. It will
> be
> "indisready" being set to false that will.
>
> But even then, this goes against the general principle ( also documnted )
> of not updating the catalog directly. See [1]
>
> I think this part should be removed.
>

Makes sense and is fair. I also did not see examples of sharing queries in
the docs either. Updated the patch accordingly.

>
> 5/
>
> In a case of a prepared statement, disabling the index
> has no effect.
>
> ...

Should this not behave like if you drop (or create) an index
> during a prepared statement? I have not yet looked closely at
> this code to see what could be done.
>
>
oof! Great catch, can't believe I missed prepared statements 😅. I have
updated the patch to ensure we are invalidating the heap relation the index
is on from ATExecEnableDisableIndex and also backed it up with tests as
well. It should also address your feedback and suggestion in [1]

Thank you once again for the pointers and guidance. Attached v7 patch
(rebased with latest master).
Shayon

[1]
https://www.postgresql.org/message-id/CAA5RZ0sWpQZxjO9NEchwPeDbtw2HFWF-v8MscBDcm00FB6N_3w%40mail.gmail.com