Date: Tue, 1 Sep 2020 11:58:35 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: jan.mussler@zalando.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table
 with a non helpful error message.
Message-ID: <20200901025835.GC3511@paquier.xyz>
References: <20200829013433.GA1529@paquier.xyz>
 <20200901012553.GA17180@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="HeFlAV5LIbMFYYuh"
Content-Disposition: inline
In-Reply-To: <20200901012553.GA17180@alvherre.pgsql>
Precedence: bulk


--HeFlAV5LIbMFYYuh
Content-Type: multipart/mixed; boundary="4zI0WCX1RcnW9Hbu"
Content-Disposition: inline


--4zI0WCX1RcnW9Hbu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Aug 31, 2020 at 09:25:53PM -0400, Alvaro Herrera wrote:
> Actually I think you're wrong; if I put it before the check, then if I
> do "drop index concurrently some_temp_partitioned_index" then it would
> fail; but if I put it after the check, then it does a normal
> non-concurrent index and it works.  I'm not sure it's necessary to break
> a case that otherwise works ...

Hmm.  Right.  I agree that it would be better to not break that case.
And it means that there is a gap in the regression tests here, so I'd
like to add a test.  Please see the attached to achieve that, which
includes your own code changes and the doc parts (I didn't see a point
in changing the new sentence for temporary relations as the follow-up
<para> mentions that).

> (But for that to work I need to test the flag in the bitmask rather than
> the option in the command, as in the attached).

Make sense.
--
Michael

--4zI0WCX1RcnW9Hbu
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-drop-part-michael.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.c
index d2b15a3387..3e57c7f9e1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1373,6 +1373,17 @@ RemoveRelations(DropStmt *drop)
 			flags |=3D PERFORM_DELETION_CONCURRENTLY;
 		}
=20
+		/*
+		 * Concurrent index drop cannot be used with partitioned indexes,
+		 * either.
+		 */
+		if ((flags & PERFORM_DELETION_CONCURRENTLY) !=3D 0 &&
+			get_rel_relkind(relOid) =3D=3D RELKIND_PARTITIONED_INDEX)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot drop partitioned index \"%s\" concurrently",
+							rel->relname)));
+
 		/* OK, we're ready to delete this one */
 		obj.classId =3D RelationRelationId;
 		obj.objectId =3D relOid;
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expe=
cted/indexing.out
index f78865ef81..7e78a07af8 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -174,6 +174,8 @@ create table idxpart1 partition of idxpart for values f=
rom (0) to (10);
 drop index idxpart1_a_idx;	-- no way
 ERROR:  cannot drop index idxpart1_a_idx because index idxpart_a_idx requi=
res it
 HINT:  You can drop index idxpart_a_idx instead.
+drop index concurrently idxpart_a_idx;	-- unsupported
+ERROR:  cannot drop partitioned index "idxpart_a_idx" concurrently
 drop index idxpart_a_idx;	-- both indexes go away
 select relname, relkind from pg_class
   where relname like 'idxpart%' order by relname;
@@ -194,6 +196,24 @@ select relname, relkind from pg_class
 (2 rows)
=20
 drop table idxpart;
+-- DROP behavior with temporary partitioned indexes
+create temp table idxpart_temp (a int) partition by range (a);
+create index on idxpart_temp(a);
+create temp table idxpart1_temp partition of idxpart_temp
+  for values from (0) to (10);
+drop index idxpart1_temp_a_idx; -- error
+ERROR:  cannot drop index idxpart1_temp_a_idx because index idxpart_temp_a=
_idx requires it
+HINT:  You can drop index idxpart_temp_a_idx instead.
+-- non-concurrent drop is enforced here, so it is a valid case.
+drop index concurrently idxpart_temp_a_idx;
+select relname, relkind from pg_class
+  where relname like 'idxpart_temp%' order by relname;
+   relname    | relkind=20
+--------------+---------
+ idxpart_temp | p
+(1 row)
+
+drop table idxpart_temp;
 -- ALTER INDEX .. ATTACH, error cases
 create table idxpart (a int, b int) partition by range (a, b);
 create table idxpart1 partition of idxpart for values from (0, 0) to (10, =
10);
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/index=
ing.sql
index 35d159f41b..260233ae73 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -92,6 +92,7 @@ create table idxpart (a int) partition by range (a);
 create index on idxpart (a);
 create table idxpart1 partition of idxpart for values from (0) to (10);
 drop index idxpart1_a_idx;	-- no way
