Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 26 Jun 2023 17:05:37 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17997: Assert failed in validatePartitionedIndex() when
 attaching partition index to child of valid index
Message-ID: <ZJlG0a/F03iPI/wj@paquier.xyz>
References: <17997-a044c27aef95daf8@postgresql.org>
 <509915.1687704058@sss.pgh.pa.us>
 <ZJk04dp0mz7s58Lx@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2R83Dw3SW2oJXwzB"
Content-Disposition: inline
In-Reply-To: <ZJk04dp0mz7s58Lx@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZJlG0a/F03iPI/wj%40paquier.xyz>
Precedence: bulk


--2R83Dw3SW2oJXwzB
Content-Type: multipart/mixed; boundary="v+DLod0R7/jNu8sx"
Content-Disposition: inline


--v+DLod0R7/jNu8sx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 26, 2023 at 03:49:05PM +0900, Michael Paquier wrote:
> It seems to me that there are two potential solutions here:
> 1) We could recurse and make sure that the index of t *also* becomes
> *invalid*, because t_a_idx and tp1_a_idx form a partition chain, still
> the leaf tp1_1 does *not* have an index yet, making it unusable.
> 2) Then I found about AttachPartitionEnsureIndexes().  This is called
> when attaching a partition to make sure that indexes are created to
> match the indexes between the partitioned table and the new partition
> attached to it, but it does not recurse when attaching a partitioned
> table, or an index would have been created on tp1_1 when running the
> 6th command to complete the index tree all the way down.
>=20
> At the end, I'd like to think that 2) is the right path forward.  For
> example, this creates a partitioned index automatically when attaching
> a partition:
> create table t(a int) partition by range (a);
> create index on t(a);
> create table tp1(a int) partition by range (a);
> alter table t attach partition tp1 for values from (1) to (100);
> =3D# \di
>                     List of relations
>  Schema |   Name    |       Type        | Owner  | Table=20
> --------+-----------+-------------------+--------+-------
>  public | t_a_idx   | partitioned index | popopo | t
>  public | tp1_a_idx | partitioned index | popopo | tp1
> (2 rows)

A third solution that came into my mind just now would be to revisit
the choice done in AttachPartitionEnsureIndexes() where an invalid
index can be chosen as a match when creating the indexes on the
partitions, so as we are able to get to the bottom of a chain with
valid indexes for the whole tree.  I have been testing the attached
and it has been working here the way I'd expect when manipulating
partition trees with ATTACH PARTITION, though this breaks the scenario
of this bug report because we would now get a failure when attempting
to attach an index in the last command.

Thoughts or comments?
--
Michael

--v+DLod0R7/jNu8sx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Fix-ATTACH-PARTITION-to-ignore-invalid-indexes-as-ma.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 3359ecbc304fecca924c637d56c105ef7e5a6497 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 26 Jun 2023 17:05:02 +0900
Subject: [PATCH] Fix ATTACH PARTITION to ignore invalid indexes as match

---
 src/backend/commands/tablecmds.c             |  8 +++--
 src/test/regress/expected/partition_info.out | 36 ++++++++++++++++++++
 src/test/regress/sql/partition_info.sql      | 20 +++++++++++
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.c
index 9b12bc44d7..d985278ac6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -18104,8 +18104,8 @@ AttachPartitionEnsureIndexes(Relation rel, Relation=
 attachrel)
