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

> 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