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

On Fri, Jan 31, 2025 at 10:18 AM Sami Imseih <samimseih(at)gmail(dot)com> wrote:

>
> What is being discussed here is different from what I can tell. This
> is referring
> to the index changing status ( visible/invisible ) and those changes being
> visible by another transaction.
>
>
+1. My vote would be to keep the behavior as is, at least for the first
roll out of this feature.

> However, the current patch may be too restrictive. Why do we need
> an AccessExclusiveLock on the table to perform the change. We are
> only changing the catalog and not the underlying data. This is a lot like
> ALTER INDEX RENAME, which only takes a ShareUpdateExclusiveLock.
> Can we do the same here?
>
>
Makes sense, I think this ended up being a remananent from one of the first
iterations. Updated it in the attached v10 patch.

> 1/ Missing ATSimpleRecursion call in PrepCmd for
> case AT_SetIndexVisible:
> case AT_SetIndexInvisible:
> Without the recursion call, the visibility changes on a
> parent will not apply to the partitions. We are also
> missing tests for partitions.
>
>
Great catch! Thank you!! Updated the patch with the support for partitions
and backed by regression specs.

2/ In ATExecSetIndexVisibility
>
> Change:
> elog(ERROR, "could not find tuple for index %u", indexOid);
>
> To:
> elog(ERROR, "cache lookup failed for index %u", indexOid);
>

Good eye! I missed this :).

>
> 3/ In ATExecSetIndexVisibility
>
> if (indexForm->indcheckxmin)
> + {
> + heap_freetuple(indexTuple);
> + table_close(pg_index, RowExclusiveLock);
> + ereport(ERROR,
>
> There is no need to close the table or free the tuple, as
> these are "cleaned up" when the transaction aborts.
>
> 4/ In ATExecSetIndexVisibility
>
> I have a suggestion below:
>
> What about we open both heapOid and IndexRelationId
> at the start and close them at the end. Also,
> inside the block for indexForm->indisvisible != visible,
> do the work to invalidate the cache, invoke the post
> alter hook and increment the command counter. This will
> also allow us to get rid of the update boolean as well.
>
> What do you think?
>

Thank you for the suggestions!! I was err'ing a bit on the "defensive" side
being new to this area, however your suggestions are much more simpler. I
have updated the patch accordingly.

I have also added support for suggestions from the earlier messages, I will
respond there accordingly.

Thank you!
Shayon