fix(replication): include source database in ROLLBACK PREPARED hint - #13
Conversation
pg_prepared_xacts is cluster-wide, but ROLLBACK PREPARED runs in the connection's database. Without naming the target database, the recommendation would silently fail when the prepared xact belongs to a different database than the one the monitoring tool is connected to. refactor(toast): simplify attcompression mapping; avoid raw control chars The attcompression default marker varies by psycopg driver / connection settings: ' ', '', '\x00', or None. The previous dict-with-fallback could echo back raw '\x00' or unknown values in the tool payload. Switch to explicit if/elif/else so 'p' -> pglz, 'l' -> lz4, and everything else (including all default-marker variants) -> 'default'. fix(linter): implicit-cast rule skips LIKE and is now INFO severity Without column-type information from the AST alone, this rule cannot distinguish a safe 'text-col = text-literal' (very common) from a suspect 'timestamp-col = text-literal'. Two mitigations: 1. Skip LIKE/ILIKE operators — they are text-only operators and were producing false positives that overlapped with NonSargableLikeRule. 2. Downgrade severity from WARNING to INFO so lint_workload's default warning threshold filters the rule out by default. Users who want the heuristic must opt in via severity_threshold='info'.
There was a problem hiding this comment.
Code Review
This pull request introduces static query linting using pglast AST, adds new MCP tools for buffer cache, TOAST storage, and replication health analysis, and updates existing tools to support PostgreSQL 17. It also hardens security with safe CORS defaults and connection URI scrubbing, and adds a comprehensive integration testing suite. The reviewer feedback highlights three improvement opportunities: refining the pg_buffercache join to match tablespace OIDs for multi-tablespace environments, refactoring linter rules to collect all findings instead of returning early on the first match, and escaping single quotes in prepared transaction identifiers to prevent SQL syntax errors in recommendations.
| -- pg_class(relfilenode). Safe here because the schema filter | ||
| -- below excludes pg_catalog/information_schema/pg_toast — | ||
| -- the only relations whose relfilenode is 0 (mapped catalogs). | ||
| JOIN pg_class c ON b.relfilenode = c.relfilenode |
There was a problem hiding this comment.
In PostgreSQL, relfilenode is only unique within a database and tablespace. If the database uses multiple tablespaces, joining solely on relfilenode can lead to incorrect relation mappings or duplicate rows for relations that share the same relfilenode value across different tablespaces. To ensure correctness, the join should also match the tablespace OID, taking into account that a reltablespace value of 0 in pg_class indicates the database's default tablespace.
| JOIN pg_class c ON b.relfilenode = c.relfilenode | |
| JOIN pg_class c ON b.relfilenode = c.relfilenode AND b.reltablespace = COALESCE(NULLIF(c.reltablespace, 0), (SELECT dattablespace FROM pg_database WHERE datname = current_database())) |
| def check(self, root: Any, sql: str) -> list[Finding]: | ||
| for n in _walk(root): | ||
| if type(n).__name__ != "ResTarget": | ||
| continue | ||
| val = getattr(n, "val", None) | ||
| if val is not None and type(val).__name__ == "ColumnRef": | ||
| fields = getattr(val, "fields", None) or () | ||
| if any(type(f).__name__ == "A_Star" for f in fields): | ||
| return [Finding( | ||
| self.rule_id, self.severity, | ||
| "SELECT * - list explicit columns to avoid over-fetching", | ||
| suggestion="Replace * with the columns actually needed", | ||
| )] | ||
| return [] |
There was a problem hiding this comment.
The check method returns immediately upon finding the first match. If a query contains multiple instances of the same anti-pattern (such as multiple SELECT * statements), only the first one will be reported. To ensure completeness, the rule should collect all findings in a list and return the list at the end of the method. This early-return pattern is repeated across several other rules in this file (such as ImplicitCastRule, OrOfEqualsRule, NonSargableLikeRule, NotInNullableRule, and FunctionOnIndexedColRule) and should be refactored similarly.
| def check(self, root: Any, sql: str) -> list[Finding]: | |
| for n in _walk(root): | |
| if type(n).__name__ != "ResTarget": | |
| continue | |
| val = getattr(n, "val", None) | |
| if val is not None and type(val).__name__ == "ColumnRef": | |
| fields = getattr(val, "fields", None) or () | |
| if any(type(f).__name__ == "A_Star" for f in fields): | |
| return [Finding( | |
| self.rule_id, self.severity, | |
| "SELECT * - list explicit columns to avoid over-fetching", | |
| suggestion="Replace * with the columns actually needed", | |
| )] | |
| return [] | |
| def check(self, root: Any, sql: str) -> list[Finding]: | |
| findings = [] | |
| for n in _walk(root): | |
| if type(n).__name__ != "ResTarget": | |
| continue | |
| val = getattr(n, "val", None) | |
| if val is not None and type(val).__name__ == "ColumnRef": | |
| fields = getattr(val, "fields", None) or () | |
| if any(type(f).__name__ == "A_Star" for f in fields): | |
| findings.append(Finding( | |
| self.rule_id, self.severity, | |
| "SELECT * - list explicit columns to avoid over-fetching", | |
| suggestion="Replace * with the columns actually needed", | |
| )) | |
| return findings |
| result["recommendations"].append( | ||
| f"Connect to database '{r['database']}' and run: " | ||
| f"ROLLBACK PREPARED '{r['gid']}'; if confirmed stale" | ||
| ) |
There was a problem hiding this comment.
If the global transaction identifier (gid) contains a single quote (which is valid in PostgreSQL), the recommended ROLLBACK PREPARED SQL command will be syntactically invalid and fail when executed. To prevent syntax errors, any single quotes within the gid should be escaped by doubling them.
| result["recommendations"].append( | |
| f"Connect to database '{r['database']}' and run: " | |
| f"ROLLBACK PREPARED '{r['gid']}'; if confirmed stale" | |
| ) | |
| escaped_gid = r['gid'].replace("'", "''") | |
| result["recommendations"].append( | |
| f"Connect to database '{r['database']}' and run: " | |
| f"ROLLBACK PREPARED '{escaped_gid}'; if confirmed stale" | |
| ) |
pg_prepared_xacts is cluster-wide, but ROLLBACK PREPARED runs in the connection's database. Without naming the target database, the recommendation would silently fail when the prepared xact belongs to a different database than the one the monitoring tool is connected to.
refactor(toast): simplify attcompression mapping; avoid raw control chars
The attcompression default marker varies by psycopg driver / connection settings: ' ', '', '\x00', or None. The previous dict-with-fallback could echo back raw '\x00' or unknown values in the tool payload. Switch to explicit if/elif/else so 'p' -> pglz, 'l' -> lz4, and everything else (including all default-marker variants) -> 'default'.
fix(linter): implicit-cast rule skips LIKE and is now INFO severity
Without column-type information from the AST alone, this rule cannot distinguish a safe 'text-col = text-literal' (very common) from a suspect 'timestamp-col = text-literal'. Two mitigations: