Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX

On Thu, Apr 24, 2025 at 12:45 AM jian he <jian.universality@gmail.com>
wrote:

> hi.
> The following is a review of version 17.
>
> ATExecSetIndexVisibility
>     if (indexForm->indisvisible != visible)
>     {
>         indexForm->indisvisible = visible;
>         CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
>         CacheInvalidateRelcache(heapRel);
>         InvokeObjectPostAlterHook(IndexRelationId, indexOid, 0);
>         CommandCounterIncrement();
>     }
> I am slightly confused. if we already used
> CommandCounterIncrement();
> then we don't need CacheInvalidateRelcache?
>
>
>
Thank you for this catch. I misunderstood the behavior of the two and was
performing both to avoid inconsistency between state within a transaction
and cross session, but as you pointed out CommandCounterIncrement() helps
achieve both. Updated.

doc/src/sgml/ref/alter_index.sgml
>   <para>
>    <command>ALTER INDEX</command> changes the definition of an existing
> index.
>    There are several subforms described below. Note that the lock level
> required
>    may differ for each subform. An <literal>ACCESS EXCLUSIVE</literal>
> lock is held
>    unless explicitly noted. When multiple subcommands are listed, the lock
>    held will be the strictest one required from any subcommand.
>
> per the above para, we need mention that ALTER INDEX SET
> INVISIBLE|INVISIBLE
> only use ShareUpdateExclusiveLock?
>
>
I wasn't sure at first where to add the note about
ShareUpdateExclusiveLock. But it looks like we have a precedent from RENAME
in doc/src/sgml/ref/alter_index.sgml, so I have done the same for VISIBLE &
INVISIBLE in doc/src/sgml/ref/alter_index.sgml as well.


> index_create is called in several places,
> most of the time, we use INDEX_CREATE_VISIBLE.
> if we define it as INDEX_CREATE_INVISIBLE rather than INDEX_CREATE_VISIBLE
> then argument flags required code changes would be less, (i didn't try
> it myself)
>

Looks like the only change we would save is the one in
src/backend/catalog/toasting.c. Rest of the code change/diffs would still
be needed IIUC (if I understand correctly). This approach felt a bit
ergonomical, hence opted for it, but happy to update. Let me know.


>

Similar to get_index_isclustered,
> We can place GetIndexVisibility in
> src/backend/utils/cache/lsyscache.c,
> make it an extern function, so others can use it;
> to align with other function names,
> maybe rename it as get_index_visibility.
>
>
I was a bit torn on this one and figured I wouldn't introduce it as it
could be a bit of premature optimization, until there were more use cases
(or maybe one more). Plus, I figured the next time we need this info, we
could expose a more public function like get_index_visibility (given N=2, N
being the number of callers). However, given you mentioned and spotted this
as well, I have introduced get_index_visibility in the new patch now.



> create index v2_idx on v1(data) visible;
> is allowed,
> doc/src/sgml/ref/create_index.sgml
> <synopsis> section need to change to
> [ VISIBLE | INVISIBLE ]
>
> ?
>

Updated to match the same pattern as the one in
doc/src/sgml/ref/alter_index.sgml.

Thank you for the feedback. I have also updated the feedback from [1] as
well. Few extra notes:

- Attached v18
- Rebased against master
- Updated the commit message
- Updated the target remote version to now be fout->remoteVersion >= 190000
- Using a UNION ALL query to show all indexes from part_tbl partitioned
tables in the specs as noted in [1]. The query suggested in [1] wasn't
encompassing all the indexes, hence the UNION ALL for WHERE i.indrelid =
'part_tbl'::regclass::oid.

[1]
https://www.postgresql.org/message-id/CACJufxFS_M7nGvFiz-dUutaWb7RQxRMO97wC5ZezKW2ZsMQPQg@mail.gmail.com

Thank you
Shayon