($INBOX_DIR/description missing)  

help / color / mirror / Atom feed

[PATCH] Disable background workers during servers start in pg_upgrade
28+ messages / 4 participants
[nested] [flat]

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* [PATCH] Disable background workers during servers start in pg_upgrade
@ 2021-01-20 16:25  Denis Laxalde <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Denis Laxalde @ 2021-01-20 16:25 UTC (permalink / raw)

We disable shared_preload_libraries to prevent background workers to
initialize and start during server start in pg_upgrade.

In essence, this is for a similar reason that we use a restricted socket
access from f763b77193b04eba03a1f4ce46df34dc0348419e because background
workers may produce undesired activities during the upgrade.

Author: Denis Laxalde <[email protected]>
Co-authored-by: Jehan-Guillaume de Rorthais <[email protected]>
---
 src/bin/pg_upgrade/server.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/server.c b/src/bin/pg_upgrade/server.c
index 31b1425202..fab95a2d24 100644
--- a/src/bin/pg_upgrade/server.c
+++ b/src/bin/pg_upgrade/server.c
@@ -240,11 +240,13 @@ start_postmaster(ClusterInfo *cluster, bool report_and_exit_on_error)
 	 * crash, the new cluster has to be recreated anyway.  fsync=off is a big
 	 * win on ext4.
 	 *
