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

On Sun, Feb 2, 2025 at 3:11 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> hi.
> the following reviews based on
> v10-0001-Introduce-the-ability-to-set-index-visibility-us.patch.
>
> Thank you for the amazing review!!

> in src/test/regress/sql/create_index.sql
> seems there are no sql tests for "create index ... invisible"?
>
>
> Good call, added in v11 patch (attached)

> "Make the specified index visible. The index can be used for query
> planning"
> ?
>

Done in v11 patch.

>
> Do we need to add GUC use_invisible_index to postgresql.conf.sample?
>
>
I wasn't sure at first, hence opted for GUC_NOT_IN_SAMPLE when introducing
the GUC. I have added the new GUC in postgresql.conf.sample as part of the
v11 patch.

> CREATE TABLE t(id INT PRIMARY KEY, data TEXT,num INT, vector INT[],
> range INT4RANGE);
> ALTER INDEX t_pkey INVISIBLE;
> alter table t alter column id set data type bigint;
> \d t
>
> after ALTER TABLE SET DATA TYPE, the "visible" status should not change?
> but here it changed.
> you may check ATPostAlterTypeParse to make the "visible" status not change.
>
>
Thank you! I was relying on the existing specs to guide me on cases like
this. That said - now I have fixed this in the v11 patch and also added
regression specs for the same (future proofing and all).

> ....
> + createFlags = INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT;
> + if (indexForm->indisvisible)
> + createFlags |= INDEX_CREATE_VISIBLE;
> the indentation level seems also not right?
>
>
>
Thank you. I have struggled with indentation in the project a bit. I have
gone ahead and fixed these, but I would love to know how do folks generally
solve this and if they use any linting tools? I just use VScode, so my
tooling may not be right for local dev.

> INVISIBLE, VISIBLE is not special words, in gram.y, you don't need
> "VISIBLE_P", "INVISIBLE_P", you can just use "INVISIBLE", "VISIBLE"
> ?
>
> Got it, thank you and updated!

> ....
>
> pg_dump will dump as
> --
> -- Name: t3 t3_pkey; Type: CONSTRAINT; Schema: public; Owner: jian
> --
> ALTER TABLE ONLY public.t3
> ADD CONSTRAINT t3_pkey PRIMARY KEY (id);
>
> after dump, restore index (primary key: t3_pkey) INVISIBLE will not be
> restored.
> We need extra work for restoring the INVISIBLE flag for the primary key
> index.
>
>
Great catch! I am learning that handling of primary keys and constraint +
indexes behave slightly differently in terms of logic across the codebase.
I have updated the v11 patch with the fixes to ensure that pg_dump will
respect the index visibility status on primary keys and also followed up
with specs in 002_pg_dump.pl as a way to future proof the behavior. It was
nice to learn more on how testing inside the .pl specs work as well.

> I am not sure if we need to change index_concurrently_swap or not.
> but many other pg_index columns changed.
>

My apologies, but I didn't fully follow this feedback. There are some specs
in create_index.sql for REINDEX behavior when index visibility and I didn't
notice any change in behavior in terms of query planning or the columns in
pg_index. The only change I noticed was the rel id, which makes sense given
the behavior of REINDEX. Once I understand the issue more, happy to follow
up with fixes/specs accordingly.

Few additional notes

- The v11 patch now shows the index invisibility status when you do \d
index_name. h/t to Benoit Lobréau for the patch [1]
- The v11 patch also updated test_ddl_deparse.c as mentioned in [2] and
also brings in new specs in
src/test/modules/test_ddl_deparse/expected/alter_index.out as a follow up.
- I found it interesting that this wasn't caught in specs in CI or
anywhere else and I think it is dependent on the clang flags (?). Anyways,
sharing for posterity. For now there is spec coverage for future cases in
v11 patch.

[1]
https://www.postgresql.org/message-id/CAPE8EZ5G%2BCZiw%3Dp1Cs7DOZ2MGLa1yTS8Tk%3DThzi1F14N2A%3D1oQ%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CACJufxHROE2pDRYecnts9u12K-2R3AGhFADEw_C-GNiRWKZ6ig%40mail.gmail.com