From a7a900cd9481ad4ff5dc898480b27d5fc30ee735 Mon Sep 17 00:00:00 2001 From: Viktor Kurilko Date: Thu, 21 Sep 2023 12:16:59 +0700 Subject: [PATCH] fixed the fact that ORCA did not check the availability of the index in the current transaction as the postgres optimizer does --- .../translate/CTranslatorRelcacheToDXL.cpp | 31 +++++++++++++++++-- src/backend/gpopt/utils/COptTasks.cpp | 7 ++++- .../libgpopt/include/gpopt/mdcache/CMDCache.h | 28 +++++++++++++++++ .../gporca/libgpopt/src/mdcache/CMDCache.cpp | 9 ++++++ src/backend/optimizer/plan/orca.c | 1 - .../translate/CTranslatorRelcacheToDXL.h | 3 ++ .../isolation/expected/create_index_hot.out | 3 +- .../isolation/specs/create_index_hot.spec | 3 ++ 8 files changed, 80 insertions(+), 5 deletions(-) diff --git a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp index 69bb4b2cbc6b..60a70df6dc51 100644 --- a/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp +++ b/src/backend/gpopt/translate/CTranslatorRelcacheToDXL.cpp @@ -30,6 +30,7 @@ extern "C" { #include "utils/lsyscache.h" #include "utils/rel.h" #include "utils/relcache.h" +#include "utils/snapmgr.h" #include "utils/syscache.h" #include "utils/typcache.h" } @@ -41,6 +42,7 @@ extern "C" { #include "gpopt/base/CUtils.h" #include "gpopt/gpdbwrappers.h" #include "gpopt/mdcache/CMDAccessor.h" +#include "gpopt/mdcache/CMDCache.h" #include "gpopt/translate/CTranslatorRelcacheToDXL.h" #include "gpopt/translate/CTranslatorScalarToDXL.h" #include "gpopt/translate/CTranslatorUtils.h" @@ -308,7 +310,7 @@ CTranslatorRelcacheToDXL::RetrieveRelIndexInfoForPartTable(CMemoryPool *mp, GPOS_TRY { - if (IsIndexSupported(index_rel)) + if (IsIndexSupported(index_rel) && IsIndexVisible(index_rel)) { CMDIdGPDB *mdid_index = GPOS_NEW(mp) CMDIdGPDB(IMDId::EmdidInd, index_oid); @@ -318,6 +320,10 @@ CTranslatorRelcacheToDXL::RetrieveRelIndexInfoForPartTable(CMemoryPool *mp, GPOS_NEW(mp) CMDIndexInfo(mdid_index, is_partial); md_index_info_array->Append(md_index_info); } + else if (!IsIndexVisible(index_rel)) + { + CMDCache::SetTransient(TransactionXmin); + } gpdb::CloseRelation(index_rel); } @@ -364,7 +370,7 @@ CTranslatorRelcacheToDXL::RetrieveRelIndexInfoForNonPartTable(CMemoryPool *mp, GPOS_TRY { - if (IsIndexSupported(index_rel)) + if (IsIndexSupported(index_rel) && IsIndexVisible(index_rel)) { CMDIdGPDB *mdid_index = GPOS_NEW(mp) CMDIdGPDB(IMDId::EmdidInd, index_oid); @@ -373,6 +379,10 @@ CTranslatorRelcacheToDXL::RetrieveRelIndexInfoForNonPartTable(CMemoryPool *mp, CMDIndexInfo(mdid_index, false /* is_partial */); md_index_info_array->Append(md_index_info); } + else if (!IsIndexVisible(index_rel)) + { + CMDCache::SetTransient(TransactionXmin); + } gpdb::CloseRelation(index_rel); } @@ -3329,6 +3339,23 @@ CTranslatorRelcacheToDXL::IsIndexSupported(Relation index_rel) GIN_AM_OID == index_rel->rd_rel->relam); } +//--------------------------------------------------------------------------- +// @function: +// CTranslatorRelcacheToDXL::IsIndexVisible +// +// @doc: +// Check if index is visible in current transaction +// +//--------------------------------------------------------------------------- +BOOL +CTranslatorRelcacheToDXL::IsIndexVisible(Relation index_rel) +{ + return !(index_rel->rd_index->indcheckxmin && + !TransactionIdPrecedes( + HeapTupleHeaderGetXmin(index_rel->rd_indextuple->t_data), + TransactionXmin)); +} + //--------------------------------------------------------------------------- // @function: // CTranslatorRelcacheToDXL::RetrievePartConstraintForIndex diff --git a/src/backend/gpopt/utils/COptTasks.cpp b/src/backend/gpopt/utils/COptTasks.cpp index f435ece2c273..97228efe9556 100644 --- a/src/backend/gpopt/utils/COptTasks.cpp +++ b/src/backend/gpopt/utils/COptTasks.cpp @@ -35,6 +35,7 @@ #include "cdb/cdbvars.h" #include "utils/fmgroids.h" #include "utils/guc.h" +#include "utils/snapmgr.h" #undef setstate #include "gpos/_api.h" @@ -498,7 +499,10 @@ COptTasks::OptimizeTask(void *ptr) CMDCache::Init(); CMDCache::SetCacheQuota(optimizer_mdcache_size * 1024L); } - else if (reset_mdcache) + else if (reset_mdcache || + (CMDCache::IsTransient() && + !TransactionIdEquals(TransactionXmin, + CMDCache::GetTransientXmin()))) { CMDCache::Reset(); CMDCache::SetCacheQuota(optimizer_mdcache_size * 1024L); @@ -605,6 +609,7 @@ COptTasks::OptimizeTask(void *ptr) (PlannedStmt *) gpdb::CopyObject(ConvertToPlanStmtFromDXL( mp, &mda, plan_dxl, opt_ctxt->m_query->canSetTag, query_to_dxl_translator->GetDistributionHashOpsKind())); + opt_ctxt->m_plan_stmt->transientPlan = CMDCache::IsTransient(); } CStatisticsConfig *stats_conf = optimizer_config->GetStatsConf(); diff --git a/src/backend/gporca/libgpopt/include/gpopt/mdcache/CMDCache.h b/src/backend/gporca/libgpopt/include/gpopt/mdcache/CMDCache.h index 395c5dde4d81..33e32fdcdac4 100644 --- a/src/backend/gporca/libgpopt/include/gpopt/mdcache/CMDCache.h +++ b/src/backend/gporca/libgpopt/include/gpopt/mdcache/CMDCache.h @@ -44,6 +44,12 @@ class CMDCache // the maximum size of the cache static ULLONG m_ullCacheQuota; + // is cached data transient + static BOOL m_transient; + + // in which transaction did it become transient + static uint32_t m_transientXmin; + // private ctor CMDCache(){}; @@ -86,6 +92,28 @@ class CMDCache return m_pcache; } + // mark cache as transient + static void + SetTransient(uint32_t transientXmin) + { + m_transient = true; + m_transientXmin = transientXmin; + } + + // get transaction id in which did it become transient + static uint32_t + GetTransientXmin() + { + return m_transientXmin; + } + + // get is cached data transient + static BOOL + IsTransient() + { + return m_transient; + } + }; // class CMDCache } // namespace gpopt diff --git a/src/backend/gporca/libgpopt/src/mdcache/CMDCache.cpp b/src/backend/gporca/libgpopt/src/mdcache/CMDCache.cpp index ac4d4f4c63e7..fa98ecf0a4bc 100644 --- a/src/backend/gporca/libgpopt/src/mdcache/CMDCache.cpp +++ b/src/backend/gporca/libgpopt/src/mdcache/CMDCache.cpp @@ -24,6 +24,12 @@ CMDAccessor::MDCache *CMDCache::m_pcache = NULL; // maximum size of the cache ULLONG CMDCache::m_ullCacheQuota = UNLIMITED_CACHE_QUOTA; +// is cached data is transient +BOOL CMDCache::m_transient = false; + +// in which transaction did it become transient +uint32_t CMDCache::m_transientXmin = 0; + //--------------------------------------------------------------------------- // @function: // CMDCache::Init @@ -127,6 +133,9 @@ CMDCache::Reset() Shutdown(); Init(); + + m_transient = false; + m_transientXmin = 0; } // EOF diff --git a/src/backend/optimizer/plan/orca.c b/src/backend/optimizer/plan/orca.c index 057f192bc667..49cdcfe76805 100644 --- a/src/backend/optimizer/plan/orca.c +++ b/src/backend/optimizer/plan/orca.c @@ -241,7 +241,6 @@ optimize_query(Query *parse, ParamListInfo boundParams) result->relationOids = glob->relationOids; result->invalItems = glob->invalItems; result->oneoffPlan = glob->oneoffPlan; - result->transientPlan = glob->transientPlan; return result; } diff --git a/src/include/gpopt/translate/CTranslatorRelcacheToDXL.h b/src/include/gpopt/translate/CTranslatorRelcacheToDXL.h index b10ef4c91d61..17d2e8f5b5e1 100644 --- a/src/include/gpopt/translate/CTranslatorRelcacheToDXL.h +++ b/src/include/gpopt/translate/CTranslatorRelcacheToDXL.h @@ -262,6 +262,9 @@ class CTranslatorRelcacheToDXL // check if index is supported static BOOL IsIndexSupported(Relation index_rel); + // check if index is visible + static BOOL IsIndexVisible(Relation index_rel); + // retrieve index info list of partitioned table static List *RetrievePartTableIndexInfo(Relation rel); diff --git a/src/test/isolation/expected/create_index_hot.out b/src/test/isolation/expected/create_index_hot.out index 519318e1d9f0..fb37c9a989d7 100644 --- a/src/test/isolation/expected/create_index_hot.out +++ b/src/test/isolation/expected/create_index_hot.out @@ -1,6 +1,6 @@ Parsed test spec with 2 sessions -starting permutation: s2begin s2select s1update s1createindexonc s2select s2forceindexscan s2select +starting permutation: s2begin s2select s1optimizeroff s1update s1createindexonc s2select s2forceindexscan s2select step s2begin: BEGIN ISOLATION LEVEL SERIALIZABLE; step s2select: select '#' as expected, c from hot where c = '#' union all @@ -8,6 +8,7 @@ step s2select: select '#' as expected, c from hot where c = '#' expected c # # +step s1optimizeroff: set optimizer = off; step s1update: update hot set c = '$' where c = '#'; step s1createindexonc: create index idx_c on hot (c); step s2select: select '#' as expected, c from hot where c = '#' diff --git a/src/test/isolation/specs/create_index_hot.spec b/src/test/isolation/specs/create_index_hot.spec index bb80d8e3cdec..423d9546f26d 100644 --- a/src/test/isolation/specs/create_index_hot.spec +++ b/src/test/isolation/specs/create_index_hot.spec @@ -23,7 +23,9 @@ teardown # Update a row, and create an index on the updated column. This produces # a broken HOT chain. +#FIXME do not turn off the optimizer when ORCA stops always using Split Update. session "s1" +step "s1optimizeroff" {set optimizer = off;} step "s1update" { update hot set c = '$' where c = '#'; } step "s1createindexonc" { create index idx_c on hot (c); } @@ -39,6 +41,7 @@ permutation "s2begin" "s2select" + "s1optimizeroff" "s1update" "s1createindexonc"