+	 * Turn off background workers by emptying shared_preload_libraries.
+	 *
 	 * Force vacuum_defer_cleanup_age to 0 on the new cluster, so that
 	 * vacuumdb --freeze actually freezes the tuples.
 	 */
 	snprintf(cmd, sizeof(cmd),
-			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s\" start",
+			 "\"%s/pg_ctl\" -w -l \"%s\" -D \"%s\" -o \"-p %d%s%s %s%s -c shared_preload_libraries=''\" start",
 			 cluster->bindir, SERVER_LOG_FILE, cluster->pgconfig, cluster->port,
 			 (cluster->controldata.cat_ver >=
 			  BINARY_UPGRADE_SERVER_FLAG_CAT_VER) ? " -b" :
-- 
2.20.1


--lvvxlk2k67d5kdun--





^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-05-23 21:14  Ilya Gladyshev <[email protected]>
  0 siblings, 2 replies; 28+ messages in thread

From: Ilya Gladyshev @ 2024-05-23 21:14 UTC (permalink / raw)
  To: Alexander Pyhalov <[email protected]>; Justin Pryzby <[email protected]>; +Cc: [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>; 曾文旌(义从) <[email protected]>; Alvaro Herrera <[email protected]>

Hi,

I think it's well worth the effort to revive the patch, so I rebased it 
on master, updated it and will return it back to the commitfest. 
Alexander, Justin feel free to add yourselves as authors

On 29.01.2024 12:43, Alexander Pyhalov wrote:
> Hi.
>
> I've rebased patch on master and it'seems to me there's one more issue -
>
> when we call DefineIndexConcurrentInternal() in partitioned case, it 
> waits for transactions, locking tableId, not tabrelid - heaprelid 
> LockRelId is constructed for parent index relation, not for child 
> index relation.
>
> Attaching fixed version.
>
> Also I'm not sure what to do with locking of child relations. If we 
> don't do anything, you can drop one of the partitioned table childs 
> while CIC is in progress, and get error
>
> ERROR:  cache lookup failed for index 16399
I agree that we need to do something about it, in particular, I think we 
should lock all the partitions inside the transaction that builds the 
catalog entries. Fixed this in the new version.
> If you try to lock all child tables in CIC session, you'll get deadlocks.

Do you mean the deadlock between the transaction that drops a partition 
and the transaction doing CIC? I think this is unavoidable and can be 
reproduced even without partitioning.

Also not sure why a list of children relation was obtained with 
ShareLock that CIC is supposed to avoid not to block writes, changed 
that to ShareUpdateExclusive.

Regards,

Ilya



Attachments:

  [text/x-patch] v2-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch (23.7K, ../../[email protected]/2-v2-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch)
  download | inline diff:
From 1cb76c8c19b1c0549fbba70febc32017bc04c0a2 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Thu, 23 May 2024 18:13:41 +0100
Subject: [PATCH v2] Allow CREATE INDEX CONCURRENTLY on partitioned table

---
 doc/src/sgml/ddl.sgml                  |   7 +-
 doc/src/sgml/ref/create_index.sgml     |  14 +-
 src/backend/commands/indexcmds.c       | 293 ++++++++++++++++++-------
 src/test/regress/expected/indexing.out | 127 ++++++++++-
 src/test/regress/sql/indexing.sql      |  26 ++-
 5 files changed, 367 insertions(+), 100 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 6aab79e901..f1d4a59a99 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4314,10 +4314,9 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      As mentioned earlier, it is possible to create indexes on partitioned
      tables so that they are applied automatically to the entire hierarchy.
      This can be very convenient as not only will all existing partitions be
-     indexed, but any future partitions will be as well.  However, one
-     limitation when creating new indexes on partitioned tables is that it
-     is not possible to use the <literal>CONCURRENTLY</literal>
-     qualifier, which could lead to long lock times.  To avoid this, you can
+     indexed, but any future partitions will be as well.
+     <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock times
+     on huge partitioned tables, to avoid that you can
      use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
      creates the new index marked as invalid, preventing automatic application
      to existing partitions.  Instead, indexes can then be created individually
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 621bc0e253..2366cfd9b5 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while build an index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -692,15 +695,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
-    Concurrent builds for indexes on partitioned tables are currently not
-    supported.  However, you may concurrently build the index on each
-    partition individually and then finally create the partitioned index
-    non-concurrently in order to reduce the time where writes to the
-    partitioned table will be locked out.  In this case, building the
-    partitioned index is a metadata only operation.
-   </para>
-
   </refsect2>
  </refsect1>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 309389e20d..5806eeb8ef 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 bool primary, bool isconstraint);
 static char *ChooseIndexNameAddition(const List *colnames);
 static List *ChooseIndexColumnNames(const List *indexElems);
+static void DefineIndexConcurrentInternal(Oid relationId,
+										  Oid indexRelationId,
+										  IndexInfo *indexInfo,
+										  LOCKTAG heaplocktag,
+										  LockRelId *heaprelid);
 static void ReindexIndex(const ReindexStmt *stmt, const ReindexParams *params,
 						 bool isTopLevel);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
@@ -561,7 +566,6 @@ DefineIndex(Oid tableId,
 	bool		amissummarizing;
 	amoptions_function amoptions;
 	bool		partitioned;
-	bool		safe_index;
 	Datum		reloptions;
 	int16	   *coloptions;
 	IndexInfo  *indexInfo;
@@ -569,12 +573,10 @@ DefineIndex(Oid tableId,
 	bits16		constr_flags;
 	int			numberOfAttributes;
 	int			numberOfKeyAttributes;
-	TransactionId limitXmin;
 	ObjectAddress address;
 	LockRelId	heaprelid;
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
-	Snapshot	snapshot;
 	Oid			root_save_userid;
 	int			root_save_sec_context;
 	int			root_save_nestlevel;
@@ -706,20 +708,6 @@ DefineIndex(Oid tableId,
 	 * partition.
 	 */
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
-	if (partitioned)
-	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
-	}
 
 	/*
 	 * Don't try to CREATE INDEX on temp tables of other backends.
@@ -1116,10 +1104,6 @@ DefineIndex(Oid tableId,
 		}
 	}
 
-	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
-	safe_index = indexInfo->ii_Expressions == NIL &&
-		indexInfo->ii_Predicate == NIL;
-
 	/*
 	 * Report index creation if appropriate (delay this till after most of the
 	 * error checks)
@@ -1184,6 +1168,11 @@ DefineIndex(Oid tableId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1533,21 +1522,7 @@ DefineIndex(Oid tableId,
 			 */
 			if (invalidate_parent)
 			{
-				Relation	pg_index = table_open(IndexRelationId, RowExclusiveLock);
-				HeapTuple	tup,
-							newtup;
-
-				tup = SearchSysCache1(INDEXRELID,
-									  ObjectIdGetDatum(indexRelationId));
-				if (!HeapTupleIsValid(tup))
-					elog(ERROR, "cache lookup failed for index %u",
-						 indexRelationId);
-				newtup = heap_copytuple(tup);
-				((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
-				CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
-				ReleaseSysCache(tup);
-				table_close(pg_index, RowExclusiveLock);
-				heap_freetuple(newtup);
+				index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
 
 				/*
 				 * CCI here to make this update visible, in case this recurses
@@ -1559,37 +1534,49 @@ DefineIndex(Oid tableId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
-		 * done here.
+		 * done here in the non-concurrent case.
 		 */
-		AtEOXact_GUC(false, root_save_nestlevel);
-		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
-		if (!OidIsValid(parentIndexId))
-			pgstat_progress_end_command();
-		else
+		if (!concurrent)
 		{
-			/* Update progress for an intermediate partitioned index itself */
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
-		}
+			AtEOXact_GUC(false, root_save_nestlevel);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+			table_close(rel, NoLock);
 
-		return address;
+			if (!OidIsValid(parentIndexId))
+				pgstat_progress_end_command();
+			else
+			{
+				/*
+				 * Update progress for an intermediate partitioned index
+				 * itself
+				 */
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			}
+
+			return address;
+		}
 	}
 
 	AtEOXact_GUC(false, root_save_nestlevel);
 	SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
 
-	if (!concurrent)
+	/*
+	 * All done in the non-concurrent case, and when building catalog entries
+	 * of partitions for CIC.
+	 */
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
 		/*
 		 * If this is the top-level index, the command is done overall;
-		 * otherwise, increment progress to report one child index is done.
+		 * otherwise (when being called recursively), increment progress to
+		 * report that one child index is done.  Except in the concurrent
+		 * (catalog-only) case, which is handled later.
 		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
-		else
+		else if (!concurrent)
 			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
@@ -1600,28 +1587,180 @@ DefineIndex(Oid tableId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
-	/*
-	 * For a concurrent build, it's important to make the catalog entries
-	 * visible to other transactions before we start to build the index. That
-	 * will prevent them from making incompatible HOT updates.  The new index
-	 * will be marked not indisready and not indisvalid, so that no one else
-	 * tries to either insert into it or use it for queries.
-	 *
-	 * We must commit our current transaction so that the index becomes
-	 * visible; then start another.  Note that all the data structures we just
-	 * built are lost in the commit.  The only data we keep past here are the
-	 * relation IDs.
-	 *
-	 * Before committing, get a session-level lock on the table, to ensure
-	 * that neither it nor the index can be dropped before we finish. This
-	 * cannot block, even if someone else is waiting for access, because we
-	 * already have the same lock within our transaction.
-	 *
-	 * Note: we don't currently bother with a session lock on the index,
-	 * because there are no operations that could change its state while we
-	 * hold lock on the parent table.  This might need to change later.
-	 */
-	LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+	if (!partitioned)
+	{
+		/*
+		 * For a concurrent build, it's important to make the catalog entries
+		 * visible to other transactions before we start to build the index.
+		 * That will prevent them from making incompatible HOT updates.  The
+		 * new index will be marked not indisready and not indisvalid, so that
+		 * no one else tries to either insert into it or use it for queries.
+		 *
+		 * DefineIndexConcurrentInternal will commit our current transaction
+		 * so that the index becomes visible; then start another.  Note that
+		 * all the data structures we just built are lost in the commit.  The
+		 * only data we keep past here are the relation IDs.
+		 *
+		 * Before committing, get a session-level lock on the table, to ensure
+		 * that neither it nor the index can be dropped before we finish. This
+		 * cannot block, even if someone else is waiting for access, because
+		 * we already have the same lock within our transaction.
+		 *
+		 * Note: we don't currently bother with a session lock on the index,
+		 * because there are no operations that could change its state while
+		 * we hold lock on the parent table.  This might need to change later.
+		 */
+
+		LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+
+		/* CREATE INDEX CONCURRENTLY on a nonpartitioned table */
+		DefineIndexConcurrentInternal(tableId, indexRelationId,
+									  indexInfo, heaplocktag, &heaprelid);
+		pgstat_progress_end_command();
+		return address;
+	}
+	else
+	{
+		/*
+		 * For CIC on a partitioned table, finish by building indexes on
+		 * partitions
+		 */
+
+		ListCell   *lc;
+		ListCell   *lc2;
+		List	   *childs;
+		List	   *part_idxs = NIL;
+		List	   *leaf_idxs = NIL;
+		List	   *leaf_idx_lockids = NIL;
+		List	   *part_idx_lockids = NIL;
+		MemoryContext cic_context,
+					old_context;
+
+		/* Create special memory context for cross-transaction storage */
+		cic_context = AllocSetContextCreate(PortalContext,
+											"Create index concurrently",
+											ALLOCSET_DEFAULT_SIZES);
+
+		old_context = MemoryContextSwitchTo(cic_context);
+		childs = find_all_inheritors(indexRelationId, ShareUpdateExclusiveLock, NULL);
+		MemoryContextSwitchTo(old_context);
+
+		foreach(lc, childs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			char		relkind;
+			LockRelId  *lockrelid;
+			Oid			tabrelid;
+
+			/*
+			 * Pre-existing partitions which were ATTACHED were already
+			 * counted in the progress report.
+			 */
+			if (get_index_isvalid(indrelid))
+				continue;
+
+			tabrelid = IndexGetRelation(indrelid, false);
+			rel = table_open(tabrelid, ShareUpdateExclusiveLock);
+			lockrelid = palloc_object(LockRelId);
+			*lockrelid = rel->rd_lockInfo.lockRelId;
+			table_close(rel, ShareUpdateExclusiveLock);
+
+			/*
+			 * Split partitions in 2 lists: partitioned tables that just need
+			 * to be marked as valid and leaf tables that actually need to
+			 * have their indexes built.
+			 */
+			relkind = get_rel_relkind(indrelid);
+			if (!RELKIND_HAS_STORAGE(relkind))
+			{
+				/* The toplevel index doesn't count towards "partitions done" */
+				if (indrelid != indexRelationId)
+					pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
+				/*
+				 * Build up a list of all the intermediate partitioned tables
+				 * which will later need to be set valid.
+				 */
+				old_context = MemoryContextSwitchTo(cic_context);
+				part_idxs = lappend_oid(part_idxs, indrelid);
+				part_idx_lockids = lappend(part_idx_lockids, lockrelid);
+				MemoryContextSwitchTo(old_context);
+			}
+			else
+			{
+				old_context = MemoryContextSwitchTo(cic_context);
+
+				leaf_idxs = lappend_oid(leaf_idxs, indrelid);
+				leaf_idx_lockids = lappend(leaf_idx_lockids, lockrelid);
+
+				MemoryContextSwitchTo(old_context);
+
+			}
+
+			/*
+			 * All partitions, including top-level parent, need to be locked
+			 * for the session before proceeding with any index rebuilds as
+			 * each of those are done in a separate transaction. After each
+			 * leaf index is built, its corresponding table will be unlocked,
+			 * all the partitioned tables will be unlocked at the very end.
+			 */
+
+			LockRelationIdForSession(lockrelid, ShareUpdateExclusiveLock);
+
+		}
+
+		forboth(lc, leaf_idxs, lc2, leaf_idx_lockids)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			LockRelId  *heaprelid = lfirst(lc2);
+			Oid			tabrelid = IndexGetRelation(indrelid, false);
+
+			SET_LOCKTAG_RELATION(heaplocktag, heaprelid->dbId, heaprelid->relId);
+
+			/* Process each partition in a separate transaction */
+			DefineIndexConcurrentInternal(tabrelid, indrelid, indexInfo,
+										  heaplocktag, heaprelid);
+
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
+		/* Set as valid all partitioned indexes, including the parent */
+		foreach(lc, part_idxs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_VALID);
+		}
+
+		foreach(lc, part_idx_lockids)
+		{
+			LockRelId  *lockrelid = lfirst(lc);
+
+			UnlockRelationIdForSession(lockrelid, ShareUpdateExclusiveLock);
+		}
+
+
+		MemoryContextDelete(cic_context);
+		pgstat_progress_end_command();
+		PopActiveSnapshot();
+		return address;
+	}
+}
+
+
+static void
+DefineIndexConcurrentInternal(Oid tableId, Oid indexRelationId, IndexInfo *indexInfo,
+							  LOCKTAG heaplocktag, LockRelId *heaprelid)
+{
+	TransactionId limitXmin;
+	Snapshot	snapshot;
+
+	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+	bool		safe_index = indexInfo->ii_Expressions == NIL &&
+		indexInfo->ii_Predicate == NIL;
 
 	PopActiveSnapshot();
 	CommitTransactionCommand();
@@ -1789,16 +1928,12 @@ DefineIndex(Oid tableId,
 	 * would be useful.  (Note that our earlier commits did not create reasons
 	 * to replan; so relcache flush on the index itself was sufficient.)
 	 */
-	CacheInvalidateRelcacheByRelid(heaprelid.relId);
+	CacheInvalidateRelcacheByRelid(heaprelid->relId);
 
 	/*
 	 * Last thing to do is release the session-level lock on the parent table.
 	 */
-	UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
-
-	pgstat_progress_end_command();
-
-	return address;
+	UnlockRelationIdForSession(heaprelid, ShareUpdateExclusiveLock);
 }
 
 
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f25723da92..44a6cf39df 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,130 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR:  cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx1"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart
+        Partitioned table "public.idxpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "idxpart_a_idx" btree (a)
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 3 (Use \d+ to list them.)
+
+\d idxpart1
+        Partitioned table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart11
+       Partitioned table "public.idxpart11"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart11_a_idx" btree (a)
+    "idxpart11_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart111
+       Partitioned table "public.idxpart111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart11 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart111_a_idx" btree (a)
+    "idxpart111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart1111
+      Partitioned table "public.idxpart1111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart111 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1111_a_idx" btree (a)
+    "idxpart1111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 0
+
+\d idxpart2
+              Table "public.idxpart2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+    "idxpart2_a_idx" btree (a)
+    "idxpart2_a_idx1" UNIQUE, btree (a) INVALID
+
+\d idxpart3
+        Partitioned table "public.idxpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (30) TO (40)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart3_a_idx" btree (a)
+    "idxpart3_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart31
+             Table "public.idxpart31"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart3 DEFAULT
+Indexes:
+    "idxpart31_a_idx" btree (a)
+    "idxpart31_a_idx1" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 5f1f4b80c9..38a730d877 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,30 @@ select relname, relkind, relhassubclass, inhparent::regclass
 	where relname like 'idxpart%' order by relname;
 drop table idxpart;
 
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart11
+\d idxpart111
+\d idxpart1111
+\d idxpart2
+\d idxpart3
+\d idxpart31
 drop table idxpart;
 
 -- Verify bugfix with query on indexed partitioned table with no partitions
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-05-24 09:04  Alexander Pyhalov <[email protected]>
  parent: Ilya Gladyshev <[email protected]>
  1 sibling, 1 reply; 28+ messages in thread

From: Alexander Pyhalov @ 2024-05-24 09:04 UTC (permalink / raw)
  To: Ilya Gladyshev <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>; 曾文旌(义从) <[email protected]>; Alvaro Herrera <[email protected]>

Ilya Gladyshev писал(а) 2024-05-24 00:14:
> Hi,

Hi.

> 
> I think it's well worth the effort to revive the patch, so I rebased it 
> on master, updated it and will return it back to the commitfest. 
> Alexander, Justin feel free to add yourselves as authors
> 
> On 29.01.2024 12:43, Alexander Pyhalov wrote:
>> Hi.
>> 
>> I've rebased patch on master and it'seems to me there's one more issue 
>> -
>> 
>> when we call DefineIndexConcurrentInternal() in partitioned case, it 
>> waits for transactions, locking tableId, not tabrelid - heaprelid 
>> LockRelId is constructed for parent index relation, not for child 
>> index relation.
>> 
>> Attaching fixed version.
>> 
>> Also I'm not sure what to do with locking of child relations. If we 
>> don't do anything, you can drop one of the partitioned table childs 
>> while CIC is in progress, and get error
>> 
>> ERROR:  cache lookup failed for index 16399
> I agree that we need to do something about it, in particular, I think 
> we should lock all the partitions inside the transaction that builds 
> the catalog entries. Fixed this in the new version.
>> If you try to lock all child tables in CIC session, you'll get 
>> deadlocks.
> 
> Do you mean the deadlock between the transaction that drops a partition 
> and the transaction doing CIC? I think this is unavoidable and can be 
> reproduced even without partitioning.

Yes, it seems we trade this error for possible deadlock between 
transaction, dropping a partition, and CIC.

> 
> Also not sure why a list of children relation was obtained with 
> ShareLock that CIC is supposed to avoid not to block writes, changed 
> that to ShareUpdateExclusive.
> 

I expect that it wasn't an issue due to the fact that it's held for a 
brief period until DefineIndexConcurrentInternal() commits for the first 
time. But it seems, it's more correct to use ShareUpdateExclusive lock 
here.


Also I'd like to note that in new patch version there's a strange 
wording in documentation:

"This can be very convenient as not only will all existing partitions be
  indexed, but any future partitions will be as well.
  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
times
  on huge partitioned tables, to avoid that you can
  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
which
  creates the new index marked as invalid, preventing automatic 
application
  to existing partitions."

All the point of CIC is to avoid long lock times. So it seems this 
paragraph should be rewritten in the following way:

"To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
CREATE INDEX ON ONLY</command> the partitioned table..."


-- 
Best regards,
Alexander Pyhalov,
Postgres Professional






^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-05-27 23:52  Ilya Gladyshev <[email protected]>
  parent: Alexander Pyhalov <[email protected]>
  0 siblings, 1 reply; 28+ messages in thread

From: Ilya Gladyshev @ 2024-05-27 23:52 UTC (permalink / raw)
  To: Alexander Pyhalov <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>


On 24.05.2024 10:04, Alexander Pyhalov wrote:
> Ilya Gladyshev писал(а) 2024-05-24 00:14:
>> Hi,
>
> Hi.
>
>>
>> I think it's well worth the effort to revive the patch, so I rebased 
>> it on master, updated it and will return it back to the commitfest. 
>> Alexander, Justin feel free to add yourselves as authors
>>
>> On 29.01.2024 12:43, Alexander Pyhalov wrote:
>>> Hi.
>>>
>>> I've rebased patch on master and it'seems to me there's one more 
>>> issue -
>>>
>>> when we call DefineIndexConcurrentInternal() in partitioned case, it 
>>> waits for transactions, locking tableId, not tabrelid - heaprelid 
>>> LockRelId is constructed for parent index relation, not for child 
>>> index relation.
>>>
>>> Attaching fixed version.
>>>
>>> Also I'm not sure what to do with locking of child relations. If we 
>>> don't do anything, you can drop one of the partitioned table childs 
>>> while CIC is in progress, and get error
>>>
>>> ERROR:  cache lookup failed for index 16399
>> I agree that we need to do something about it, in particular, I think 
>> we should lock all the partitions inside the transaction that builds 
>> the catalog entries. Fixed this in the new version.
>>> If you try to lock all child tables in CIC session, you'll get 
>>> deadlocks.
>>
>> Do you mean the deadlock between the transaction that drops a 
>> partition and the transaction doing CIC? I think this is unavoidable 
>> and can be reproduced even without partitioning.
>
> Yes, it seems we trade this error for possible deadlock between 
> transaction, dropping a partition, and CIC.
>
>>
>> Also not sure why a list of children relation was obtained with 
>> ShareLock that CIC is supposed to avoid not to block writes, changed 
>> that to ShareUpdateExclusive.
>>
>
> I expect that it wasn't an issue due to the fact that it's held for a 
> brief period until DefineIndexConcurrentInternal() commits for the 
> first time. But it seems, it's more correct to use 
> ShareUpdateExclusive lock here.
>
>
> Also I'd like to note that in new patch version there's a strange 
> wording in documentation:
>
> "This can be very convenient as not only will all existing partitions be
>  indexed, but any future partitions will be as well.
>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
> times
>  on huge partitioned tables, to avoid that you can
>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
>  creates the new index marked as invalid, preventing automatic 
> application
>  to existing partitions."
>
> All the point of CIC is to avoid long lock times. So it seems this 
> paragraph should be rewritten in the following way:
>
> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
> CREATE INDEX ON ONLY</command> the partitioned table..."


True, the current wording doesn't look right. Right now CREATE INDEX ON 
ONLY is described as a workaround for the missing CIC. I think it rather 
makes sense to say that it gives more fine-grained control of partition 
locking than both CIC and ordinary CREATE INDEX. See the updated patch.


Attachments:

  [text/x-patch] v3-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch (24.0K, ../../[email protected]/2-v3-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch)
  download | inline diff:
From 7687b4c3ba10fc1df5ca4c8f50198dfb269be8bb Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Thu, 23 May 2024 18:13:41 +0100
Subject: [PATCH v3] Allow CREATE INDEX CONCURRENTLY on partitioned table

---
 doc/src/sgml/ddl.sgml                  |  10 +-
 doc/src/sgml/ref/create_index.sgml     |  14 +-
 src/backend/commands/indexcmds.c       | 293 ++++++++++++++++++-------
 src/test/regress/expected/indexing.out | 127 ++++++++++-
 src/test/regress/sql/indexing.sql      |  26 ++-
 5 files changed, 368 insertions(+), 102 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 6aab79e901..904978c6e5 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4314,14 +4314,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      As mentioned earlier, it is possible to create indexes on partitioned
      tables so that they are applied automatically to the entire hierarchy.
      This can be very convenient as not only will all existing partitions be
-     indexed, but any future partitions will be as well.  However, one
-     limitation when creating new indexes on partitioned tables is that it
-     is not possible to use the <literal>CONCURRENTLY</literal>
-     qualifier, which could lead to long lock times.  To avoid this, you can
-     use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
+     indexed, but any future partitions will be as well. For more control over
+     locking of the partitions you can use <command>CREATE INDEX ON ONLY</command>
+     on the partitioned table, which
      creates the new index marked as invalid, preventing automatic application
      to existing partitions.  Instead, indexes can then be created individually
-     on each partition using <literal>CONCURRENTLY</literal> and
+     on each partition and
      <firstterm>attached</firstterm> to the partitioned index on the parent
      using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once indexes for
      all the partitions are attached to the parent index, the parent index will
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 621bc0e253..2366cfd9b5 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while build an index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -692,15 +695,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
-    Concurrent builds for indexes on partitioned tables are currently not
-    supported.  However, you may concurrently build the index on each
-    partition individually and then finally create the partitioned index
-    non-concurrently in order to reduce the time where writes to the
-    partitioned table will be locked out.  In this case, building the
-    partitioned index is a metadata only operation.
-   </para>
-
   </refsect2>
  </refsect1>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 309389e20d..5806eeb8ef 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 bool primary, bool isconstraint);
 static char *ChooseIndexNameAddition(const List *colnames);
 static List *ChooseIndexColumnNames(const List *indexElems);
+static void DefineIndexConcurrentInternal(Oid relationId,
+										  Oid indexRelationId,
+										  IndexInfo *indexInfo,
+										  LOCKTAG heaplocktag,
+										  LockRelId *heaprelid);
 static void ReindexIndex(const ReindexStmt *stmt, const ReindexParams *params,
 						 bool isTopLevel);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
@@ -561,7 +566,6 @@ DefineIndex(Oid tableId,
 	bool		amissummarizing;
 	amoptions_function amoptions;
 	bool		partitioned;
-	bool		safe_index;
 	Datum		reloptions;
 	int16	   *coloptions;
 	IndexInfo  *indexInfo;
@@ -569,12 +573,10 @@ DefineIndex(Oid tableId,
 	bits16		constr_flags;
 	int			numberOfAttributes;
 	int			numberOfKeyAttributes;
-	TransactionId limitXmin;
 	ObjectAddress address;
 	LockRelId	heaprelid;
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
-	Snapshot	snapshot;
 	Oid			root_save_userid;
 	int			root_save_sec_context;
 	int			root_save_nestlevel;
@@ -706,20 +708,6 @@ DefineIndex(Oid tableId,
 	 * partition.
 	 */
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
-	if (partitioned)
-	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
-	}
 
 	/*
 	 * Don't try to CREATE INDEX on temp tables of other backends.
@@ -1116,10 +1104,6 @@ DefineIndex(Oid tableId,
 		}
 	}
 
-	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
-	safe_index = indexInfo->ii_Expressions == NIL &&
-		indexInfo->ii_Predicate == NIL;
-
 	/*
 	 * Report index creation if appropriate (delay this till after most of the
 	 * error checks)
@@ -1184,6 +1168,11 @@ DefineIndex(Oid tableId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1533,21 +1522,7 @@ DefineIndex(Oid tableId,
 			 */
 			if (invalidate_parent)
 			{
-				Relation	pg_index = table_open(IndexRelationId, RowExclusiveLock);
-				HeapTuple	tup,
-							newtup;
-
-				tup = SearchSysCache1(INDEXRELID,
-									  ObjectIdGetDatum(indexRelationId));
-				if (!HeapTupleIsValid(tup))
-					elog(ERROR, "cache lookup failed for index %u",
-						 indexRelationId);
-				newtup = heap_copytuple(tup);
-				((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
-				CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
-				ReleaseSysCache(tup);
-				table_close(pg_index, RowExclusiveLock);
-				heap_freetuple(newtup);
+				index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
 
 				/*
 				 * CCI here to make this update visible, in case this recurses
@@ -1559,37 +1534,49 @@ DefineIndex(Oid tableId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
-		 * done here.
+		 * done here in the non-concurrent case.
 		 */
-		AtEOXact_GUC(false, root_save_nestlevel);
-		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
-		if (!OidIsValid(parentIndexId))
-			pgstat_progress_end_command();
-		else
+		if (!concurrent)
 		{
-			/* Update progress for an intermediate partitioned index itself */
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
-		}
+			AtEOXact_GUC(false, root_save_nestlevel);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+			table_close(rel, NoLock);
 
-		return address;
+			if (!OidIsValid(parentIndexId))
+				pgstat_progress_end_command();
+			else
+			{
+				/*
+				 * Update progress for an intermediate partitioned index
+				 * itself
+				 */
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			}
+
+			return address;
+		}
 	}
 
 	AtEOXact_GUC(false, root_save_nestlevel);
 	SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
 
-	if (!concurrent)
+	/*
+	 * All done in the non-concurrent case, and when building catalog entries
+	 * of partitions for CIC.
+	 */
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
 		/*
 		 * If this is the top-level index, the command is done overall;
-		 * otherwise, increment progress to report one child index is done.
+		 * otherwise (when being called recursively), increment progress to
+		 * report that one child index is done.  Except in the concurrent
+		 * (catalog-only) case, which is handled later.
 		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
-		else
+		else if (!concurrent)
 			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
@@ -1600,28 +1587,180 @@ DefineIndex(Oid tableId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
-	/*
-	 * For a concurrent build, it's important to make the catalog entries
-	 * visible to other transactions before we start to build the index. That
-	 * will prevent them from making incompatible HOT updates.  The new index
-	 * will be marked not indisready and not indisvalid, so that no one else
-	 * tries to either insert into it or use it for queries.
-	 *
-	 * We must commit our current transaction so that the index becomes
-	 * visible; then start another.  Note that all the data structures we just
-	 * built are lost in the commit.  The only data we keep past here are the
-	 * relation IDs.
-	 *
-	 * Before committing, get a session-level lock on the table, to ensure
-	 * that neither it nor the index can be dropped before we finish. This
-	 * cannot block, even if someone else is waiting for access, because we
-	 * already have the same lock within our transaction.
-	 *
-	 * Note: we don't currently bother with a session lock on the index,
-	 * because there are no operations that could change its state while we
-	 * hold lock on the parent table.  This might need to change later.
-	 */
-	LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+	if (!partitioned)
+	{
+		/*
+		 * For a concurrent build, it's important to make the catalog entries
+		 * visible to other transactions before we start to build the index.
+		 * That will prevent them from making incompatible HOT updates.  The
+		 * new index will be marked not indisready and not indisvalid, so that
+		 * no one else tries to either insert into it or use it for queries.
+		 *
+		 * DefineIndexConcurrentInternal will commit our current transaction
+		 * so that the index becomes visible; then start another.  Note that
+		 * all the data structures we just built are lost in the commit.  The
+		 * only data we keep past here are the relation IDs.
+		 *
+		 * Before committing, get a session-level lock on the table, to ensure
+		 * that neither it nor the index can be dropped before we finish. This
+		 * cannot block, even if someone else is waiting for access, because
+		 * we already have the same lock within our transaction.
+		 *
+		 * Note: we don't currently bother with a session lock on the index,
+		 * because there are no operations that could change its state while
+		 * we hold lock on the parent table.  This might need to change later.
+		 */
+
+		LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
+
+		/* CREATE INDEX CONCURRENTLY on a nonpartitioned table */
+		DefineIndexConcurrentInternal(tableId, indexRelationId,
+									  indexInfo, heaplocktag, &heaprelid);
+		pgstat_progress_end_command();
+		return address;
+	}
+	else
+	{
+		/*
+		 * For CIC on a partitioned table, finish by building indexes on
+		 * partitions
+		 */
+
+		ListCell   *lc;
+		ListCell   *lc2;
+		List	   *childs;
+		List	   *part_idxs = NIL;
+		List	   *leaf_idxs = NIL;
+		List	   *leaf_idx_lockids = NIL;
+		List	   *part_idx_lockids = NIL;
+		MemoryContext cic_context,
+					old_context;
+
+		/* Create special memory context for cross-transaction storage */
+		cic_context = AllocSetContextCreate(PortalContext,
+											"Create index concurrently",
+											ALLOCSET_DEFAULT_SIZES);
+
+		old_context = MemoryContextSwitchTo(cic_context);
+		childs = find_all_inheritors(indexRelationId, ShareUpdateExclusiveLock, NULL);
+		MemoryContextSwitchTo(old_context);
+
+		foreach(lc, childs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			char		relkind;
+			LockRelId  *lockrelid;
+			Oid			tabrelid;
+
+			/*
+			 * Pre-existing partitions which were ATTACHED were already
+			 * counted in the progress report.
+			 */
+			if (get_index_isvalid(indrelid))
+				continue;
+
+			tabrelid = IndexGetRelation(indrelid, false);
+			rel = table_open(tabrelid, ShareUpdateExclusiveLock);
+			lockrelid = palloc_object(LockRelId);
+			*lockrelid = rel->rd_lockInfo.lockRelId;
+			table_close(rel, ShareUpdateExclusiveLock);
+
+			/*
+			 * Split partitions in 2 lists: partitioned tables that just need
+			 * to be marked as valid and leaf tables that actually need to
+			 * have their indexes built.
+			 */
+			relkind = get_rel_relkind(indrelid);
+			if (!RELKIND_HAS_STORAGE(relkind))
+			{
+				/* The toplevel index doesn't count towards "partitions done" */
+				if (indrelid != indexRelationId)
+					pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
+				/*
+				 * Build up a list of all the intermediate partitioned tables
+				 * which will later need to be set valid.
+				 */
+				old_context = MemoryContextSwitchTo(cic_context);
+				part_idxs = lappend_oid(part_idxs, indrelid);
+				part_idx_lockids = lappend(part_idx_lockids, lockrelid);
+				MemoryContextSwitchTo(old_context);
+			}
+			else
+			{
+				old_context = MemoryContextSwitchTo(cic_context);
+
+				leaf_idxs = lappend_oid(leaf_idxs, indrelid);
+				leaf_idx_lockids = lappend(leaf_idx_lockids, lockrelid);
+
+				MemoryContextSwitchTo(old_context);
+
+			}
+
+			/*
+			 * All partitions, including top-level parent, need to be locked
+			 * for the session before proceeding with any index rebuilds as
+			 * each of those are done in a separate transaction. After each
+			 * leaf index is built, its corresponding table will be unlocked,
+			 * all the partitioned tables will be unlocked at the very end.
+			 */
+
+			LockRelationIdForSession(lockrelid, ShareUpdateExclusiveLock);
+
+		}
+
+		forboth(lc, leaf_idxs, lc2, leaf_idx_lockids)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			LockRelId  *heaprelid = lfirst(lc2);
+			Oid			tabrelid = IndexGetRelation(indrelid, false);
+
+			SET_LOCKTAG_RELATION(heaplocktag, heaprelid->dbId, heaprelid->relId);
+
+			/* Process each partition in a separate transaction */
+			DefineIndexConcurrentInternal(tabrelid, indrelid, indexInfo,
+										  heaplocktag, heaprelid);
+
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
+		/* Set as valid all partitioned indexes, including the parent */
+		foreach(lc, part_idxs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_VALID);
+		}
+
+		foreach(lc, part_idx_lockids)
+		{
+			LockRelId  *lockrelid = lfirst(lc);
+
+			UnlockRelationIdForSession(lockrelid, ShareUpdateExclusiveLock);
+		}
+
+
+		MemoryContextDelete(cic_context);
+		pgstat_progress_end_command();
+		PopActiveSnapshot();
+		return address;
+	}
+}
+
+
+static void
+DefineIndexConcurrentInternal(Oid tableId, Oid indexRelationId, IndexInfo *indexInfo,
+							  LOCKTAG heaplocktag, LockRelId *heaprelid)
+{
+	TransactionId limitXmin;
+	Snapshot	snapshot;
+
+	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+	bool		safe_index = indexInfo->ii_Expressions == NIL &&
+		indexInfo->ii_Predicate == NIL;
 
 	PopActiveSnapshot();
 	CommitTransactionCommand();
@@ -1789,16 +1928,12 @@ DefineIndex(Oid tableId,
 	 * would be useful.  (Note that our earlier commits did not create reasons
 	 * to replan; so relcache flush on the index itself was sufficient.)
 	 */
-	CacheInvalidateRelcacheByRelid(heaprelid.relId);
+	CacheInvalidateRelcacheByRelid(heaprelid->relId);
 
 	/*
 	 * Last thing to do is release the session-level lock on the parent table.
 	 */
-	UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
-
-	pgstat_progress_end_command();
-
-	return address;
+	UnlockRelationIdForSession(heaprelid, ShareUpdateExclusiveLock);
 }
 
 
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f25723da92..44a6cf39df 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,130 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR:  cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx1"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart
+        Partitioned table "public.idxpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "idxpart_a_idx" btree (a)
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 3 (Use \d+ to list them.)
+
+\d idxpart1
+        Partitioned table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart11
+       Partitioned table "public.idxpart11"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart11_a_idx" btree (a)
+    "idxpart11_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart111
+       Partitioned table "public.idxpart111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart11 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart111_a_idx" btree (a)
+    "idxpart111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart1111
+      Partitioned table "public.idxpart1111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart111 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1111_a_idx" btree (a)
+    "idxpart1111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 0
+
+\d idxpart2
+              Table "public.idxpart2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+    "idxpart2_a_idx" btree (a)
+    "idxpart2_a_idx1" UNIQUE, btree (a) INVALID
+
+\d idxpart3
+        Partitioned table "public.idxpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (30) TO (40)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart3_a_idx" btree (a)
+    "idxpart3_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart31
+             Table "public.idxpart31"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart3 DEFAULT
+Indexes:
+    "idxpart31_a_idx" btree (a)
+    "idxpart31_a_idx1" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 5f1f4b80c9..38a730d877 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,30 @@ select relname, relkind, relhassubclass, inhparent::regclass
 	where relname like 'idxpart%' order by relname;
 drop table idxpart;
 
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart11
+\d idxpart111
+\d idxpart1111
+\d idxpart2
+\d idxpart3
+\d idxpart31
 drop table idxpart;
 
 -- Verify bugfix with query on indexed partitioned table with no partitions
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-05-28 06:05  Alexander Pyhalov <[email protected]>
  parent: Ilya Gladyshev <[email protected]>
  0 siblings, 1 reply; 28+ messages in thread

From: Alexander Pyhalov @ 2024-05-28 06:05 UTC (permalink / raw)
  To: Ilya Gladyshev <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>

Ilya Gladyshev писал(а) 2024-05-28 02:52:

>> Also I'd like to note that in new patch version there's a strange 
>> wording in documentation:
>> 
>> "This can be very convenient as not only will all existing partitions 
>> be
>>  indexed, but any future partitions will be as well.
>>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long lock 
>> times
>>  on huge partitioned tables, to avoid that you can
>>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
>> which
>>  creates the new index marked as invalid, preventing automatic 
>> application
>>  to existing partitions."
>> 
>> All the point of CIC is to avoid long lock times. So it seems this 
>> paragraph should be rewritten in the following way:
>> 
>> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
>> CREATE INDEX ON ONLY</command> the partitioned table..."
> 
> 
> True, the current wording doesn't look right. Right now CREATE INDEX ON 
> ONLY is described as a workaround for the missing CIC. I think it 
> rather makes sense to say that it gives more fine-grained control of 
> partition locking than both CIC and ordinary CREATE INDEX. See the 
> updated patch.

Hi.

Not sure if it's worth removing mentioning of CIC in

       creates the new index marked as invalid, preventing automatic 
application
       to existing partitions.  Instead, indexes can then be created 
individually
-     on each partition using <literal>CONCURRENTLY</literal> and
+     on each partition and
       <firstterm>attached</firstterm> to the partitioned index on the 
parent
       using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once 
indexes for
       all the partitions are attached to the parent index, the parent 
index will

but at least now it looks better.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional






^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-06-15 18:56  Ilya Gladyshev <[email protected]>
  parent: Alexander Pyhalov <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Ilya Gladyshev @ 2024-06-15 18:56 UTC (permalink / raw)
  To: Alexander Pyhalov <[email protected]>; +Cc: Justin Pryzby <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>


On 28.05.2024 07:05, Alexander Pyhalov wrote:
> Ilya Gladyshev писал(а) 2024-05-28 02:52:
>
>>> Also I'd like to note that in new patch version there's a strange 
>>> wording in documentation:
>>>
>>> "This can be very convenient as not only will all existing 
>>> partitions be
>>>  indexed, but any future partitions will be as well.
>>>  <command>CREATE INDEX ... CONCURRENTLY</command> can incur long 
>>> lock times
>>>  on huge partitioned tables, to avoid that you can
>>>  use <command>CREATE INDEX ON ONLY</command> the partitioned table, 
>>> which
>>>  creates the new index marked as invalid, preventing automatic 
>>> application
>>>  to existing partitions."
>>>
>>> All the point of CIC is to avoid long lock times. So it seems this 
>>> paragraph should be rewritten in the following way:
>>>
>>> "To avoid long lock times, you can use CREATE INDEX CONCURRENTLY or 
>>> CREATE INDEX ON ONLY</command> the partitioned table..."
>>
>>
>> True, the current wording doesn't look right. Right now CREATE INDEX 
>> ON ONLY is described as a workaround for the missing CIC. I think it 
>> rather makes sense to say that it gives more fine-grained control of 
>> partition locking than both CIC and ordinary CREATE INDEX. See the 
>> updated patch.
>
> Hi.
>
> Not sure if it's worth removing mentioning of CIC in
>
>       creates the new index marked as invalid, preventing automatic 
> application
>       to existing partitions.  Instead, indexes can then be created 
> individually
> -     on each partition using <literal>CONCURRENTLY</literal> and
> +     on each partition and
>       <firstterm>attached</firstterm> to the partitioned index on the 
> parent
>       using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once 
> indexes for
>       all the partitions are attached to the parent index, the parent 
> index will
>
> but at least now it looks better.

The current patch version locks all the partitions in the first 
transaction up until each of them is built, which makes for long lock 
times for partitions that are built last. Having looked at the 
implementation of REINDEX CONCURRENTLY for partitioned tables, I think 
we can improve this by using the same approach of just skipping the 
relations that we find out are dropped when trying to lock them. 
Incidentally, this implementation in the new patch version is also simpler.

In addition, I noticed that progress tracking is once again broken for 
partitioned tables, while looking at REINDEX implementation, attaching 
the second patch to fix it.



Attachments:

  [text/x-patch] v4-0002-Fix-progress-report-for-partitioned-REINDEX.patch (7.3K, ../../[email protected]/2-v4-0002-Fix-progress-report-for-partitioned-REINDEX.patch)
  download | inline diff:
From 884be03aaeabee5c6eeb5a3f639ac9afe712c24b Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH v4 2/2] Fix progress report for partitioned REINDEX

---
 src/backend/catalog/index.c      | 11 ++++--
 src/backend/commands/indexcmds.c | 63 +++++++++++++++++++++++++++++---
 src/include/catalog/index.h      |  1 +
 3 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 55fdde4b24..c5bc72b350 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3559,6 +3559,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	volatile bool skipped_constraint = false;
 	PGRUsage	ru0;
 	bool		progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+	bool		partition = ((params->options & REINDEXOPT_PARTITION) != 0);
 	bool		set_tablespace = false;
 
 	pg_rusage_init(&ru0);
@@ -3604,8 +3605,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 			indexId
 		};
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
-									  heapId);
+		if (!partition)
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+										  heapId);
 		pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
 	}
 
