From 855f15487f2686558d9ee2dd725e766b79e548b1 Mon Sep 17 00:00:00 2001 From: Zhenghua Lyu Date: Wed, 1 Jul 2020 14:54:10 +0800 Subject: [PATCH] Make QEs does not use the GUC gp_enable_global_deadlock_detector Previously, there are some code executed in QEs that will check the value of the GUC gp_enable_global_deadlock_detector. The historical reason is that: Before we have GDD, UPDATE|DELETE operations cannot be concurrently executed, and we do not meet the EvalPlanQual issue and concurrent split update issue. After we have GDD, we meet such issues and add code to resolve them, and we add the code `if (gp_enable_global_deadlock_detector)` at the very first. It is just like habitual thinking. In fact, we do not rely on it in QEs and I tried to remove this in the context: https://github.com/greenplum-db/gpdb/pull/9992#pullrequestreview-402364938. I tried to add an assert there but could not handle utility mode as Heikki's comments. To continue that idea, we can just remove the check of gp_enable_global_deadlock_detector. This brings two benefits: 1. Some users only change this GUC on master. By removing the usage in QEs, it is safe now to make the GUC master only. 2. We can bring back the skills to only restart master node's postmaster to enable GDD, this save a lot of time in pipeline. This commit also do this for the isolation2 test cases: lockmodes and gdd/. The Github issue https://github.com/greenplum-db/gpdb/issues/10030 is gone after this commit. --- src/backend/executor/nodeModifyTable.c | 4 +- src/backend/storage/lmgr/lmgr.c | 7 +-- src/test/isolation2/expected/gdd/end.out | 55 ++++++++------------ src/test/isolation2/expected/gdd/prepare.out | 54 +++++++------------ src/test/isolation2/expected/lockmodes.out | 46 ++++++++++++---- src/test/isolation2/sql/gdd/end.sql | 15 ++++-- src/test/isolation2/sql/gdd/prepare.sql | 18 +++++-- src/test/isolation2/sql/lockmodes.sql | 26 ++++++--- 8 files changed, 123 insertions(+), 102 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index c54a1fb67f..8eda2a6d9b 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -938,7 +938,7 @@ ldelete:; errmsg("could not serialize access due to concurrent update"))); /* - * If GDD is enabled and the DELETE operator is generated by + * If the DELETE operator is generated by * SplitUpdate node (SplitUpdate node is generated for updating * the distributed keys), we raise an error. * @@ -949,7 +949,7 @@ ldelete:; * transaction, but it is difficult to skip the inserting * tuple, so it will leads to more tuples after updating. */ - if (gp_enable_global_deadlock_detector && isUpdate) + if (isUpdate) { ereport(ERROR, (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE ), diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index ce03f9bcb4..ead079e235 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -621,11 +621,12 @@ XactLockTableWait(TransactionId xid, Relation rel, ItemPointer ctid, bool first = true; /* - * Concurrent update and delete will wait on segment when GDD is enabled, - * need to report the waited transactions to QD to make sure the they have + * Concurrent update and delete will wait on segment when GDD is enabled (or + * the corner case that utility mode delete|update in segment which does not hold + * gxid), need to report the waited transactions to QD to make sure the they have * the same transaction order on the master. */ - if (gp_enable_global_deadlock_detector && Gp_role == GP_ROLE_EXECUTE) + if (Gp_role == GP_ROLE_EXECUTE) { MemoryContext oldContext; DistributedTransactionId gxid = LocalXidGetDistributedXid(xid); diff --git a/src/test/isolation2/expected/gdd/end.out b/src/test/isolation2/expected/gdd/end.out index 65d8f49828..97e04ed65c 100644 --- a/src/test/isolation2/expected/gdd/end.out +++ b/src/test/isolation2/expected/gdd/end.out @@ -1,41 +1,28 @@ --- disable GDD +include: helpers/server_helpers.sql; +CREATE --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v off; -20200424:15:51:08:052278 gpconfig:zlv:gpadmin-[INFO]:-completed successfully with parameters '-c gp_enable_global_deadlock_detector -v off' - -! gpstop -rai; -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Starting gpstop with args: -rai -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Gathering information and validating the environment... -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Obtaining Greenplum Master catalog information -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Obtaining Segment details from master... -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Greenplum Version: 'postgres (Greenplum Database) 7.0.0-alpha.0+dev.6778.g000faed10b build dev' -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Commencing Master instance shutdown with mode='immediate' -20200424:15:51:08:052977 gpstop:zlv:gpadmin-[INFO]:-Master segment instance directory=/home/gpadmin/workspace/gpdb/gpAux/gpdemo/datadirs/qddir/demoDataDir-1 -20200424:15:51:09:052977 gpstop:zlv:gpadmin-[INFO]:-Attempting forceful termination of any leftover master process -20200424:15:51:09:052977 gpstop:zlv:gpadmin-[INFO]:-Terminating processes for segment /home/gpadmin/workspace/gpdb/gpAux/gpdemo/datadirs/qddir/demoDataDir-1 -20200424:15:51:09:052977 gpstop:zlv:gpadmin-[INFO]:-Stopping master standby host zlv mode=immediate -20200424:15:51:10:052977 gpstop:zlv:gpadmin-[INFO]:-Successfully shutdown standby process on zlv -20200424:15:51:10:052977 gpstop:zlv:gpadmin-[INFO]:-Targeting dbid [2, 5, 3, 6, 4, 7] for shutdown -20200424:15:51:10:052977 gpstop:zlv:gpadmin-[INFO]:-Commencing parallel primary segment instance shutdown, please wait... -20200424:15:51:10:052977 gpstop:zlv:gpadmin-[INFO]:-0.00% of jobs completed -20200424:15:51:11:052977 gpstop:zlv:gpadmin-[INFO]:-100.00% of jobs completed -20200424:15:51:11:052977 gpstop:zlv:gpadmin-[INFO]:-Commencing parallel mirror segment instance shutdown, please wait... -20200424:15:51:11:052977 gpstop:zlv:gpadmin-[INFO]:-0.00% of jobs completed -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:-100.00% of jobs completed -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:----------------------------------------------------- -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:- Segments stopped successfully = 6 -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:- Segments with errors during stop = 0 -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:----------------------------------------------------- -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:-Successfully shutdown 6 of 6 segment instances -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:-Database successfully shutdown with no errors reported -20200424:15:51:12:052977 gpstop:zlv:gpadmin-[INFO]:-Cleaning up leftover shared memory -20200424:15:51:13:052977 gpstop:zlv:gpadmin-[INFO]:-Restarting System... - --- end_ignore +ALTER SYSTEM RESET gp_enable_global_deadlock_detector; +ALTER +ALTER SYSTEM RESET gp_global_deadlock_detector_period; +ALTER +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from datadir; + pg_ctl +-------- + OK +(1 row) +-- Start new session on master to make sure it has fully completed +-- recovery and up and running again. 1: SHOW gp_enable_global_deadlock_detector; gp_enable_global_deadlock_detector ------------------------------------ off (1 row) +1: SHOW gp_global_deadlock_detector_period; + gp_global_deadlock_detector_period +------------------------------------ + 2min +(1 row) diff --git a/src/test/isolation2/expected/gdd/prepare.out b/src/test/isolation2/expected/gdd/prepare.out index 47c19ae327..87b03eefe3 100644 --- a/src/test/isolation2/expected/gdd/prepare.out +++ b/src/test/isolation2/expected/gdd/prepare.out @@ -1,3 +1,6 @@ +include: helpers/server_helpers.sql; +CREATE + -- t0r is the reference table to provide the data distribution info. DROP TABLE IF EXISTS t0p; DROP @@ -60,43 +63,26 @@ SELECT segid(2,10) is not null; (1 row) --enable GDD --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v on; -20200424:15:49:54:048653 gpconfig:zlv:gpadmin-[INFO]:-completed successfully with parameters '-c gp_enable_global_deadlock_detector -v on' - -! gpconfig -c gp_global_deadlock_detector_period -v 5; -20200424:15:49:56:049353 gpconfig:zlv:gpadmin-[INFO]:-completed successfully with parameters '-c gp_global_deadlock_detector_period -v 5' -! gpstop -rai; -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Starting gpstop with args: -rai -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Gathering information and validating the environment... -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Obtaining Greenplum Master catalog information -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Obtaining Segment details from master... -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Greenplum Version: 'postgres (Greenplum Database) 7.0.0-alpha.0+dev.6778.g000faed10b build dev' -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Commencing Master instance shutdown with mode='immediate' -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Master segment instance directory=/home/gpadmin/workspace/gpdb/gpAux/gpdemo/datadirs/qddir/demoDataDir-1 -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Attempting forceful termination of any leftover master process -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Terminating processes for segment /home/gpadmin/workspace/gpdb/gpAux/gpdemo/datadirs/qddir/demoDataDir-1 -20200424:15:49:56:050055 gpstop:zlv:gpadmin-[INFO]:-Stopping master standby host zlv mode=immediate -20200424:15:49:58:050055 gpstop:zlv:gpadmin-[INFO]:-Successfully shutdown standby process on zlv -20200424:15:49:58:050055 gpstop:zlv:gpadmin-[INFO]:-Targeting dbid [2, 5, 3, 6, 4, 7] for shutdown -20200424:15:49:58:050055 gpstop:zlv:gpadmin-[INFO]:-Commencing parallel primary segment instance shutdown, please wait... -20200424:15:49:58:050055 gpstop:zlv:gpadmin-[INFO]:-0.00% of jobs completed -20200424:15:49:59:050055 gpstop:zlv:gpadmin-[INFO]:-100.00% of jobs completed -20200424:15:49:59:050055 gpstop:zlv:gpadmin-[INFO]:-Commencing parallel mirror segment instance shutdown, please wait... -20200424:15:49:59:050055 gpstop:zlv:gpadmin-[INFO]:-0.00% of jobs completed -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:-100.00% of jobs completed -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:----------------------------------------------------- -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:- Segments stopped successfully = 6 -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:- Segments with errors during stop = 0 -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:----------------------------------------------------- -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:-Successfully shutdown 6 of 6 segment instances -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:-Database successfully shutdown with no errors reported -20200424:15:50:00:050055 gpstop:zlv:gpadmin-[INFO]:-Cleaning up leftover shared memory -20200424:15:50:01:050055 gpstop:zlv:gpadmin-[INFO]:-Restarting System... +-- table to just store the master's data directory path on segment. +CREATE TABLE datadir(a int, dir text); +CREATE +INSERT INTO datadir select 1,datadir from gp_segment_configuration where role='p' and content=-1; +INSERT 1 --- end_ignore +ALTER SYSTEM SET gp_enable_global_deadlock_detector TO on; +ALTER +ALTER SYSTEM SET gp_global_deadlock_detector_period TO 5; +ALTER +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from datadir; + pg_ctl +-------- + OK +(1 row) -- Start new session on master to make sure it has fully completed -- recovery and up and running again. 1: SHOW gp_enable_global_deadlock_detector; diff --git a/src/test/isolation2/expected/lockmodes.out b/src/test/isolation2/expected/lockmodes.out index 02b11030b0..f65a805ab8 100644 --- a/src/test/isolation2/expected/lockmodes.out +++ b/src/test/isolation2/expected/lockmodes.out @@ -1,3 +1,12 @@ +include: helpers/server_helpers.sql; +CREATE + +-- table to just store the master's data directory path on segment. +CREATE TABLE lockmodes_datadir(a int, dir text); +CREATE +INSERT INTO lockmodes_datadir select 1,datadir from gp_segment_configuration where role='p' and content=-1; +INSERT 1 + 1: set optimizer = off; SET @@ -1008,12 +1017,23 @@ ABORT 1q: ... 2q: ... --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v on; - -! gpstop -rai; +-- enable gdd +ALTER SYSTEM SET gp_enable_global_deadlock_detector TO on; +ALTER +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from lockmodes_datadir; + pg_ctl +-------- + OK +(1 row) --- end_ignore +1: show gp_enable_global_deadlock_detector; + gp_enable_global_deadlock_detector +------------------------------------ + on +(1 row) 1: set optimizer = off; SET @@ -1994,16 +2014,20 @@ ABORT 1q: ... 2q: ... --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v off; - -! gpstop -rai; - --- end_ignore +2: ALTER SYSTEM RESET gp_enable_global_deadlock_detector; +ALTER +1U:SELECT pg_ctl(dir, 'restart') from lockmodes_datadir; + pg_ctl +-------- + OK +(1 row) 1: show gp_enable_global_deadlock_detector; gp_enable_global_deadlock_detector ------------------------------------ off (1 row) + +1: drop table lockmodes_datadir; +DROP 1q: ... diff --git a/src/test/isolation2/sql/gdd/end.sql b/src/test/isolation2/sql/gdd/end.sql index d26e10c1ea..e75700a3b1 100644 --- a/src/test/isolation2/sql/gdd/end.sql +++ b/src/test/isolation2/sql/gdd/end.sql @@ -1,8 +1,13 @@ --- disable GDD +include: helpers/server_helpers.sql; --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v off; -! gpstop -rai; --- end_ignore +ALTER SYSTEM RESET gp_enable_global_deadlock_detector; +ALTER SYSTEM RESET gp_global_deadlock_detector_period; +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from datadir; +-- Start new session on master to make sure it has fully completed +-- recovery and up and running again. 1: SHOW gp_enable_global_deadlock_detector; +1: SHOW gp_global_deadlock_detector_period; diff --git a/src/test/isolation2/sql/gdd/prepare.sql b/src/test/isolation2/sql/gdd/prepare.sql index 49f3bed15b..474a3224e5 100644 --- a/src/test/isolation2/sql/gdd/prepare.sql +++ b/src/test/isolation2/sql/gdd/prepare.sql @@ -1,3 +1,5 @@ +include: helpers/server_helpers.sql; + -- t0r is the reference table to provide the data distribution info. DROP TABLE IF EXISTS t0p; CREATE TABLE t0p (id int, val int); @@ -48,12 +50,18 @@ SELECT segid(1,10) is not null; SELECT segid(2,10) is not null; --enable GDD --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v on; -! gpconfig -c gp_global_deadlock_detector_period -v 5; -! gpstop -rai; --- end_ignore +-- table to just store the master's data directory path on segment. +CREATE TABLE datadir(a int, dir text); +INSERT INTO datadir select 1,datadir from gp_segment_configuration where role='p' and content=-1; + +ALTER SYSTEM SET gp_enable_global_deadlock_detector TO on; +ALTER SYSTEM SET gp_global_deadlock_detector_period TO 5; + +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from datadir; -- Start new session on master to make sure it has fully completed -- recovery and up and running again. 1: SHOW gp_enable_global_deadlock_detector; diff --git a/src/test/isolation2/sql/lockmodes.sql b/src/test/isolation2/sql/lockmodes.sql index 2e8e2b5b7e..7971bbb592 100644 --- a/src/test/isolation2/sql/lockmodes.sql +++ b/src/test/isolation2/sql/lockmodes.sql @@ -1,3 +1,9 @@ +include: helpers/server_helpers.sql; + +-- table to just store the master's data directory path on segment. +CREATE TABLE lockmodes_datadir(a int, dir text); +INSERT INTO lockmodes_datadir select 1,datadir from gp_segment_configuration where role='p' and content=-1; + 1: set optimizer = off; create or replace view show_locks_lockmodes as @@ -306,10 +312,14 @@ create table t_lockmods_ao1 (c int) with (appendonly=true) distributed randomly; 1q: 2q: --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v on; -! gpstop -rai; --- end_ignore +-- enable gdd +ALTER SYSTEM SET gp_enable_global_deadlock_detector TO on; +-- Use utility session on seg 0 to restart master. This way avoids the +-- situation where session issuing the restart doesn't disappear +-- itself. +1U:SELECT pg_ctl(dir, 'restart') from lockmodes_datadir; + +1: show gp_enable_global_deadlock_detector; 1: set optimizer = off; @@ -594,10 +604,10 @@ create table t_lockmods_ao1 (c int) with (appendonly=true) distributed randomly; 1q: 2q: --- start_ignore -! gpconfig -c gp_enable_global_deadlock_detector -v off; -! gpstop -rai; --- end_ignore +2: ALTER SYSTEM RESET gp_enable_global_deadlock_detector; +1U:SELECT pg_ctl(dir, 'restart') from lockmodes_datadir; 1: show gp_enable_global_deadlock_detector; + +1: drop table lockmodes_datadir; 1q: -- GitLab