From 65b8151af3009c00b1bf8cb76ac0482c0557a9dd Mon Sep 17 00:00:00 2001 From: Sven Klemm Date: Thu, 30 Jan 2025 20:41:40 +0100 Subject: [PATCH] Add actionable hints to update script checker --- .github/workflows/catalog-updates-check.yaml | 6 +- ...{check_updates_ast.py => check_updates.py} | 76 +++++++++---------- 2 files changed, 41 insertions(+), 41 deletions(-) rename scripts/{check_updates_ast.py => check_updates.py} (74%) diff --git a/.github/workflows/catalog-updates-check.yaml b/.github/workflows/catalog-updates-check.yaml index e1f234093da..360100d8de9 100644 --- a/.github/workflows/catalog-updates-check.yaml +++ b/.github/workflows/catalog-updates-check.yaml @@ -16,11 +16,11 @@ jobs: - name: Check sql file contents run: | - find sql -name '*.sql' -not -path 'sql/updates/*' -not -path 'sql/compat.sql' | xargs -IFILE python scripts/check_updates_ast.py FILE + find sql -name '*.sql' -not -path 'sql/updates/*' -not -path 'sql/compat.sql' | xargs -IFILE python scripts/check_updates.py FILE - name: Check latest-dev contents run: | - python scripts/check_updates_ast.py --latest "sql/updates/latest-dev.sql" + python scripts/check_updates.py --latest "sql/updates/latest-dev.sql" - name: Check for idempotency in SQL scripts if: always() @@ -32,5 +32,5 @@ jobs: - name: Check reverse-dev contents if: always() run: | - python scripts/check_updates_ast.py "sql/updates/reverse-dev.sql" || true + python scripts/check_updates.py "sql/updates/reverse-dev.sql" || true diff --git a/scripts/check_updates_ast.py b/scripts/check_updates.py similarity index 74% rename from scripts/check_updates_ast.py rename to scripts/check_updates.py index 3c505de68aa..cb9d1cecfbe 100644 --- a/scripts/check_updates_ast.py +++ b/scripts/check_updates.py @@ -28,6 +28,13 @@ def __init__(self): ] super().__init__() + def error(self, msg, hint=None): + self.errors += 1 + print(msg) + if hint: + print(hint) + print() + # ALTER TABLE _timescaledb_catalog. ADD/DROP COLUMN def visit_AlterTableStmt(self, ancestors, node): # pylint: disable=unused-argument if ( @@ -41,17 +48,17 @@ def visit_AlterTableStmt(self, ancestors, node): # pylint: disable=unused-argum enums.AlterTableType.AT_AddColumn, enums.AlterTableType.AT_DropColumn, ): - self.errors += 1 if cmd.subtype == enums.AlterTableType.AT_AddColumn: + subcmd = "ADD" column = cmd.def_.colname - print( - f"ERROR: Attempting to ADD COLUMN {column} to catalog table {schema}.{table}" - ) else: + subcmd = "DROP" column = cmd.name - print( - f"ERROR: Attempting to DROP COLUMN {column} from catalog table {schema}.{table}" - ) + + self.error( + f"Attempting to {subcmd} COLUMN {column} to catalog table {schema}.{table}", + "Tables need to be rebuilt in update script to ensure consistent attribute numbers", + ) # ALTER TABLE _timescaledb_catalog. RENAME TO def visit_RenameStmt(self, ancestors, node): # pylint: disable=unused-argument @@ -59,28 +66,27 @@ def visit_RenameStmt(self, ancestors, node): # pylint: disable=unused-argument node.renameType == enums.ObjectType.OBJECT_TABLE and node.relation.schemaname in self.catalog_schemata ): - self.errors += 1 - print( - f"ERROR: Attempting to RENAME catalog table {node.relation.schemaname}.{node.relation.relname}" + self.error( + f"Attempting to RENAME catalog table {node.relation.schemaname}.{node.relation.relname}", + "Catalog tables should be rebuilt in update scripts to ensure consistent naming for dependent objects", ) # CREATE TEMP | TEMPORARY TABLE .. # CREATE TABLE IF NOT EXISTS .. def visit_CreateStmt(self, ancestors, node): # pylint: disable=unused-argument if node.relation.relpersistence == "t": - self.errors += 1 schema = ( node.relation.schemaname + "." if node.relation.schemaname is not None else "" ) - print( - f"ERROR: Attempting to CREATE TEMPORARY TABLE {schema}{node.relation.relname}" + self.error( + f"Attempting to CREATE TEMPORARY TABLE {schema}{node.relation.relname}" + "Creating temporary tables is blocked in pg_extwlist context" ) if node.if_not_exists: - self.errors += 1 - print( - f"ERROR: Attempting to CREATE TABLE IF NOT EXISTS {node.relation.relname}" + self.error( + f"Attempting to CREATE TABLE IF NOT EXISTS {node.relation.relname}" ) # We have to be careful with the column types we use in our catalog to only allow types @@ -94,9 +100,8 @@ def visit_CreateStmt(self, ancestors, node): # pylint: disable=unused-argument "bool", "text", ]: - self.errors += 1 - print( - f"ERROR: Attempting to CREATE TABLE {node.relation.relname} with blocked array type {coldef.typeName.names[-1].sval}" + self.error( + f"Attempting to CREATE TABLE {node.relation.relname} with blocked array type {coldef.typeName.names[-1].sval}" ) else: if coldef.typeName.names[-1].sval not in [ @@ -114,9 +119,8 @@ def visit_CreateStmt(self, ancestors, node): # pylint: disable=unused-argument "text", "timestamptz", ]: - self.errors += 1 - print( - f"ERROR: Attempting to CREATE TABLE {node.relation.relname} with blocked type {coldef.typeName.names[-1].sval}" + self.error( + f"Attempting to CREATE TABLE {node.relation.relname} with blocked type {coldef.typeName.names[-1].sval}" ) # CREATE SCHEMA IF NOT EXISTS .. @@ -124,17 +128,15 @@ def visit_CreateSchemaStmt( self, ancestors, node ): # pylint: disable=unused-argument if node.if_not_exists: - self.errors += 1 - print(f"ERROR: Attempting to CREATE SCHEMA IF NOT EXISTS {node.schemaname}") + self.error(f"Attempting to CREATE SCHEMA IF NOT EXISTS {node.schemaname}") # CREATE MATERIALIZED VIEW IF NOT EXISTS .. def visit_CreateTableAsStmt( self, ancestors, node ): # pylint: disable=unused-argument if node.if_not_exists: - self.errors += 1 - print( - f"ERROR: Attempting to CREATE MATERIALIZED VIEW IF NOT EXISTS {node.into.rel.relname}" + self.error( + f"Attempting to CREATE MATERIALIZED VIEW IF NOT EXISTS {node.into.rel.relname}" ) # CREATE FOREIGN TABLE IF NOT EXISTS .. @@ -142,16 +144,14 @@ def visit_CreateForeignTableStmt( self, ancestors, node ): # pylint: disable=unused-argument if node.base.if_not_exists: - self.errors += 1 - print( - f"ERROR: Attempting to CREATE FOREIGN TABLE IF NOT EXISTS {node.base.relation.relname}" + self.error( + f"Attempting to CREATE FOREIGN TABLE IF NOT EXISTS {node.base.relation.relname}" ) # CREATE INDEX IF NOT EXISTS .. def visit_IndexStmt(self, ancestors, node): # pylint: disable=unused-argument if node.if_not_exists: - self.errors += 1 - print(f"ERROR: Attempting to CREATE INDEX IF NOT EXISTS {node.idxname}") + self.error(f"Attempting to CREATE INDEX IF NOT EXISTS {node.idxname}") # CREATE FUNCTION / PROCEDURE _timescaledb_internal... def visit_CreateFunctionStmt( @@ -170,17 +170,17 @@ def visit_CreateFunctionStmt( and node.returnType.names[0].sval not in ["table_am_handler", "index_am_handler"] ): - self.errors += 1 functype = "procedure" if node.is_procedure else "function" - print( - f"ERROR: Attempting to create {functype} {node.funcname[1].sval} with language 'c'" + self.error( + f"Attempting to create {functype} {node.funcname[-1].sval} with language 'c'", + "latest-dev should link C functions to ts_update_placeholder", ) if len(node.funcname) == 2 and node.funcname[0].sval == "_timescaledb_internal": - self.errors += 1 functype = "procedure" if node.is_procedure else "function" - print( - f"ERROR: Attempting to create {functype} {node.funcname[1].sval} in the internal schema" + self.error( + f"Attempting to create {functype} {node.funcname[1].sval} in the internal schema", + "_timescaledb_functions should be used as schema for internal functions", )