@@ -3845,8 +3847,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	index_close(iRel, NoLock);
 	table_close(heapRelation, NoLock);
 
-	if (progress)
+	if (progress && !partition)
+	{
+		/* progress for partitions is tracked in the caller */
 		pgstat_progress_end_command();
+	}
 }
 
 /*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 5da2df2d3b..17b30ad6aa 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -118,6 +118,7 @@ static bool ReindexRelationConcurrently(const ReindexStmt *stmt,
 										const ReindexParams *params);
 static void update_relispartition(Oid relationId, bool newval);
 static inline void set_indexsafe_procflags(void);
+static inline void progress_index_partition_done(void);
 
 /*
  * callback argument type for RangeVarCallbackForReindexIndex()
@@ -1550,7 +1551,7 @@ DefineIndex(Oid tableId,
 				 * Update progress for an intermediate partitioned index
 				 * itself
 				 */
-				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				progress_index_partition_done();
 			}
 
 			return address;
@@ -1577,7 +1578,7 @@ DefineIndex(Oid tableId,
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 		else if (!concurrent)
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			progress_index_partition_done();
 
 		return address;
 	}
@@ -1682,7 +1683,7 @@ DefineIndex(Oid tableId,
 										  heaplocktag, heaprelid);
 
 			PushActiveSnapshot(GetTransactionSnapshot());
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			progress_index_partition_done();
 		}
 
 		/* Set as valid all partitioned indexes, including the parent */
