On Sun, Feb 2, 2025 at 3:11 PM jian he <jian.universal...@gmail.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
v11-0001-Introduce-the-ability-to-set-index-visibility-us.patch
Description: Binary data