=20
 		/*
 		 * Scan the list of existing indexes in the partition-to-be, and mark
-		 * the first matching, unattached one we find, if any, as partition of
-		 * the parent index.  If we find one, we're done.
+		 * the first matching, valid, unattached one we find, if any, as
+		 * partition of the parent index.  If we find one, we're done.
 		 */
 		for (i =3D 0; i < list_length(attachRelIdxs); i++)
 		{
@@ -18116,6 +18116,10 @@ AttachPartitionEnsureIndexes(Relation rel, Relatio=
n attachrel)
 			if (attachrelIdxRels[i]->rd_rel->relispartition)
 				continue;
=20
+			/* If this index is invalid, can't use it */
+			if (!attachrelIdxRels[i]->rd_index->indisvalid)
+				continue;
+
 			if (CompareIndexInfo(attachInfos[i], info,
 								 attachrelIdxRels[i]->rd_indcollation,
 								 idxRel->rd_indcollation,
diff --git a/src/test/regress/expected/partition_info.out b/src/test/regres=
s/expected/partition_info.out
index 42b6bc77ca..4ee51a2f73 100644
--- a/src/test/regress/expected/partition_info.out
+++ b/src/test/regress/expected/partition_info.out
@@ -349,3 +349,39 @@ SELECT pg_partition_root('ptif_li_child');
 DROP VIEW ptif_test_view;
 DROP MATERIALIZED VIEW ptif_test_matview;
 DROP TABLE ptif_li_parent, ptif_li_child;
+-- Check that invalid indexes are not selected when attaching a partition.
+CREATE TABLE ptif_inval_tab (a int) PARTITION BY RANGE (a);
+CREATE INDEX ptif_inval_idx ON ptif_inval_tab (a);
+CREATE TABLE ptif_inval_tab_1 (a int) PARTITION BY RANGE (a);
+CREATE TABLE ptif_inval_tab_1_1 PARTITION OF ptif_inval_tab_1
+  FOR VALUES FROM (0) TO (10);
+CREATE TABLE ptif_inval_tab_1_2 PARTITION OF ptif_inval_tab_1
+  FOR VALUES FROM (10) TO (20);
+-- This creates an invalid index.
+CREATE INDEX ptif_inval_ixd_1 ON ONLY ptif_inval_tab_1 (a);
+-- This creates new indexes for all the partitions of ptif_inval_tab_1,
+-- discarding the invalid index created previously as something to choose.
+ALTER TABLE ptif_inval_tab ATTACH PARTITION ptif_inval_tab_1
+  FOR VALUES FROM (1) to (100);
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+  WHERE indexrelid::regclass::text ~ 'ptif_inval_'
+  ORDER BY indexrelid::regclass::text;
+        indexrelid        | indisvalid=20
+--------------------------+------------
+ ptif_inval_idx           | t
+ ptif_inval_ixd_1         | f
+ ptif_inval_tab_1_1_a_idx | t
+ ptif_inval_tab_1_2_a_idx | t
+ ptif_inval_tab_1_a_idx   | t
+(5 rows)
+
+SELECT * FROM pg_partition_tree('ptif_inval_idx');
+          relid           |      parentrelid       | isleaf | level=20
+--------------------------+------------------------+--------+-------
+ ptif_inval_idx           |                        | f      |     0
+ ptif_inval_tab_1_a_idx   | ptif_inval_idx         | f      |     1
+ ptif_inval_tab_1_1_a_idx | ptif_inval_tab_1_a_idx | t      |     2
+ ptif_inval_tab_1_2_a_idx | ptif_inval_tab_1_a_idx | t      |     2
+(4 rows)
+
+DROP TABLE ptif_inval_tab;
diff --git a/src/test/regress/sql/partition_info.sql b/src/test/regress/sql=
/partition_info.sql
index b5060bec7f..5d5f142728 100644
--- a/src/test/regress/sql/partition_info.sql
+++ b/src/test/regress/sql/partition_info.sql
@@ -127,3 +127,23 @@ SELECT pg_partition_root('ptif_li_child');
 DROP VIEW ptif_test_view;
 DROP MATERIALIZED VIEW ptif_test_matview;
 DROP TABLE ptif_li_parent, ptif_li_child;
+
+-- Check that invalid indexes are not selected when attaching a partition.
+CREATE TABLE ptif_inval_tab (a int) PARTITION BY RANGE (a);
+CREATE INDEX ptif_inval_idx ON ptif_inval_tab (a);
+CREATE TABLE ptif_inval_tab_1 (a int) PARTITION BY RANGE (a);
+CREATE TABLE ptif_inval_tab_1_1 PARTITION OF ptif_inval_tab_1
+  FOR VALUES FROM (0) TO (10);
+CREATE TABLE ptif_inval_tab_1_2 PARTITION OF ptif_inval_tab_1
+  FOR VALUES FROM (10) TO (20);
+-- This creates an invalid index.
+CREATE INDEX ptif_inval_ixd_1 ON ONLY ptif_inval_tab_1 (a);
+-- This creates new indexes for all the partitions of ptif_inval_tab_1,
+-- discarding the invalid index created previously as something to choose.
+ALTER TABLE ptif_inval_tab ATTACH PARTITION ptif_inval_tab_1
+  FOR VALUES FROM (1) to (100);
+SELECT indexrelid::regclass, indisvalid FROM pg_index
+  WHERE indexrelid::regclass::text ~ 'ptif_inval_'
+  ORDER BY indexrelid::regclass::text;
+SELECT * FROM pg_partition_tree('ptif_inval_idx');
+DROP TABLE ptif_inval_tab;
--=20
2.40.1


--v+DLod0R7/jNu8sx--

--2R83Dw3SW2oJXwzB
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmSZRtEACgkQnvQgOdby
QH1BpQ/8CxAa+82gZSJw+L1ETo/ysppltv+A2wo6Pgi/aphCySVCq3jmjbXULGtV
JKvAV6y4zAPXbML1q1G0B9fEb5gMWRmUOG4FYf0rDmQMV3IwqovsZacGy+bADk5G
vNSRE1Nus1J516Igc+45UkaUjeoLr7+BdjFpuLDSWSFfv5odb66N9RFD0HHIR/UL
vZcpxzH79FbnRapX6gAQQyt0pOqZWpJYERcDN+jgepgn0Qiw8UlgjmY5VB7RmVKV
Ow6B1NLgcyhqAmSI0OJ7R/CGt+RKLT+8u85Nr+9/vuh3OyOpp+HE6Bs+eD19X+cl
pH9je+WAU9FY+ilbRf/Eyonh02g1qYoYNKXnTVDuiCuqm0k7NZ2cuH0yJ90tE23+
TxZ+CiSMcB2rzghbVLpVr9BxPyYBVeRadFEKxYkCBz6j6QYffnOVEbdTgX5wFwRl
NQPBmBwR8t0mb8JIE4DKMoETVEW3UWNLunDn4QLt3vcKI0gnhp6vMjbs8t2wt9mR
DPL9Jg7+8D9B/SlTSiEn6Jnj981aRmwku5Wz6NYjWzNjlkiZLl16CCPVld/DL0Ha
xTh/rabRGalKal7phxRkwInOgfieZbZrjRUHWDTA5fkPokIdJcz3Qc/gBMPopB8+
Yn5PuiPl/oBoIr49HY86Nq8TgdX7dlIoNmZc7yC+5/ep3/FuzyA=
=kxhW
-----END PGP SIGNATURE-----

--2R83Dw3SW2oJXwzB--