@@ -3327,6 +3328,14 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
 	ListCell   *lc;
 	ErrorContextCallback errcallback;
 	ReindexErrorInfo errinfo;
+	ReindexParams newparams;
+	int			progress_params[3] = {
+		PROGRESS_CREATEIDX_COMMAND,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+	};
+	int64		progress_values[3];
+	Oid			heapId = relid;
 
 	Assert(RELKIND_HAS_PARTITIONS(relkind));
 
@@ -3388,11 +3397,28 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
 		MemoryContextSwitchTo(old_context);
 	}
 
+	if (relkind == RELKIND_PARTITIONED_INDEX)
+	{
+		heapId = IndexGetRelation(relid, true);
+	}
+
+	progress_values[0] = (params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
+		PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY :
+		PROGRESS_CREATEIDX_COMMAND_REINDEX;
+	progress_values[1] = 0;
+	progress_values[2] = list_length(partitions);
+	pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+	pgstat_progress_update_multi_param(3, progress_params, progress_values);
+
 	/*
 	 * Process each partition listed in a separate transaction.  Note that
 	 * this commits and then starts a new transaction immediately.
 	 */
-	ReindexMultipleInternal(stmt, partitions, params);
+	newparams = *params;
+	newparams.options |= REINDEXOPT_PARTITION;
+	ReindexMultipleInternal(stmt, partitions, &newparams);
+
+	pgstat_progress_end_command();
 
 	/*
 	 * Clean up working storage --- note we must do this after
@@ -3413,6 +3439,7 @@ static void
 ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const ReindexParams *params)
 {
 	ListCell   *l;
+	bool		partitions = ((params->options & REINDEXOPT_PARTITION) != 0);
 
 	PopActiveSnapshot();
 	CommitTransactionCommand();
@@ -3506,6 +3533,9 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
 		}
 
 		CommitTransactionCommand();
+
+		if (partitions)
+			progress_index_partition_done();
 	}
 
 	StartTransactionCommand();
@@ -3558,6 +3588,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 	char	   *relationName = NULL;
 	char	   *relationNamespace = NULL;
 	PGRUsage	ru0;
+	bool		partition = ((params->options & REINDEXOPT_PARTITION) != 0);
 	const int	progress_index[] = {
 		PROGRESS_CREATEIDX_COMMAND,
 		PROGRESS_CREATEIDX_PHASE,
@@ -3901,7 +3932,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 		if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
 			elog(ERROR, "cannot reindex a temporary table concurrently");
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
+		if (!partition)
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
 
 		progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
 		progress_vals[1] = 0;	/* initializing */
@@ -4375,7 +4407,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 
 	MemoryContextDelete(private_context);
 
-	pgstat_progress_end_command();
+	if (!partition)
+		pgstat_progress_end_command();
 
 	return true;
 }
