From 246a0b7ab80b74f1d79bef28801efcd39f3d7164 Mon Sep 17 00:00:00 2001
From: Karthikeyan Chinnakonda <karthikeyan@hasura.io>
Date: Thu, 23 Apr 2020 02:33:23 +0530
Subject: [PATCH] server: Improve `queryModifiesSchemaCache` check for run_sql
 (#4283)

The previous check was too conservative and acquired a lock on the
schema cache in situations where it was unnecessary. This change
exposes the logic run_sql uses to determine whether to use the
metadata check to make the check more precise.
---
 server/src-lib/Hasura/RQL/DDL/Schema.hs | 41 ++++++++++++++-----------
 server/src-lib/Hasura/Server/Query.hs   |  6 +---
 2 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/server/src-lib/Hasura/RQL/DDL/Schema.hs b/server/src-lib/Hasura/RQL/DDL/Schema.hs
index 886dbcde882..0ddd5ea8135 100644
--- a/server/src-lib/Hasura/RQL/DDL/Schema.hs
+++ b/server/src-lib/Hasura/RQL/DDL/Schema.hs
@@ -32,6 +32,7 @@ module Hasura.RQL.DDL.Schema
 
  , RunSQL(..)
  , runRunSQL
+ , isSchemaCacheBuildRequiredRunSQL
  ) where
 
 import           Hasura.Prelude
@@ -86,15 +87,30 @@ instance ToJSON RunSQL where
           Q.ReadWrite -> False
       ]
 
+-- | see Note [Checking metadata consistency in run_sql]
+isSchemaCacheBuildRequiredRunSQL :: RunSQL -> Bool
+isSchemaCacheBuildRequiredRunSQL RunSQL {..} =
+  case rTxAccessMode of
+    Q.ReadOnly  -> False
+    Q.ReadWrite -> fromMaybe (containsDDLKeyword rSql) rCheckMetadataConsistency
+  where
+    containsDDLKeyword :: Text -> Bool
+    containsDDLKeyword = TDFA.match $$(quoteRegex
+      TDFA.defaultCompOpt
+        { TDFA.caseSensitive = False
+        , TDFA.multiline = True
+        , TDFA.lastStarGreedy = True }
+        TDFA.defaultExecOpt
+        { TDFA.captureGroups = False }
+        "\\balter\\b|\\bdrop\\b|\\breplace\\b|\\bcreate function\\b|\\bcomment on\\b")
+
 runRunSQL :: (MonadTx m, CacheRWM m, HasSQLGenCtx m) => RunSQL -> m EncJSON
-runRunSQL RunSQL {..} = do
+runRunSQL q@RunSQL {..}
   -- see Note [Checking metadata consistency in run_sql]
-  let metadataCheckNeeded = case rTxAccessMode of
-        Q.ReadOnly  -> False
-        Q.ReadWrite -> fromMaybe (containsDDLKeyword rSql) rCheckMetadataConsistency
-  if metadataCheckNeeded
-    then withMetadataCheck rCascade $ execRawSQL rSql
-    else execRawSQL rSql
+  | isSchemaCacheBuildRequiredRunSQL q
+  = withMetadataCheck rCascade $ execRawSQL rSql
+  | otherwise
+  = execRawSQL rSql
   where
     execRawSQL :: (MonadTx m) => Text -> m EncJSON
     execRawSQL =
@@ -103,17 +119,6 @@ runRunSQL RunSQL {..} = do
         rawSqlErrHandler txe =
           (err400 PostgresError "query execution failed") { qeInternal = Just $ toJSON txe }
 
-    -- see Note [Checking metadata consistency in run_sql]
-    containsDDLKeyword :: Text -> Bool
-    containsDDLKeyword = TDFA.match $$(quoteRegex
-      TDFA.defaultCompOpt
-        { TDFA.caseSensitive = False
-        , TDFA.multiline = True
-        , TDFA.lastStarGreedy = True }
-      TDFA.defaultExecOpt
-        { TDFA.captureGroups = False }
-      "\\balter\\b|\\bdrop\\b|\\breplace\\b|\\bcreate function\\b|\\bcomment on\\b")
-
 {- Note [Checking metadata consistency in run_sql]
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 SQL queries executed by run_sql may change the Postgres schema in arbitrary
diff --git a/server/src-lib/Hasura/Server/Query.hs b/server/src-lib/Hasura/Server/Query.hs
index 78aba43bc5d..77d6c0dd56c 100644
--- a/server/src-lib/Hasura/Server/Query.hs
+++ b/server/src-lib/Hasura/Server/Query.hs
@@ -1,5 +1,4 @@
 {-# LANGUAGE NamedFieldPuns #-}
-
 module Hasura.Server.Query where
 
 import           Control.Lens
@@ -259,10 +258,7 @@ queryModifiesSchemaCache (RQV1 qi) = case qi of
   RQAddCollectionToAllowlist _    -> True
   RQDropCollectionFromAllowlist _ -> True
 
-  RQRunSql RunSQL{rTxAccessMode, rCheckMetadataConsistency}  ->
-    case rTxAccessMode of
-      Q.ReadOnly  -> False
-      Q.ReadWrite -> fromMaybe True rCheckMetadataConsistency
+  RQRunSql q                      -> isSchemaCacheBuildRequiredRunSQL q
 
   RQReplaceMetadata _             -> True
   RQExportMetadata _              -> False