From d1ba4da5ae27ffb7af80a464c0e3d62a5c540bca Mon Sep 17 00:00:00 2001 From: Hubert Zhang Date: Wed, 15 Jul 2020 13:45:55 +0800 Subject: [PATCH] Cleanup idle reader gang after utility statements Reader gangs use local snapshot to access catalog, as a result, it will not synchronize with the sharedSnapshot from write gang which will lead to inconsistent visibility of catalog table on idle reader gang. Considering the case: select * from t, t t1; -- create a reader gang. begin; create role r1; set role r1; -- set command will also dispatched to idle reader gang When set role command dispatched to idle reader gang, reader gang cannot see the new tuple t1 in catalog table pg_auth. To fix this issue, we should drop the idle reader gangs after each utility statement which may modify the catalog table. Reviewed-by: Zhenghua Lyu --- src/backend/cdb/dispatcher/cdbdisp.c | 7 +++ src/backend/cdb/dispatcher/cdbdisp_query.c | 17 +++++ src/include/cdb/cdbdisp.h | 1 + src/test/regress/expected/guc_gp.out | 72 ++++++++++++++++++++++ src/test/regress/sql/guc_gp.sql | 42 +++++++++++++ 5 files changed, 139 insertions(+) diff --git a/src/backend/cdb/dispatcher/cdbdisp.c b/src/backend/cdb/dispatcher/cdbdisp.c index 3553552c8c..9fd9ad4535 100644 --- a/src/backend/cdb/dispatcher/cdbdisp.c +++ b/src/backend/cdb/dispatcher/cdbdisp.c @@ -301,6 +301,7 @@ cdbdisp_makeDispatcherState(bool isExtendedQuery) #endif handle->dispatcherState->allocatedGangs = NIL; handle->dispatcherState->largestGangSize = 0; + handle->dispatcherState->destroyIdleReaderGang = false; return handle->dispatcherState; } @@ -377,6 +378,12 @@ cdbdisp_destroyDispatcherState(CdbDispatcherState *ds) RecycleGang(gp, ds->forceDestroyGang); } + /* + * Destroy all the idle reader gangs when flag destroyIdleReaderGang is true + */ + if (ds->destroyIdleReaderGang) + cdbcomponent_cleanupIdleQEs(false); + ds->allocatedGangs = NIL; ds->dispatchParams = NULL; ds->primaryResults = NULL; diff --git a/src/backend/cdb/dispatcher/cdbdisp_query.c b/src/backend/cdb/dispatcher/cdbdisp_query.c index d0fc1f7ad5..81f9802ab7 100644 --- a/src/backend/cdb/dispatcher/cdbdisp_query.c +++ b/src/backend/cdb/dispatcher/cdbdisp_query.c @@ -464,6 +464,23 @@ cdbdisp_dispatchCommandInternal(DispatchCommandQueryParms *pQueryParms, */ ds = cdbdisp_makeDispatcherState(false); + /* + * Reader gangs use local snapshot to access catalog, as a result, it will + * not synchronize with the global snapshot from write gang which will lead + * to inconsistent visibilty of catalog table. Considering the case: + * + * select * from t, t t1; -- create a reader gang. + * begin; + * create role r1; + * set role r1; -- set command will also dispatched to idle reader gang + * + * When set role command dispatched to reader gang, reader gang cannot see + * the new tuple t1 in catalog table pg_auth. + * To fix this issue, we should drop the idle reader gangs after each + * utility statement which may modify the catalog table. + */ + ds->destroyIdleReaderGang = true; + queryText = buildGpQueryString(pQueryParms, &queryTextLength); /* diff --git a/src/include/cdb/cdbdisp.h b/src/include/cdb/cdbdisp.h index 65411e8fb6..d44a6cdd4c 100644 --- a/src/include/cdb/cdbdisp.h +++ b/src/include/cdb/cdbdisp.h @@ -48,6 +48,7 @@ typedef struct CdbDispatcherState #ifdef USE_ASSERT_CHECKING bool isGangDestroying; #endif + bool destroyIdleReaderGang; } CdbDispatcherState; typedef struct DispatcherInternalFuncs diff --git a/src/test/regress/expected/guc_gp.out b/src/test/regress/expected/guc_gp.out index 84cbd8e8d9..68ef862996 100644 --- a/src/test/regress/expected/guc_gp.out +++ b/src/test/regress/expected/guc_gp.out @@ -336,3 +336,75 @@ INFO: iteration:3 unsafe truncate performed (1 row) +CREATE TABLE guc_gp_t1(i int); +NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'i' as the Greenplum Database data distribution key for this table. +HINT: The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew. +INSERT INTO guc_gp_t1 VALUES(1),(2); +-- generate an idle redaer gang by the following query +SELECT count(*) FROM guc_gp_t1, guc_gp_t1 t; + count +------- + 4 +(1 row) + +-- test create role and set role in the same transaction +BEGIN; +DROP ROLE IF EXISTS guc_gp_test_role1; +NOTICE: role "guc_gp_test_role1" does not exist, skipping +CREATE ROLE guc_gp_test_role1; +NOTICE: resource queue required -- using default resource queue "pg_default" +SET ROLE guc_gp_test_role1; +RESET ROLE; +END; +-- generate an idle redaer gang by the following query +SELECT count(*) FROM guc_gp_t1, guc_gp_t1 t; + count +------- + 4 +(1 row) + +BEGIN ISOLATION LEVEL REPEATABLE READ; +DROP ROLE IF EXISTS guc_gp_test_role2; +NOTICE: role "guc_gp_test_role2" does not exist, skipping +CREATE ROLE guc_gp_test_role2; +NOTICE: resource queue required -- using default resource queue "pg_default" +SET ROLE guc_gp_test_role2; +RESET ROLE; +END; +-- test cursor case +-- cursors are also reader gangs, but they are not idle, thus will not be +-- destroyed by utility statement. +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM guc_gp_t1 a, guc_gp_t1 b order by a.i, b.i; +DECLARE c2 CURSOR FOR SELECT * FROM guc_gp_t1 a, guc_gp_t1 b order by a.i, b.i; +FETCH c1; + i | i +---+--- + 1 | 1 +(1 row) + +DROP ROLE IF EXISTS guc_gp_test_role1; +CREATE ROLE guc_gp_test_role1; +NOTICE: resource queue required -- using default resource queue "pg_default" +SET ROLE guc_gp_test_role1; +RESET ROLE; +FETCH c2; + i | i +---+--- + 1 | 1 +(1 row) + +FETCH c1; + i | i +---+--- + 1 | 2 +(1 row) + +FETCH c2; + i | i +---+--- + 1 | 2 +(1 row) + +END; +DROP TABLE guc_gp_t1; diff --git a/src/test/regress/sql/guc_gp.sql b/src/test/regress/sql/guc_gp.sql index 02f52f2003..c59979ab70 100644 --- a/src/test/regress/sql/guc_gp.sql +++ b/src/test/regress/sql/guc_gp.sql @@ -219,3 +219,45 @@ $$ $$ language plpythonu; select run_all_in_one(); + +CREATE TABLE guc_gp_t1(i int); +INSERT INTO guc_gp_t1 VALUES(1),(2); + +-- generate an idle redaer gang by the following query +SELECT count(*) FROM guc_gp_t1, guc_gp_t1 t; + +-- test create role and set role in the same transaction +BEGIN; +DROP ROLE IF EXISTS guc_gp_test_role1; +CREATE ROLE guc_gp_test_role1; +SET ROLE guc_gp_test_role1; +RESET ROLE; +END; + +-- generate an idle redaer gang by the following query +SELECT count(*) FROM guc_gp_t1, guc_gp_t1 t; + +BEGIN ISOLATION LEVEL REPEATABLE READ; +DROP ROLE IF EXISTS guc_gp_test_role2; +CREATE ROLE guc_gp_test_role2; +SET ROLE guc_gp_test_role2; +RESET ROLE; +END; + +-- test cursor case +-- cursors are also reader gangs, but they are not idle, thus will not be +-- destroyed by utility statement. +BEGIN; +DECLARE c1 CURSOR FOR SELECT * FROM guc_gp_t1 a, guc_gp_t1 b order by a.i, b.i; +DECLARE c2 CURSOR FOR SELECT * FROM guc_gp_t1 a, guc_gp_t1 b order by a.i, b.i; +FETCH c1; +DROP ROLE IF EXISTS guc_gp_test_role1; +CREATE ROLE guc_gp_test_role1; +SET ROLE guc_gp_test_role1; +RESET ROLE; +FETCH c2; +FETCH c1; +FETCH c2; +END; + +DROP TABLE guc_gp_t1; -- GitLab