@@ -4565,3 +4598,21 @@ set_indexsafe_procflags(void)
 	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 	LWLockRelease(ProcArrayLock);
 }
+
+static inline void
+progress_index_partition_done(void)
+{
+	int			nparam = 6;
+	const int	progress_idx[] = {
+		PROGRESS_CREATEIDX_INDEX_OID,
+		PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_SUBPHASE,
+		PROGRESS_CREATEIDX_TUPLES_TOTAL,
+		PROGRESS_CREATEIDX_TUPLES_DONE
+	};
+	const int64 progress_vals[] = {0, 0, 0, 0, 0, 0};
+
+	pgstat_progress_update_multi_param(nparam, progress_idx, progress_vals);
+	pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+}
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7d434f8e65..ccba65fbbf 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
 #define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
 #define REINDEXOPT_MISSING_OK 	0x04	/* skip missing relations */
 #define REINDEXOPT_CONCURRENTLY	0x08	/* concurrent mode */
+#define REINDEXOPT_PARTITION 0x10 /* reindexing is done as part of partitioned table/index reindex */
 
 /* state info for validate_index bulkdelete callback */
 typedef struct ValidateIndexState
-- 
2.43.0



  [text/x-patch] v4-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch (20.8K, ../../[email protected]/3-v4-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch)
  download | inline diff:
From 45576292012468f2d3deea0e568ee74891cb73b1 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Thu, 23 May 2024 18:13:41 +0100
Subject: [PATCH v4 1/2] Allow CREATE INDEX CONCURRENTLY on partitioned table

---
 doc/src/sgml/ddl.sgml                  |  10 +-
 doc/src/sgml/ref/create_index.sgml     |  14 +-
 src/backend/commands/indexcmds.c       | 224 +++++++++++++++++++------
 src/test/regress/expected/indexing.out | 127 +++++++++++++-
 src/test/regress/sql/indexing.sql      |  26 ++-
 5 files changed, 323 insertions(+), 78 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 6aab79e901..904978c6e5 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4314,14 +4314,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      As mentioned earlier, it is possible to create indexes on partitioned
      tables so that they are applied automatically to the entire hierarchy.
      This can be very convenient as not only will all existing partitions be
-     indexed, but any future partitions will be as well.  However, one
-     limitation when creating new indexes on partitioned tables is that it
-     is not possible to use the <literal>CONCURRENTLY</literal>
-     qualifier, which could lead to long lock times.  To avoid this, you can
-     use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
+     indexed, but any future partitions will be as well. For more control over
+     locking of the partitions you can use <command>CREATE INDEX ON ONLY</command>
+     on the partitioned table, which
      creates the new index marked as invalid, preventing automatic application
      to existing partitions.  Instead, indexes can then be created individually
-     on each partition using <literal>CONCURRENTLY</literal> and
+     on each partition and
      <firstterm>attached</firstterm> to the partitioned index on the parent
      using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once indexes for
      all the partitions are attached to the parent index, the parent index will
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 621bc0e253..2366cfd9b5 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while build an index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -692,15 +695,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
-    Concurrent builds for indexes on partitioned tables are currently not
-    supported.  However, you may concurrently build the index on each
-    partition individually and then finally create the partitioned index
-    non-concurrently in order to reduce the time where writes to the
-    partitioned table will be locked out.  In this case, building the
-    partitioned index is a metadata only operation.
-   </para>
-
   </refsect2>
  </refsect1>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 309389e20d..5da2df2d3b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 bool primary, bool isconstraint);
 static char *ChooseIndexNameAddition(const List *colnames);
 static List *ChooseIndexColumnNames(const List *indexElems);
+static void DefineIndexConcurrentInternal(Oid relationId,
+										  Oid indexRelationId,
+										  IndexInfo *indexInfo,
+										  LOCKTAG heaplocktag,
+										  LockRelId heaprelid);
 static void ReindexIndex(const ReindexStmt *stmt, const ReindexParams *params,
 						 bool isTopLevel);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