+drop index concurrently idxpart_a_idx;	-- unsupported
 drop index idxpart_a_idx;	-- both indexes go away
 select relname, relkind from pg_class
   where relname like 'idxpart%' order by relname;
@@ -100,6 +101,17 @@ drop table idxpart1;		-- the index on partition goes a=
way too
 select relname, relkind from pg_class
   where relname like 'idxpart%' order by relname;
 drop table idxpart;
+-- DROP behavior with temporary partitioned indexes
+create temp table idxpart_temp (a int) partition by range (a);
+create index on idxpart_temp(a);
+create temp table idxpart1_temp partition of idxpart_temp
+  for values from (0) to (10);
+drop index idxpart1_temp_a_idx; -- error
+-- non-concurrent drop is enforced here, so it is a valid case.
+drop index concurrently idxpart_temp_a_idx;
+select relname, relkind from pg_class
+  where relname like 'idxpart_temp%' order by relname;
+drop table idxpart_temp;
=20
 -- ALTER INDEX .. ATTACH, error cases
 create table idxpart (a int, b int) partition by range (a, b);
diff --git a/doc/src/sgml/ref/drop_index.sgml b/doc/src/sgml/ref/drop_index=
=2Esgml
index 0aedd71bd6..85cf23bca2 100644
--- a/doc/src/sgml/ref/drop_index.sgml
+++ b/doc/src/sgml/ref/drop_index.sgml
@@ -57,6 +57,8 @@ DROP INDEX [ CONCURRENTLY ] [ IF EXISTS ] <replaceable cl=
ass=3D"parameter">name</r
       Also, regular <command>DROP INDEX</command> commands can be
       performed within a transaction block, but
       <command>DROP INDEX CONCURRENTLY</command> cannot.
+      Lastly, indexes on partitioned tables cannot be dropped using this
+      option.
      </para>
      <para>
       For temporary tables, <command>DROP INDEX</command> is always

--4zI0WCX1RcnW9Hbu--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl9NuNsACgkQnvQgOdby
QH0uqA//c1K7uJpiLXB27EclhEKYhwjhvB9GOaQygbx4EFl7isWanbTseZATe/Pn
0j2BzdXLRjjC0ty8R5dKUW50sk4iQVfXFi7Y/V44VVbrsqbUOKTb+pxsZzxxK1Vm
Ptrva4Y3uthODeoq+NhRoQceCyPJP2hLCbmbsV23oKTEHWoxECNaKYriO+0efWc3
snwFnGHUFXS6iltfZMwt+JbuI1Urlp9P/h5TEeHJz6jW0qW6wvqDleUpZNgJJELT
+kqaCq00+qMDA1X6NfOqxLpIFaA7Zhp5R9Z0M6o2xBA5h1lORNxlNkfnLHQLQ4OJ
6HxaI0oNW0b2HkOQj7pJElPQRllaKXrcoTPAdbOGs6y8GgHdFgpoL4YzTf975QEK
uABkAnG9COTFPqIsGChpGuHtNbWfWeUKqwFnk91/FburMo1J3LDvDB3rXSknWj6O
zGoehdMfE3UNOXtPTcaaA5uWdEWGyhaPurz6xFqZHdW4A8tncjFKZeMsNAglvXGh
khA1hXT2KtJDLujMKSXVEUsQ9RJ+SKESJ7NiGXV54rrp6Kpi/WySasJIAebgZKQd
R0uUMcnu3U7T1Ae/y++mGzvHQVZ7pW7To+qQZdIFGRuh6PFAxtxNODB4y0K+qsdn
n/7MAwBR0nBfbh9t0eNHViSkr3TNAfRjDcEaqK5xOeTcjDi28lw=
=RdsK
-----END PGP SIGNATURE-----

--HeFlAV5LIbMFYYuh--