From 1c055768982b71dd9d3400d51ee9058bd39142de Mon Sep 17 00:00:00 2001 From: Fantix King Date: Thu, 13 Feb 2025 16:39:38 -0500 Subject: [PATCH 1/3] Skip SQL cache in cache recompilation A complete SQL compilation requires a backend connection to amend the in/out tids. We recompiled wrong SQL cache with out_type_tid=000...000, so this commit temporarily discards SQL cache after DDL to avoid such issue. --- edb/server/dbview/dbview.pyx | 14 ++++++++++--- tests/test_server_ops.py | 40 +++++++++++++++++++++++++++++++----- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/edb/server/dbview/dbview.pyx b/edb/server/dbview/dbview.pyx index 35abd3f532b..3fc2c22ce64 100644 --- a/edb/server/dbview/dbview.pyx +++ b/edb/server/dbview/dbview.pyx @@ -1262,6 +1262,9 @@ cdef class DatabaseConnectionView: # Only recompile queries from the *latest* version, # to avoid quadratic slowdown problems. and req.schema_version == self.schema_version + # SQL queries require _amend_typedesc_in_sql() with a + # backend connection, which is not available here. + and req.input_language != enums.InputLanguage.SQL ): g.create_task(recompile_request(req)) @@ -1447,9 +1450,14 @@ cdef class DatabaseConnectionView: ) if use_metrics: - metrics.edgeql_query_compilations.inc( - 1.0, self.tenant.get_instance_name(), 'compiler' - ) + if query_req.input_language is enums.InputLanguage.EDGEQL: + metrics.edgeql_query_compilations.inc( + 1.0, self.tenant.get_instance_name(), 'compiler' + ) + else: + metrics.sql_compilations.inc( + 1.0, self.tenant.get_instance_name() + ) source = query_req.source if query_unit_group.force_non_normalized: diff --git a/tests/test_server_ops.py b/tests/test_server_ops.py index e6cf9a4d628..fb71c906881 100644 --- a/tests/test_server_ops.py +++ b/tests/test_server_ops.py @@ -745,12 +745,34 @@ def measure_sql_compilations( con = await sd.connect() try: qry = 'select schema::Object { name }' + sql = ''' + SELECT + n.nspname::text AS table_schema, + c.relname::text AS table_name, + CASE + WHEN c.relkind = 'r' THEN 'table' + WHEN c.relkind = 'v' THEN 'view' + WHEN c.relkind = 'm' THEN 'materialized_view' + END AS type, + c.relrowsecurity AS rls_enabled + FROM + pg_catalog.pg_class c + JOIN + pg_catalog.pg_namespace n + ON n.oid::text = c.relnamespace::text + WHERE + c.relkind IN ('r', 'v', 'm') + AND n.nspname = 'public'; + ''' await con.query(qry) + await con.query_sql(sql) # Querying a second time should hit the cache with self.assertChange(measure_compilations(sd), 0): await con.query(qry) + with self.assertChange(measure_sql_compilations(sd), 0): + await con.query_sql(sql) await con.query(''' create type X @@ -764,6 +786,14 @@ def measure_sql_compilations( with self.assertChange(measure_compilations(sd), 0): await con.query(qry) + # The SQL cache is reset after DDL, because recompiling SQL + # requires backend connection for amending in/out tids, and + # we don't have the infra for that yet. + with self.assertChange(measure_sql_compilations(sd), 1): + await con.query_sql(sql) + with self.assertChange(measure_sql_compilations(sd), 0): + await con.query_sql(sql) + # TODO: this does not behave the way I thing it should # with self.assertChange(measure_compilations(sd), 1): # con_c = con.with_config(apply_access_policies=False) @@ -802,24 +832,24 @@ def measure_sql_compilations( # Now, a similar thing for SQL queries - with self.assertChange(measure_compilations(sd), 1): + with self.assertChange(measure_sql_compilations(sd), 1): await con.query_sql('select 1') # cache hit - with self.assertChange(measure_compilations(sd), 0): + with self.assertChange(measure_sql_compilations(sd), 0): await con.query_sql('select 1') # changing globals: cache hit - with self.assertChange(measure_compilations(sd), 0): + with self.assertChange(measure_sql_compilations(sd), 0): con_g = con.with_globals({'g': 'hello'}) await con_g.query_sql('select 1') # normalization: pg_query_normalize is underwhelming - with self.assertChange(measure_compilations(sd), 1): + with self.assertChange(measure_sql_compilations(sd), 1): await con.query_sql('sElEct 1') # constant extraction: cache hit - with self.assertChange(measure_compilations(sd), 0): + with self.assertChange(measure_sql_compilations(sd), 0): await con.query_sql('select 2') # TODO: this does not behave the way I though it should From e3c8e82a4a0d0d276b772b2c5805e7fba826632e Mon Sep 17 00:00:00 2001 From: Fantix King Date: Thu, 13 Feb 2025 18:16:33 -0500 Subject: [PATCH 2/3] Complete the test --- tests/test_server_ops.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/test_server_ops.py b/tests/test_server_ops.py index fb71c906881..2410c312914 100644 --- a/tests/test_server_ops.py +++ b/tests/test_server_ops.py @@ -824,6 +824,12 @@ def measure_sql_compilations( with self.assertChange(measure_compilations(sd), 1): await con.query(qry) + with self.assertChange(measure_compilations(sd), 0): + await con.query(qry) + with self.assertChange(measure_sql_compilations(sd), 1): + await con.query_sql(sql) + with self.assertChange(measure_sql_compilations(sd), 0): + await con.query_sql(sql) await con.execute( "configure current database " @@ -912,6 +918,9 @@ def measure_sql_compilations( # It should hit the cache no problem. with self.assertChange(measure_compilations(sd), 0): await con.query(qry) + # TODO(fantix): persistent SQL cache not working? + # with self.assertChange(measure_sql_compilation(sd), 0): + # await con.query_sql(sql) finally: await con.aclose() From 6aea3df37fa6666d5684393f73450e453f01497b Mon Sep 17 00:00:00 2001 From: Fantix King Date: Thu, 13 Feb 2025 18:20:03 -0500 Subject: [PATCH 3/3] bug fix: auto_rebuild_query_cache gate was broken --- edb/server/config/__init__.py | 1 + edb/server/dbview/dbview.pyx | 2 +- tests/test_server_ops.py | 21 +++++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/edb/server/config/__init__.py b/edb/server/config/__init__.py index c3420ae0272..50c147e9885 100644 --- a/edb/server/config/__init__.py +++ b/edb/server/config/__init__.py @@ -77,6 +77,7 @@ def lookup( spec: Spec, allow_unrecognized: bool = False, ) -> Any: + assert len(configs) > 0 try: setting = spec[name] diff --git a/edb/server/dbview/dbview.pyx b/edb/server/dbview/dbview.pyx index 3fc2c22ce64..49574d6dced 100644 --- a/edb/server/dbview/dbview.pyx +++ b/edb/server/dbview/dbview.pyx @@ -1440,7 +1440,7 @@ cdef class DatabaseConnectionView: if unit.user_schema: user_schema = unit.user_schema user_schema_version = unit.user_schema_version - if user_schema and not self.server.config_lookup( + if user_schema and not self.config_lookup( "auto_rebuild_query_cache", ): user_schema = None diff --git a/tests/test_server_ops.py b/tests/test_server_ops.py index 2410c312914..779c833ba3a 100644 --- a/tests/test_server_ops.py +++ b/tests/test_server_ops.py @@ -864,6 +864,27 @@ def measure_sql_compilations( # con_c = con.with_config(apply_access_policies=False) # await con_c.query_sql(qry_sql) + # At last, make sure recompilation switch works fine + await con.execute( + "configure current database " + "set auto_rebuild_query_cache := false" + ) + await con.query(''' + create type X + ''') + with self.assertChange(measure_compilations(sd), 1): + await con.query(qry) + with self.assertChange(measure_compilations(sd), 0): + await con.query(qry) + with self.assertChange(measure_sql_compilations(sd), 1): + await con.query_sql(sql) + with self.assertChange(measure_sql_compilations(sd), 0): + await con.query_sql(sql) + await con.execute( + "configure current database " + "reset auto_rebuild_query_cache" + ) + finally: await con.aclose()