@@ -561,7 +566,6 @@ DefineIndex(Oid tableId,
 	bool		amissummarizing;
 	amoptions_function amoptions;
 	bool		partitioned;
-	bool		safe_index;
 	Datum		reloptions;
 	int16	   *coloptions;
 	IndexInfo  *indexInfo;
@@ -569,12 +573,10 @@ DefineIndex(Oid tableId,
 	bits16		constr_flags;
 	int			numberOfAttributes;
 	int			numberOfKeyAttributes;
-	TransactionId limitXmin;
 	ObjectAddress address;
 	LockRelId	heaprelid;
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
-	Snapshot	snapshot;
 	Oid			root_save_userid;
 	int			root_save_sec_context;
 	int			root_save_nestlevel;
@@ -706,20 +708,6 @@ DefineIndex(Oid tableId,
 	 * partition.
 	 */
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
-	if (partitioned)
-	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
-	}
 
 	/*
 	 * Don't try to CREATE INDEX on temp tables of other backends.
@@ -1116,10 +1104,6 @@ DefineIndex(Oid tableId,
 		}
 	}
 
-	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
-	safe_index = indexInfo->ii_Expressions == NIL &&
-		indexInfo->ii_Predicate == NIL;
-
 	/*
 	 * Report index creation if appropriate (delay this till after most of the
 	 * error checks)
@@ -1184,6 +1168,11 @@ DefineIndex(Oid tableId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1533,21 +1522,7 @@ DefineIndex(Oid tableId,
 			 */
 			if (invalidate_parent)
 			{
-				Relation	pg_index = table_open(IndexRelationId, RowExclusiveLock);
-				HeapTuple	tup,
-							newtup;
-
-				tup = SearchSysCache1(INDEXRELID,
-									  ObjectIdGetDatum(indexRelationId));
-				if (!HeapTupleIsValid(tup))
-					elog(ERROR, "cache lookup failed for index %u",
-						 indexRelationId);
-				newtup = heap_copytuple(tup);
-				((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
-				CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
-				ReleaseSysCache(tup);
-				table_close(pg_index, RowExclusiveLock);
-				heap_freetuple(newtup);
+				index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
 
 				/*
 				 * CCI here to make this update visible, in case this recurses
@@ -1559,37 +1534,49 @@ DefineIndex(Oid tableId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
-		 * done here.
+		 * done here in the non-concurrent case.
 		 */
-		AtEOXact_GUC(false, root_save_nestlevel);
-		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
-		if (!OidIsValid(parentIndexId))
-			pgstat_progress_end_command();
-		else
+		if (!concurrent)
 		{
-			/* Update progress for an intermediate partitioned index itself */
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
-		}
+			AtEOXact_GUC(false, root_save_nestlevel);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+			table_close(rel, NoLock);
 
-		return address;
+			if (!OidIsValid(parentIndexId))
+				pgstat_progress_end_command();
+			else
+			{
+				/*
+				 * Update progress for an intermediate partitioned index
+				 * itself
+				 */
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			}
+
+			return address;
+		}
 	}
 
 	AtEOXact_GUC(false, root_save_nestlevel);
 	SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
 
-	if (!concurrent)
+	/*
+	 * All done in the non-concurrent case, and when building catalog entries
+	 * of partitions for CIC.
+	 */
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
 		/*
 		 * If this is the top-level index, the command is done overall;
-		 * otherwise, increment progress to report one child index is done.
+		 * otherwise (when being called recursively), increment progress to
+		 * report that one child index is done.  Except in the concurrent
+		 * (catalog-only) case, which is handled later.
 		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
-		else
+		else if (!concurrent)
 			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
@@ -1600,6 +1587,137 @@ DefineIndex(Oid tableId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
+	if (!partitioned)
+	{
+		/* CREATE INDEX CONCURRENTLY on a nonpartitioned table */
+		DefineIndexConcurrentInternal(tableId, indexRelationId,
+									  indexInfo, heaplocktag, heaprelid);
+		pgstat_progress_end_command();
+		return address;
+	}
+	else
+	{
+		/*
+		 * For CIC on a partitioned table, finish by building indexes on
+		 * partitions
+		 */
+
+		ListCell   *lc;
+		List	   *childs;
+		List	   *tosetvalid = NIL;
+		MemoryContext cic_context,
+					old_context;
+
+		/* Create special memory context for cross-transaction storage */
+		cic_context = AllocSetContextCreate(PortalContext,
+											"Create index concurrently",
+											ALLOCSET_DEFAULT_SIZES);
+
+		old_context = MemoryContextSwitchTo(cic_context);
+		childs = find_all_inheritors(indexRelationId, ShareUpdateExclusiveLock, NULL);
+		MemoryContextSwitchTo(old_context);
+
+		foreach(lc, childs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			Oid			tabrelid;
+			char		relkind;
+
+			/*
+			 * Pre-existing partitions which were ATTACHED were already
+			 * counted in the progress report.
+			 */
+			if (get_index_isvalid(indrelid))
+				continue;
+
+			/*
+			 * Partitioned indexes are counted in the progress report, but
+			 * don't need to be further processed.
+			 */
+			relkind = get_rel_relkind(indrelid);
+			if (!RELKIND_HAS_STORAGE(relkind))
+			{
+				/* The toplevel index doesn't count towards "partitions done" */
+				if (indrelid != indexRelationId)
+					pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
+				/*
+				 * Build up a list of all the intermediate partitioned tables
+				 * which will later need to be set valid.
+				 */
+				old_context = MemoryContextSwitchTo(cic_context);
+				tosetvalid = lappend_oid(tosetvalid, indrelid);
+				MemoryContextSwitchTo(old_context);
+				continue;
+			}
+
+			/*
+			 * Partition could have been dropped, since we looked it up. In
+			 * this case consider it done and go to the next one.
+			 */
+			tabrelid = IndexGetRelation(indrelid, true);
+			if (!tabrelid)
+			{
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				continue;
+			}
+			rel = try_table_open(tabrelid, ShareUpdateExclusiveLock);
+			if (!rel)
+			{
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				continue;
+			}
+
+			heaprelid = rel->rd_lockInfo.lockRelId;
+
+			/*
+			 * Close the table but retain the lock, that should be extended to
+			 * session level in DefineIndexConcurrentInternal.
+			 */
+			table_close(rel, NoLock);
+			SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
+
+			/* Process each partition in a separate transaction */
+			DefineIndexConcurrentInternal(tabrelid, indrelid, indexInfo,
+										  heaplocktag, heaprelid);
+
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
+		/* Set as valid all partitioned indexes, including the parent */
+		foreach(lc, tosetvalid)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			Relation	indrel = try_index_open(indrelid, ShareUpdateExclusiveLock);
+
+			if (!indrel)
+				continue;
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_VALID);
+			index_close(indrel, ShareUpdateExclusiveLock);
+		}
+
+		MemoryContextDelete(cic_context);
+		pgstat_progress_end_command();
+		PopActiveSnapshot();
+		return address;
+	}
+}
+
+
+static void
+DefineIndexConcurrentInternal(Oid tableId, Oid indexRelationId, IndexInfo *indexInfo,
+							  LOCKTAG heaplocktag, LockRelId heaprelid)
+{
+	TransactionId limitXmin;
+	Snapshot	snapshot;
+
+	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+	bool		safe_index = indexInfo->ii_Expressions == NIL &&
+		indexInfo->ii_Predicate == NIL;
+
 	/*
 	 * For a concurrent build, it's important to make the catalog entries
 	 * visible to other transactions before we start to build the index. That
@@ -1795,10 +1913,6 @@ DefineIndex(Oid tableId,
 	 * Last thing to do is release the session-level lock on the parent table.
 	 */
 	UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
-
-	pgstat_progress_end_command();
-
-	return address;
 }
 
 
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f25723da92..44a6cf39df 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,130 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR:  cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx1"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart
+        Partitioned table "public.idxpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "idxpart_a_idx" btree (a)
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 3 (Use \d+ to list them.)
+
+\d idxpart1
+        Partitioned table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart11
+       Partitioned table "public.idxpart11"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart11_a_idx" btree (a)
+    "idxpart11_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart111
+       Partitioned table "public.idxpart111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart11 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart111_a_idx" btree (a)
+    "idxpart111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart1111
+      Partitioned table "public.idxpart1111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart111 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1111_a_idx" btree (a)
+    "idxpart1111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 0
+
+\d idxpart2
+              Table "public.idxpart2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+    "idxpart2_a_idx" btree (a)
+    "idxpart2_a_idx1" UNIQUE, btree (a) INVALID
+
+\d idxpart3
+        Partitioned table "public.idxpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (30) TO (40)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart3_a_idx" btree (a)
+    "idxpart3_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart31
+             Table "public.idxpart31"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart3 DEFAULT
+Indexes:
+    "idxpart31_a_idx" btree (a)
+    "idxpart31_a_idx1" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 5f1f4b80c9..38a730d877 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,30 @@ select relname, relkind, relhassubclass, inhparent::regclass
 	where relname like 'idxpart%' order by relname;
 drop table idxpart;
 
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart11
+\d idxpart111
+\d idxpart1111
+\d idxpart2
+\d idxpart3
+\d idxpart31
 drop table idxpart;
 
 -- Verify bugfix with query on indexed partitioned table with no partitions
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-06-15 19:40  Justin Pryzby <[email protected]>
  parent: Ilya Gladyshev <[email protected]>
  1 sibling, 1 reply; 28+ messages in thread

From: Justin Pryzby @ 2024-06-15 19:40 UTC (permalink / raw)
  To: Ilya Gladyshev <[email protected]>; +Cc: Alexander Pyhalov <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>; 曾文旌(义从) <[email protected]>; Alvaro Herrera <[email protected]>

On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:
> Hi,
> 
> I think it's well worth the effort to revive the patch, so I rebased it on
> master, updated it and will return it back to the commitfest. Alexander,
> Justin feel free to add yourselves as authors

Thanks -- I was intending to write about this.

I realized that the patch will need some isolation tests to exercise its
concurrent behavior.

-- 
Justin






^ permalink  raw  reply  [nested|flat] 28+ messages in thread

* Re: CREATE INDEX CONCURRENTLY on partitioned index
@ 2024-06-20 19:20  Ilya Gladyshev <[email protected]>
  parent: Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 28+ messages in thread

From: Ilya Gladyshev @ 2024-06-20 19:20 UTC (permalink / raw)
  To: Justin Pryzby <[email protected]>; +Cc: Alexander Pyhalov <[email protected]>; [email protected]; Masahiko Sawada <[email protected]>; Michael Paquier <[email protected]>; 李杰(慎追) <[email protected]>


On 15.06.2024 20:40, Justin Pryzby wrote:
> On Thu, May 23, 2024 at 10:14:57PM +0100, Ilya Gladyshev wrote:
>> Hi,
>>
>> I think it's well worth the effort to revive the patch, so I rebased it on
>> master, updated it and will return it back to the commitfest. Alexander,
>> Justin feel free to add yourselves as authors
> Thanks -- I was intending to write about this.
>
> I realized that the patch will need some isolation tests to exercise its
> concurrent behavior.


Thanks for the suggestion, added an isolation test that verifies 
behaviour of partitioned CIC with simultaneous partition drop/detach 
going on. Also fixed some issues in the new patch that I found while 
writing the test.



Attachments:

  [text/x-patch] v5-0002-Fix-progress-report-for-partitioned-REINDEX.patch (7.3K, ../../[email protected]/3-v5-0002-Fix-progress-report-for-partitioned-REINDEX.patch)
  download | inline diff:
From 45f2ec9ee57a5337b77b66db3c8c5092f305a176 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Tue, 11 Jun 2024 17:48:08 +0100
Subject: [PATCH v5 2/2] Fix progress report for partitioned REINDEX

---
 src/backend/catalog/index.c      | 11 ++++--
 src/backend/commands/indexcmds.c | 63 +++++++++++++++++++++++++++++---
 src/include/catalog/index.h      |  1 +
 3 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 55fdde4b24..c5bc72b350 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3559,6 +3559,7 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	volatile bool skipped_constraint = false;
 	PGRUsage	ru0;
 	bool		progress = ((params->options & REINDEXOPT_REPORT_PROGRESS) != 0);
+	bool		partition = ((params->options & REINDEXOPT_PARTITION) != 0);
 	bool		set_tablespace = false;
 
 	pg_rusage_init(&ru0);
@@ -3604,8 +3605,9 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 			indexId
 		};
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
-									  heapId);
+		if (!partition)
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+										  heapId);
 		pgstat_progress_update_multi_param(2, progress_cols, progress_vals);
 	}
 
@@ -3845,8 +3847,11 @@ reindex_index(const ReindexStmt *stmt, Oid indexId,
 	index_close(iRel, NoLock);
 	table_close(heapRelation, NoLock);
 
-	if (progress)
+	if (progress && !partition)
+	{
+		/* progress for partitions is tracked in the caller */
 		pgstat_progress_end_command();
+	}
 }
 
 /*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index dcb4ea89e9..6abe1f017c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -118,6 +118,7 @@ static bool ReindexRelationConcurrently(const ReindexStmt *stmt,
 										const ReindexParams *params);
 static void update_relispartition(Oid relationId, bool newval);
 static inline void set_indexsafe_procflags(void);
+static inline void progress_index_partition_done(void);
 
 /*
  * callback argument type for RangeVarCallbackForReindexIndex()
@@ -1550,7 +1551,7 @@ DefineIndex(Oid tableId,
 				 * Update progress for an intermediate partitioned index
 				 * itself
 				 */
-				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				progress_index_partition_done();
 			}
 
 			return address;
@@ -1577,7 +1578,7 @@ DefineIndex(Oid tableId,
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 		else if (!concurrent)
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			progress_index_partition_done();
 
 		return address;
 	}
@@ -1686,7 +1687,7 @@ DefineIndex(Oid tableId,
 										  heaplocktag, heaprelid);
 
 			PushActiveSnapshot(GetTransactionSnapshot());
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			progress_index_partition_done();
 		}
 
 		/* Set as valid all partitioned indexes, including the parent */
@@ -3331,6 +3332,14 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
 	ListCell   *lc;
 	ErrorContextCallback errcallback;
 	ReindexErrorInfo errinfo;
+	ReindexParams newparams;
+	int			progress_params[3] = {
+		PROGRESS_CREATEIDX_COMMAND,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_PARTITIONS_TOTAL
+	};
+	int64		progress_values[3];
+	Oid			heapId = relid;
 
 	Assert(RELKIND_HAS_PARTITIONS(relkind));
 
@@ -3392,11 +3401,28 @@ ReindexPartitions(const ReindexStmt *stmt, Oid relid, const ReindexParams *param
 		MemoryContextSwitchTo(old_context);
 	}
 
+	if (relkind == RELKIND_PARTITIONED_INDEX)
+	{
+		heapId = IndexGetRelation(relid, true);
+	}
+
+	progress_values[0] = (params->options & REINDEXOPT_CONCURRENTLY) != 0 ?
+		PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY :
+		PROGRESS_CREATEIDX_COMMAND_REINDEX;
+	progress_values[1] = 0;
+	progress_values[2] = list_length(partitions);
+	pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+	pgstat_progress_update_multi_param(3, progress_params, progress_values);
+
 	/*
 	 * Process each partition listed in a separate transaction.  Note that
 	 * this commits and then starts a new transaction immediately.
 	 */
