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.
Thank you
Shayon
