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

From: Sami Imseih <[email-masked]>
Date: Thu, 30 Jan 2025 22:48:18 -0600

> I had the "BEGIN; ALTER INDEX; EXPLAIN; ROLLBACK;" scenario in mind, but
> didn't realise we acquire an AccessExclusiveLock on the index. Therefore, it's
> not possible to change the visibility within a single transaction....
> unless you don’t mind blocking all access to the relation.
>
> I read the comments at the top of "AlterTableGetLockLevel" and in the
> documentation and I understand that this behavior seems unavoidable.
> I suppose this is what was meant by the "globally visible effects" of an ALTER
> INDEX in the old discussion ? [1]

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.

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?

I am also still reviewing and have a few comments on v9

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.

2/ In ATExecSetIndexVisibility

Change:
elog(ERROR, "could not find tuple for index %u", indexOid);

To:
elog(ERROR, "cache lookup failed for index %u", indexOid);

I see both message formats are used all over the place,
but in tablecmds.c, the "cache lookup" variant is the one
used, so let's do that for consistency.

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?

Regards,

Sami

---

From: Shayon Mukherjee <[email-masked]>
Date: Sat, 1 Feb 2025 13:05:25 +0530

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

> ALTER INDEX RENAME, which only takes a ShareUpdateExclusiveLock.
Makes sense, I think this ended up being a remananent from one of the first
iterations. Updated it in the attached v10 patch.

> missing tests for partitions.
Great catch! Thank you!! Updated the patch with the support for partitions
and backed by regression specs.

> elog(ERROR, "cache lookup failed for index %u", indexOid);

Good eye! I missed this :).

> also allow us to get rid of the update boolean as well.

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