-	ReindexMultipleInternal(stmt, partitions, params);
+	newparams = *params;
+	newparams.options |= REINDEXOPT_PARTITION;
+	ReindexMultipleInternal(stmt, partitions, &newparams);
+
+	pgstat_progress_end_command();
 
 	/*
 	 * Clean up working storage --- note we must do this after
@@ -3417,6 +3443,7 @@ static void
 ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const ReindexParams *params)
 {
 	ListCell   *l;
+	bool		partitions = ((params->options & REINDEXOPT_PARTITION) != 0);
 
 	PopActiveSnapshot();
 	CommitTransactionCommand();
@@ -3510,6 +3537,9 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
 		}
 
 		CommitTransactionCommand();
+
+		if (partitions)
+			progress_index_partition_done();
 	}
 
 	StartTransactionCommand();
@@ -3562,6 +3592,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 	char	   *relationName = NULL;
 	char	   *relationNamespace = NULL;
 	PGRUsage	ru0;
+	bool		partition = ((params->options & REINDEXOPT_PARTITION) != 0);
 	const int	progress_index[] = {
 		PROGRESS_CREATEIDX_COMMAND,
 		PROGRESS_CREATEIDX_PHASE,
@@ -3905,7 +3936,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 		if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
 			elog(ERROR, "cannot reindex a temporary table concurrently");
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
+		if (!partition)
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, idx->tableId);
 
 		progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
 		progress_vals[1] = 0;	/* initializing */
@@ -4379,7 +4411,8 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 
 	MemoryContextDelete(private_context);
 
-	pgstat_progress_end_command();
+	if (!partition)
+		pgstat_progress_end_command();
 
 	return true;
 }
@@ -4569,3 +4602,21 @@ set_indexsafe_procflags(void)
 	ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 	LWLockRelease(ProcArrayLock);
 }
+
+static inline void
+progress_index_partition_done(void)
+{
+	int			nparam = 6;
+	const int	progress_idx[] = {
+		PROGRESS_CREATEIDX_INDEX_OID,
+		PROGRESS_CREATEIDX_ACCESS_METHOD_OID,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_SUBPHASE,
+		PROGRESS_CREATEIDX_TUPLES_TOTAL,
+		PROGRESS_CREATEIDX_TUPLES_DONE
+	};
+	const int64 progress_vals[] = {0, 0, 0, 0, 0, 0};
+
+	pgstat_progress_update_multi_param(nparam, progress_idx, progress_vals);
+	pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+}
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index 7d434f8e65..ccba65fbbf 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
 #define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
 #define REINDEXOPT_MISSING_OK 	0x04	/* skip missing relations */
 #define REINDEXOPT_CONCURRENTLY	0x08	/* concurrent mode */
+#define REINDEXOPT_PARTITION 0x10 /* reindexing is done as part of partitioned table/index reindex */
 
 /* state info for validate_index bulkdelete callback */
 typedef struct ValidateIndexState
-- 
2.43.0



  [text/x-patch] v5-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch (28.1K, ../../[email protected]/4-v5-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch)
  download | inline diff:
From acf5cf5d4a984c0f8635a25e03c23409601c0c93 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email protected]>
Date: Thu, 23 May 2024 18:13:41 +0100
Subject: [PATCH v5 1/2] Allow CREATE INDEX CONCURRENTLY on partitioned table

---
 doc/src/sgml/ddl.sgml                         |  10 +-
 doc/src/sgml/ref/create_index.sgml            |  14 +-
 src/backend/commands/indexcmds.c              | 228 +++++++++++++-----
 .../isolation/expected/partitioned-cic.out    | 135 +++++++++++
 src/test/isolation/isolation_schedule         |   1 +
 src/test/isolation/specs/partitioned-cic.spec |  57 +++++
 src/test/regress/expected/indexing.out        | 127 +++++++++-
 src/test/regress/sql/indexing.sql             |  26 +-
 8 files changed, 520 insertions(+), 78 deletions(-)
 create mode 100644 src/test/isolation/expected/partitioned-cic.out
 create mode 100644 src/test/isolation/specs/partitioned-cic.spec

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 6aab79e901..904978c6e5 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4314,14 +4314,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      As mentioned earlier, it is possible to create indexes on partitioned
      tables so that they are applied automatically to the entire hierarchy.
      This can be very convenient as not only will all existing partitions be
-     indexed, but any future partitions will be as well.  However, one
-     limitation when creating new indexes on partitioned tables is that it
-     is not possible to use the <literal>CONCURRENTLY</literal>
-     qualifier, which could lead to long lock times.  To avoid this, you can
-     use <command>CREATE INDEX ON ONLY</command> the partitioned table, which
+     indexed, but any future partitions will be as well. For more control over
+     locking of the partitions you can use <command>CREATE INDEX ON ONLY</command>
+     on the partitioned table, which
      creates the new index marked as invalid, preventing automatic application
      to existing partitions.  Instead, indexes can then be created individually
-     on each partition using <literal>CONCURRENTLY</literal> and
+     on each partition and
      <firstterm>attached</firstterm> to the partitioned index on the parent
      using <command>ALTER INDEX ... ATTACH PARTITION</command>.  Once indexes for
      all the partitions are attached to the parent index, the parent index will
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 621bc0e253..2366cfd9b5 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while build an index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -692,15 +695,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
-    Concurrent builds for indexes on partitioned tables are currently not
-    supported.  However, you may concurrently build the index on each
-    partition individually and then finally create the partitioned index
-    non-concurrently in order to reduce the time where writes to the
-    partitioned table will be locked out.  In this case, building the
-    partitioned index is a metadata only operation.
-   </para>
-
   </refsect2>
  </refsect1>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 309389e20d..dcb4ea89e9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -95,6 +95,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 bool primary, bool isconstraint);
 static char *ChooseIndexNameAddition(const List *colnames);
 static List *ChooseIndexColumnNames(const List *indexElems);
+static void DefineIndexConcurrentInternal(Oid relationId,
+										  Oid indexRelationId,
+										  IndexInfo *indexInfo,
+										  LOCKTAG heaplocktag,
+										  LockRelId heaprelid);
 static void ReindexIndex(const ReindexStmt *stmt, const ReindexParams *params,
 						 bool isTopLevel);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
@@ -561,7 +566,6 @@ DefineIndex(Oid tableId,
 	bool		amissummarizing;
 	amoptions_function amoptions;
 	bool		partitioned;
-	bool		safe_index;
 	Datum		reloptions;
 	int16	   *coloptions;
 	IndexInfo  *indexInfo;
@@ -569,12 +573,10 @@ DefineIndex(Oid tableId,
 	bits16		constr_flags;
 	int			numberOfAttributes;
 	int			numberOfKeyAttributes;
-	TransactionId limitXmin;
 	ObjectAddress address;
 	LockRelId	heaprelid;
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
-	Snapshot	snapshot;
 	Oid			root_save_userid;
 	int			root_save_sec_context;
 	int			root_save_nestlevel;
@@ -706,20 +708,6 @@ DefineIndex(Oid tableId,
 	 * partition.
 	 */
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
-	if (partitioned)
-	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
-	}
 
 	/*
 	 * Don't try to CREATE INDEX on temp tables of other backends.
@@ -1116,10 +1104,6 @@ DefineIndex(Oid tableId,
 		}
 	}
 
-	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
-	safe_index = indexInfo->ii_Expressions == NIL &&
-		indexInfo->ii_Predicate == NIL;
-
 	/*
 	 * Report index creation if appropriate (delay this till after most of the
 	 * error checks)
@@ -1184,6 +1168,11 @@ DefineIndex(Oid tableId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1533,21 +1522,7 @@ DefineIndex(Oid tableId,
 			 */
 			if (invalidate_parent)
 			{
-				Relation	pg_index = table_open(IndexRelationId, RowExclusiveLock);
-				HeapTuple	tup,
-							newtup;
-
-				tup = SearchSysCache1(INDEXRELID,
-									  ObjectIdGetDatum(indexRelationId));
-				if (!HeapTupleIsValid(tup))
-					elog(ERROR, "cache lookup failed for index %u",
-						 indexRelationId);
-				newtup = heap_copytuple(tup);
-				((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
-				CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
-				ReleaseSysCache(tup);
-				table_close(pg_index, RowExclusiveLock);
-				heap_freetuple(newtup);
+				index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
 
 				/*
 				 * CCI here to make this update visible, in case this recurses
@@ -1559,37 +1534,49 @@ DefineIndex(Oid tableId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
-		 * done here.
+		 * done here in the non-concurrent case.
 		 */
-		AtEOXact_GUC(false, root_save_nestlevel);
-		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
-		if (!OidIsValid(parentIndexId))
-			pgstat_progress_end_command();
-		else
+		if (!concurrent)
 		{
-			/* Update progress for an intermediate partitioned index itself */
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
-		}
+			AtEOXact_GUC(false, root_save_nestlevel);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+			table_close(rel, NoLock);
 
-		return address;
+			if (!OidIsValid(parentIndexId))
+				pgstat_progress_end_command();
+			else
+			{
+				/*
+				 * Update progress for an intermediate partitioned index
+				 * itself
+				 */
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			}
+
+			return address;
+		}
 	}
 
 	AtEOXact_GUC(false, root_save_nestlevel);
 	SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
 
-	if (!concurrent)
+	/*
+	 * All done in the non-concurrent case, and when building catalog entries
+	 * of partitions for CIC.
+	 */
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
 		/*
 		 * If this is the top-level index, the command is done overall;
-		 * otherwise, increment progress to report one child index is done.
+		 * otherwise (when being called recursively), increment progress to
+		 * report that one child index is done.  Except in the concurrent
+		 * (catalog-only) case, which is handled later.
 		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
-		else
+		else if (!concurrent)
 			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
@@ -1600,6 +1587,141 @@ DefineIndex(Oid tableId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
+	if (!partitioned)
+	{
+		/* CREATE INDEX CONCURRENTLY on a nonpartitioned table */
+		DefineIndexConcurrentInternal(tableId, indexRelationId,
+									  indexInfo, heaplocktag, heaprelid);
+		pgstat_progress_end_command();
+		return address;
+	}
+	else
+	{
+		/*
+		 * For CIC on a partitioned table, finish by building indexes on
+		 * partitions
+		 */
+
+		ListCell   *lc;
+		List	   *childs;
+		List	   *tosetvalid = NIL;
+		MemoryContext cic_context,
+					old_context;
+
+		/* Create special memory context for cross-transaction storage */
+		cic_context = AllocSetContextCreate(PortalContext,
+											"Create index concurrently",
+											ALLOCSET_DEFAULT_SIZES);
+
+		old_context = MemoryContextSwitchTo(cic_context);
+		childs = find_all_inheritors(indexRelationId, ShareUpdateExclusiveLock, NULL);
+		MemoryContextSwitchTo(old_context);
+
+		foreach(lc, childs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			Oid			tabrelid;
+			char		relkind;
+
+			/*
+			 * Partition could have been dropped, since we looked it up. In
+			 * this case consider it done and go to the next one.
+			 */
+			tabrelid = IndexGetRelation(indrelid, true);
+			if (!tabrelid)
+			{
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				continue;
+			}
+			rel = try_table_open(tabrelid, ShareUpdateExclusiveLock);
+			if (!rel)
+			{
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+				continue;
+			}
+
+			/*
+			 * Pre-existing partitions which were ATTACHED were already
+			 * counted in the progress report.
+			 */
+			if (get_index_isvalid(indrelid))
+			{
+				table_close(rel, ShareUpdateExclusiveLock);
+				continue;
+			}
+
+			/*
+			 * Partitioned indexes are counted in the progress report, but
+			 * don't need to be further processed.
+			 */
+			relkind = get_rel_relkind(indrelid);
+			if (!RELKIND_HAS_STORAGE(relkind))
+			{
+				/* The toplevel index doesn't count towards "partitions done" */
+				if (indrelid != indexRelationId)
+					pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
+				/*
+				 * Build up a list of all the intermediate partitioned tables
+				 * which will later need to be set valid.
+				 */
+				old_context = MemoryContextSwitchTo(cic_context);
+				tosetvalid = lappend_oid(tosetvalid, indrelid);
+				MemoryContextSwitchTo(old_context);
+				table_close(rel, ShareUpdateExclusiveLock);
+				continue;
+			}
+
+			heaprelid = rel->rd_lockInfo.lockRelId;
+
+			/*
+			 * Close the table but retain the lock, that should be extended to
+			 * session level in DefineIndexConcurrentInternal.
+			 */
+			table_close(rel, NoLock);
+			SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
+
+			/* Process each partition in a separate transaction */
+			DefineIndexConcurrentInternal(tabrelid, indrelid, indexInfo,
+										  heaplocktag, heaprelid);
+
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
+		/* Set as valid all partitioned indexes, including the parent */
+		foreach(lc, tosetvalid)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			Relation	indrel = try_index_open(indrelid, ShareUpdateExclusiveLock);
+
+			if (!indrel)
+				continue;
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_VALID);
+			index_close(indrel, ShareUpdateExclusiveLock);
+		}
+
+		MemoryContextDelete(cic_context);
+		pgstat_progress_end_command();
+		PopActiveSnapshot();
+		return address;
+	}
+}
+
+
+static void
+DefineIndexConcurrentInternal(Oid tableId, Oid indexRelationId, IndexInfo *indexInfo,
+							  LOCKTAG heaplocktag, LockRelId heaprelid)
+{
+	TransactionId limitXmin;
+	Snapshot	snapshot;
+
+	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+	bool		safe_index = indexInfo->ii_Expressions == NIL &&
+		indexInfo->ii_Predicate == NIL;
+
 	/*
 	 * For a concurrent build, it's important to make the catalog entries
 	 * visible to other transactions before we start to build the index. That
@@ -1795,10 +1917,6 @@ DefineIndex(Oid tableId,
 	 * Last thing to do is release the session-level lock on the parent table.
 	 */
 	UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
-
-	pgstat_progress_end_command();
-
-	return address;
 }
 
 
diff --git a/src/test/isolation/expected/partitioned-cic.out b/src/test/isolation/expected/partitioned-cic.out
new file mode 100644
index 0000000000..b66acc6f6a
--- /dev/null
+++ b/src/test/isolation/expected/partitioned-cic.out
@@ -0,0 +1,135 @@
+Parsed test spec with 3 sessions
+
+starting permutation: lock_p1 cic insert drop2 commit chk_content
+step lock_p1: lock cictab_part_1 in row exclusive mode;
+step cic: CREATE INDEX CONCURRENTLY ON cictab(i); <waiting ...>
+step insert: insert into cictab values (1, 1), (11, 1);
+step drop2: DROP TABLE cictab_part_2;
+step commit: COMMIT;
+step cic: <... completed>
+step chk_content: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab where i > 0;
+  select * from cictab where i > 0;
+
+QUERY PLAN                                                  
+------------------------------------------------------------
+Index Scan using cictab_part_1_i_idx on cictab_part_1 cictab
+  Index Cond: (i > 0)                                       
+(2 rows)
+
+i|j
+-+-
+1|0
+1|1
+(2 rows)
+
+
+starting permutation: lock_p2 cic insert drop1 commit chk_content
+step lock_p2: lock cictab_part_2 in row exclusive mode;
+step cic: CREATE INDEX CONCURRENTLY ON cictab(i); <waiting ...>
+step insert: insert into cictab values (1, 1), (11, 1);
+step drop1: DROP TABLE cictab_part_1;
+step commit: COMMIT;
+step cic: <... completed>
+step chk_content: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab where i > 0;
+  select * from cictab where i > 0;
+
+QUERY PLAN                                                  
+------------------------------------------------------------
+Index Scan using cictab_part_2_i_idx on cictab_part_2 cictab
+  Index Cond: (i > 0)                                       
+(2 rows)
+
+ i|j
+--+-
+11|0
+11|1
+(2 rows)
+
+
+starting permutation: lock_p1 cic insert detach2 commit chk_content chk_content_part2
+step lock_p1: lock cictab_part_1 in row exclusive mode;
+step cic: CREATE INDEX CONCURRENTLY ON cictab(i); <waiting ...>
+step insert: insert into cictab values (1, 1), (11, 1);
+step detach2: ALTER TABLE cictab DETACH PARTITION cictab_part_2;
+step commit: COMMIT;
+step cic: <... completed>
+step chk_content: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab where i > 0;
+  select * from cictab where i > 0;
+
+QUERY PLAN                                                  
+------------------------------------------------------------
+Index Scan using cictab_part_1_i_idx on cictab_part_1 cictab
+  Index Cond: (i > 0)                                       
+(2 rows)
+
+i|j
+-+-
+1|0
+1|1
+(2 rows)
+
+step chk_content_part2: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab_part_2 where i > 0;
+  select * from cictab_part_2 where i > 0;
+
+QUERY PLAN                                           
+-----------------------------------------------------
+Index Scan using cictab_part_2_i_idx on cictab_part_2
+  Index Cond: (i > 0)                                
+(2 rows)
+
+ i|j
+--+-
+11|0
+11|1
+(2 rows)
+
+
+starting permutation: lock_p2 cic insert detach1 commit chk_content chk_content_part1
+step lock_p2: lock cictab_part_2 in row exclusive mode;
+step cic: CREATE INDEX CONCURRENTLY ON cictab(i); <waiting ...>
+step insert: insert into cictab values (1, 1), (11, 1);
+step detach1: ALTER TABLE cictab DETACH PARTITION cictab_part_1;
+step commit: COMMIT;
+step cic: <... completed>
+step chk_content: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab where i > 0;
+  select * from cictab where i > 0;
+
+QUERY PLAN                                                  
+------------------------------------------------------------
+Index Scan using cictab_part_2_i_idx on cictab_part_2 cictab
+  Index Cond: (i > 0)                                       
+(2 rows)
+
+ i|j
+--+-
+11|0
+11|1
+(2 rows)
+
+step chk_content_part1: 
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab_part_1 where i > 0;
+  select * from cictab_part_1 where i > 0;
+
+QUERY PLAN                                           
+-----------------------------------------------------
+Index Scan using cictab_part_1_i_idx on cictab_part_1
+  Index Cond: (i > 0)                                
+(2 rows)
+
+i|j
+-+-
+1|0
+1|1
+(2 rows)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 0342eb39e4..57b7948687 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -114,3 +114,4 @@ test: serializable-parallel-2
 test: serializable-parallel-3
 test: matview-write-skew
 test: lock-nowait
+test: partitioned-cic
diff --git a/src/test/isolation/specs/partitioned-cic.spec b/src/test/isolation/specs/partitioned-cic.spec
new file mode 100644
index 0000000000..95f0bb2b47
--- /dev/null
+++ b/src/test/isolation/specs/partitioned-cic.spec
@@ -0,0 +1,57 @@
+# Test the ability to drop/detach partitions while CREATE INDEX CONCURRENTLY is running.
+# To achieve this, start a transaction that will pause CIC in progress by
+# locking a partition in row exclusive mode, giving us a change to drop/detach another partition.
+# Dropping/detaching is tested for each partition to test two scenarios:
+# when the partition has already been indexed and when it's yet to be indexed.
+
+setup {
+  create table cictab(i int, j int) partition by range(i);
+  create table cictab_part_1 partition of cictab for values from (0) to (10);
+  create table cictab_part_2 partition of cictab for values from (10) to (20);
+
+  insert into cictab values (1, 0), (11, 0);
+}
+
+teardown {
+    drop table if exists cictab_part_1;
+    drop table if exists cictab_part_2;
+    drop table cictab;
+}
+
+session s1
+setup {BEGIN;}
+step lock_p1 { lock cictab_part_1 in row exclusive mode; }
+step lock_p2 { lock cictab_part_2 in row exclusive mode; }
+step commit { COMMIT; }
+
+session s2
+step cic { CREATE INDEX CONCURRENTLY ON cictab(i); }
+step chk_content {
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab where i > 0;
+  select * from cictab where i > 0;
+}
+
+step chk_content_part1 {
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab_part_1 where i > 0;
+  select * from cictab_part_1 where i > 0;
+}
+
+step chk_content_part2 {
+  set enable_seqscan to off;
+  explain (costs off) select * from cictab_part_2 where i > 0;
+  select * from cictab_part_2 where i > 0;
+}
+
+session s3
+step detach1 { ALTER TABLE cictab DETACH PARTITION cictab_part_1; }
+step detach2 { ALTER TABLE cictab DETACH PARTITION cictab_part_2; }
+step drop1 { DROP TABLE cictab_part_1; }
+step drop2 { DROP TABLE cictab_part_2; }
+step insert { insert into cictab values (1, 1), (11, 1); }
+
+permutation lock_p1 cic insert drop2 commit chk_content
+permutation lock_p2 cic insert drop1 commit chk_content
+permutation lock_p1 cic insert detach2 commit chk_content chk_content_part2
+permutation lock_p2 cic insert detach1 commit chk_content chk_content_part1
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index f25723da92..44a6cf39df 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,130 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR:  cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx1"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart
+        Partitioned table "public.idxpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "idxpart_a_idx" btree (a)
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 3 (Use \d+ to list them.)
+
+\d idxpart1
+        Partitioned table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1_a_idx" btree (a)
+    "idxpart1_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart11
+       Partitioned table "public.idxpart11"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart11_a_idx" btree (a)
+    "idxpart11_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart111
+       Partitioned table "public.idxpart111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart11 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart111_a_idx" btree (a)
+    "idxpart111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart1111
+      Partitioned table "public.idxpart1111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart111 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1111_a_idx" btree (a)
+    "idxpart1111_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 0
+
+\d idxpart2
+              Table "public.idxpart2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+    "idxpart2_a_idx" btree (a)
+    "idxpart2_a_idx1" UNIQUE, btree (a) INVALID
+
+\d idxpart3
+        Partitioned table "public.idxpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (30) TO (40)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart3_a_idx" btree (a)
+    "idxpart3_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart31
+             Table "public.idxpart31"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart3 DEFAULT
+Indexes:
+    "idxpart31_a_idx" btree (a)
+    "idxpart31_a_idx1" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email protected]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 5f1f4b80c9..38a730d877 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,30 @@ select relname, relkind, relhassubclass, inhparent::regclass
 	where relname like 'idxpart%' order by relname;
 drop table idxpart;
 
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart2 (a); -- leaf
+create index concurrently on idxpart (a); -- partitioned
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart11
+\d idxpart111
+\d idxpart1111
+\d idxpart2
+\d idxpart3
+\d idxpart31
 drop table idxpart;
 
 -- Verify bugfix with query on indexed partitioned table with no partitions
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 28+ messages in thread

end of thread, other threads:[~2024-06-20 19:20 UTC | newest]

Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2021-01-20 16:25 [PATCH] Disable background workers during servers start in pg_upgrade Denis Laxalde <[email protected]>
2024-05-23 21:14 Re: CREATE INDEX CONCURRENTLY on partitioned index Ilya Gladyshev <[email protected]>
2024-05-24 09:04 ` Re: CREATE INDEX CONCURRENTLY on partitioned index Alexander Pyhalov <[email protected]>
2024-05-27 23:52   ` Re: CREATE INDEX CONCURRENTLY on partitioned index Ilya Gladyshev <[email protected]>
2024-05-28 06:05     ` Re: CREATE INDEX CONCURRENTLY on partitioned index Alexander Pyhalov <[email protected]>
2024-06-15 18:56       ` Re: CREATE INDEX CONCURRENTLY on partitioned index Ilya Gladyshev <[email protected]>
2024-06-15 19:40 ` Re: CREATE INDEX CONCURRENTLY on partitioned index Justin Pryzby <[email protected]>
2024-06-20 19:20   ` Re: CREATE INDEX CONCURRENTLY on partitioned index Ilya Gladyshev <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox