From 4f6c414f4c52b03157273a6da34ef2726983169a Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Wed, 2 Sep 2026 19:00:00 -0700 Subject: [PATCH 1/7] feat(queries): read a table's inheritance tree in one recursive query Add RelationTree struct and methods to traverse pg_inherits recursively. GetRelationTree fetches root and all descendants in depth-first order, with methods to filter heap leaves, foreign relations, and check inheritance. --- db/queries/relations.go | 153 +++++++++++++++++++++++++++++++++++ db/queries/relations_test.go | 104 ++++++++++++++++++++++++ db/queries/templates.go | 29 +++++++ 3 files changed, 286 insertions(+) create mode 100644 db/queries/relations.go create mode 100644 db/queries/relations_test.go diff --git a/db/queries/relations.go b/db/queries/relations.go new file mode 100644 index 0000000..b6aaa2d --- /dev/null +++ b/db/queries/relations.go @@ -0,0 +1,153 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package queries + +import ( + "context" + "fmt" +) + +// RelationInfo is one relation in an inheritance tree. +type RelationInfo struct { + Schema string + Name string + RelKind string // r = heap, p = partitioned, f = foreign + Depth int // 0 for the table the tree was built from + Parent string // qualified parent name, "" for the root +} + +// Qualified returns schema.name without quoting. +func (r RelationInfo) Qualified() string { + return r.Schema + "." + r.Name +} + +// RelationTree is a table together with every relation that inherits from it, +// directly or through intermediate parents. +type RelationTree struct { + Root RelationInfo + Descendants []RelationInfo // ordered by depth, then name +} + +// HasForeign reports whether the root or any descendant is a foreign table. +func (t *RelationTree) HasForeign() bool { + if t.Root.RelKind == "f" { + return true + } + for _, d := range t.Descendants { + if d.RelKind == "f" { + return true + } + } + return false +} + +// ForeignRelations lists the qualified names of every foreign relation in +// the tree, root first, then in tree order. +func (t *RelationTree) ForeignRelations() []string { + var out []string + if t.Root.RelKind == "f" { + out = append(out, t.Root.Qualified()) + } + for _, d := range t.Descendants { + if d.RelKind == "f" { + out = append(out, d.Qualified()) + } + } + return out +} + +// HeapLeaves lists the relations that actually store rows: the root if it is +// a heap table, then every heap descendant. Partitioned relations hold no +// rows and are skipped; foreign relations are skipped. +func (t *RelationTree) HeapLeaves() []RelationInfo { + var out []RelationInfo + if t.Root.RelKind == "r" { + out = append(out, t.Root) + } + for _, d := range t.Descendants { + if d.RelKind == "r" { + out = append(out, d) + } + } + return out +} + +// IsInherited reports whether anything inherits from the root. +func (t *RelationTree) IsInherited() bool { + return len(t.Descendants) > 0 +} + +// GetRelationTree runs the recursive pg_inherits query for schema.table. +// It returns nil, nil when the table does not exist. A relation reachable +// through more than one parent (multiple inheritance) is listed once. +func GetRelationTree(ctx context.Context, db DBQuerier, schema, table string) (*RelationTree, error) { + sql, err := RenderSQL(SQLTemplates.GetRelationTree, nil) + if err != nil { + return nil, fmt.Errorf("failed to render GetRelationTree SQL: %w", err) + } + // The query text is a fixed template; schema and table travel as bind + // parameters, so nothing from the caller is spliced into the SQL. + rows, err := db.Query(ctx, sql, schema, table) // nosemgrep + if err != nil { + return nil, fmt.Errorf("query to get relation tree for %s.%s failed: %w", schema, table, err) + } + defer rows.Close() + + var relations []RelationInfo + for rows.Next() { + var r RelationInfo + if err := rows.Scan(&r.Schema, &r.Name, &r.RelKind, &r.Depth, &r.Parent); err != nil { + return nil, fmt.Errorf("failed to scan relation tree row: %w", err) + } + relations = append(relations, r) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("error iterating relation tree rows: %w", err) + } + tree, err := buildRelationTree(relations) + if err != nil { + return nil, fmt.Errorf("relation tree for %s.%s: %w", schema, table, err) + } + return tree, nil +} + +// relationKey identifies a relation by its separate schema and name so that +// "a"."b.c" and "a.b"."c" never collide the way their dotted rendering does. +type relationKey struct { + schema string + name string +} + +// buildRelationTree folds the query's rows, ordered by depth with the root +// first, into a tree. A relation that appears more than once (reachable +// through several parents) is kept once. It returns nil, nil for no rows. +func buildRelationTree(relations []RelationInfo) (*RelationTree, error) { + var tree *RelationTree + seen := make(map[relationKey]bool) + for _, r := range relations { + key := relationKey{schema: r.Schema, name: r.Name} + if r.Depth == 0 { + tree = &RelationTree{Root: r} + seen[key] = true + continue + } + if tree == nil { + return nil, fmt.Errorf("has a descendant before its root") + } + if seen[key] { + continue + } + seen[key] = true + tree.Descendants = append(tree.Descendants, r) + } + return tree, nil +} diff --git a/db/queries/relations_test.go b/db/queries/relations_test.go new file mode 100644 index 0000000..baf66d3 --- /dev/null +++ b/db/queries/relations_test.go @@ -0,0 +1,104 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package queries + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func sampleTree() *RelationTree { + return &RelationTree{ + Root: RelationInfo{Schema: "s", Name: "parent", RelKind: "r", Depth: 0}, + Descendants: []RelationInfo{ + {Schema: "s", Name: "child_fdw", RelKind: "f", Depth: 1, Parent: "s.parent"}, + {Schema: "s", Name: "child_heap", RelKind: "r", Depth: 1, Parent: "s.parent"}, + {Schema: "s", Name: "grandchild_heap", RelKind: "r", Depth: 2, Parent: "s.child_heap"}, + }, + } +} + +func TestRelationTree_HasForeign(t *testing.T) { + assert.True(t, sampleTree().HasForeign()) + + plain := &RelationTree{Root: RelationInfo{Schema: "s", Name: "t", RelKind: "r"}} + assert.False(t, plain.HasForeign()) + + foreignRoot := &RelationTree{Root: RelationInfo{Schema: "s", Name: "t", RelKind: "f"}} + assert.True(t, foreignRoot.HasForeign()) +} + +func TestRelationTree_ForeignRelations(t *testing.T) { + assert.Equal(t, []string{"s.child_fdw"}, sampleTree().ForeignRelations()) +} + +func TestRelationTree_HeapLeaves_IncludesHeapRootAndAllHeapDescendants(t *testing.T) { + leaves := sampleTree().HeapLeaves() + names := make([]string, 0, len(leaves)) + for _, l := range leaves { + names = append(names, l.Qualified()) + } + assert.Equal(t, []string{"s.parent", "s.child_heap", "s.grandchild_heap"}, names) +} + +func TestRelationTree_HeapLeaves_SkipsPartitionedRoot(t *testing.T) { + tree := &RelationTree{ + Root: RelationInfo{Schema: "s", Name: "p", RelKind: "p"}, + Descendants: []RelationInfo{ + {Schema: "s", Name: "p_1", RelKind: "r", Depth: 1, Parent: "s.p"}, + {Schema: "s", Name: "p_2", RelKind: "f", Depth: 1, Parent: "s.p"}, + }, + } + leaves := tree.HeapLeaves() + assert.Len(t, leaves, 1) + assert.Equal(t, "s.p_1", leaves[0].Qualified()) +} + +func TestRelationTree_IsInherited(t *testing.T) { + assert.True(t, sampleTree().IsInherited()) + assert.False(t, (&RelationTree{Root: RelationInfo{RelKind: "r"}}).IsInherited()) +} +func TestBuildRelationTree_KeysBySchemaAndNameSeparately(t *testing.T) { + // "a"."b.c" and "a.b"."c" both render as a.b.c. They must stay two + // relations, so a foreign one among them is not dropped. + tree, err := buildRelationTree([]RelationInfo{ + {Schema: "a", Name: "p", RelKind: "r", Depth: 0}, + {Schema: "a", Name: "b.c", RelKind: "r", Depth: 1, Parent: "a.p"}, + {Schema: "a.b", Name: "c", RelKind: "f", Depth: 1, Parent: "a.p"}, + }) + require.NoError(t, err) + require.Len(t, tree.Descendants, 2) + assert.True(t, tree.HasForeign()) +} + +func TestBuildRelationTree_DeduplicatesMultipleInheritance(t *testing.T) { + tree, err := buildRelationTree([]RelationInfo{ + {Schema: "s", Name: "p", RelKind: "r", Depth: 0}, + {Schema: "s", Name: "child", RelKind: "r", Depth: 1, Parent: "s.p"}, + {Schema: "s", Name: "child", RelKind: "r", Depth: 1, Parent: "s.q"}, + }) + require.NoError(t, err) + assert.Len(t, tree.Descendants, 1) +} + +func TestBuildRelationTree_NoRowsMeansNoTable(t *testing.T) { + tree, err := buildRelationTree(nil) + require.NoError(t, err) + assert.Nil(t, tree) +} + +func TestBuildRelationTree_DescendantBeforeRootIsAnError(t *testing.T) { + _, err := buildRelationTree([]RelationInfo{{Schema: "s", Name: "c", RelKind: "r", Depth: 1}}) + require.Error(t, err) +} diff --git a/db/queries/templates.go b/db/queries/templates.go index 4277ea7..0b4597c 100644 --- a/db/queries/templates.go +++ b/db/queries/templates.go @@ -42,6 +42,7 @@ type Templates struct { CheckRepSetExists *template.Template GetTablesInRepSet *template.Template GetPkeyColumnTypes *template.Template + GetRelationTree *template.Template CreateMetadataTable *template.Template GetPkeyOffsets *template.Template @@ -652,6 +653,34 @@ var SQLTemplates = Templates{ CheckSchemaExists: template.Must(template.New("checkSchemaExists").Parse( `SELECT EXISTS (SELECT 1 FROM pg_namespace WHERE nspname = $1);`, )), + // GetRelationTree walks pg_inherits from one table down to every + // descendant in a single query. Depth 0 is the table itself. relkind + // tells heap (r), partitioned (p), and foreign (f) relations apart. + GetRelationTree: template.Must(template.New("getRelationTree").Parse(` + WITH RECURSIVE tree AS ( + SELECT c.oid, c.relkind, 0 AS depth, NULL::oid AS parent_oid + FROM pg_catalog.pg_class c + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = $1 AND c.relname = $2 + UNION ALL + SELECT c.oid, c.relkind, t.depth + 1, i.inhparent + FROM tree t + JOIN pg_catalog.pg_inherits i ON i.inhparent = t.oid + JOIN pg_catalog.pg_class c ON c.oid = i.inhrelid + ) + SELECT + n.nspname, + c.relname, + t.relkind::text, + t.depth, + COALESCE(pn.nspname || '.' || pc.relname, '') AS parent + FROM tree t + JOIN pg_catalog.pg_class c ON c.oid = t.oid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + LEFT JOIN pg_catalog.pg_class pc ON pc.oid = t.parent_oid + LEFT JOIN pg_catalog.pg_namespace pn ON pn.oid = pc.relnamespace + ORDER BY t.depth, n.nspname, c.relname; + `)), GetTablesInSchema: template.Must(template.New("getTablesInSchema").Parse(` SELECT table_name From d4a079a02228695999090308137bf074fc028f0d Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Wed, 2 Sep 2026 19:38:38 -0700 Subject: [PATCH 2/7] feat(mtree): refuse foreign tables and parents with foreign children Add checks in RunChecks to reject tables that are foreign tables or have foreign relations in their inheritance tree. Merkle tree support for inheritance with foreign children is deferred to a later ticket, so queries are rejected with clear messages instead of failing later during publication. --- internal/consistency/mtree/merkle.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/consistency/mtree/merkle.go b/internal/consistency/mtree/merkle.go index dfae413..578d43a 100644 --- a/internal/consistency/mtree/merkle.go +++ b/internal/consistency/mtree/merkle.go @@ -1579,6 +1579,18 @@ func (m *MerkleTreeTask) RunChecks(skipValidation bool) error { } defer tx.Rollback(m.Ctx) + tree, err := queries.GetRelationTree(m.Ctx, tx, m.Schema, m.Table) + if err != nil { + return fmt.Errorf("failed to read inheritance tree on node %s: %w", nodeInfo["Name"], err) + } + if tree != nil && tree.Root.RelKind == "f" { + return fmt.Errorf("'%s' is a foreign table on node %s; ACE does not build Merkle trees for foreign tables", m.QualifiedTableName, nodeInfo["Name"]) + } + if tree != nil && tree.HasForeign() { + return fmt.Errorf("'%s' has foreign relations in its inheritance tree on node %s (%s); mtree does not yet track inheritance children, so it cannot build a tree that skips them. Use table-diff for this table", + m.QualifiedTableName, nodeInfo["Name"], strings.Join(tree.ForeignRelations(), ", ")) + } + currentColsSlice, err := queries.GetColumns(m.Ctx, tx, m.Schema, m.Table) if err != nil { return fmt.Errorf("failed to get columns on node %s: %w", nodeInfo["Name"], err) From 8d9df2e810bf37b9d9d9c1605f6d67a30e62bc40 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Wed, 2 Sep 2026 19:43:51 -0700 Subject: [PATCH 3/7] feat(schema-diff): report skipped foreign tables; skip them in repset-diff schema-diff already excludes foreign tables via information_schema's table_type filter, but gave no indication of what it left out. It now looks up the foreign tables in the schema and logs them as skipped. repset-diff lists tables from spock.tables, which does not distinguish foreign tables from regular ones. It now checks each table's relation kind and skips (with a log line) any that turn out to be foreign. --- db/queries/queries.go | 23 ++++ db/queries/templates.go | 143 ++++++++++++----------- internal/consistency/diff/repset_diff.go | 15 ++- internal/consistency/diff/schema_diff.go | 12 +- 4 files changed, 125 insertions(+), 68 deletions(-) diff --git a/db/queries/queries.go b/db/queries/queries.go index dbb6f15..4b74b96 100644 --- a/db/queries/queries.go +++ b/db/queries/queries.go @@ -1137,6 +1137,29 @@ func GetTablesInSchema(ctx context.Context, db DBQuerier, schema string) ([]stri return tables, nil } +// GetForeignTablesInSchema lists the foreign tables in a schema so callers +// can say which tables a schema diff skipped. +func GetForeignTablesInSchema(ctx context.Context, db DBQuerier, schema string) ([]string, error) { + sql, err := RenderSQL(SQLTemplates.GetForeignTablesInSchema, nil) + if err != nil { + return nil, fmt.Errorf("failed to render GetForeignTablesInSchema SQL: %w", err) + } + rows, err := db.Query(ctx, sql, schema) + if err != nil { + return nil, fmt.Errorf("query to get foreign tables in schema %s failed: %w", schema, err) + } + defer rows.Close() + var tables []string + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + return nil, fmt.Errorf("failed to scan foreign table name: %w", err) + } + tables = append(tables, name) + } + return tables, rows.Err() +} + func GetViewsInSchema(ctx context.Context, db DBQuerier, schema string) ([]string, error) { sql, err := RenderSQL(SQLTemplates.GetViewsInSchema, nil) if err != nil { diff --git a/db/queries/templates.go b/db/queries/templates.go index 0b4597c..dd9f8f7 100644 --- a/db/queries/templates.go +++ b/db/queries/templates.go @@ -25,73 +25,74 @@ var aceTemplateFuncs = template.FuncMap{ } type Templates struct { - EstimateRowCount *template.Template - GetPrimaryKey *template.Template - GetColumnTypes *template.Template - GetColumns *template.Template - CheckUserPrivileges *template.Template - SpockNodeAndSubInfo *template.Template - SpockRepSetInfo *template.Template - EnsurePgcrypto *template.Template - GetSpockNodeNames *template.Template - CheckSchemaExists *template.Template - GetTablesInSchema *template.Template - GetViewsInSchema *template.Template - GetFunctionsInSchema *template.Template - GetIndicesInSchema *template.Template - CheckRepSetExists *template.Template - GetTablesInRepSet *template.Template - GetPkeyColumnTypes *template.Template - GetRelationTree *template.Template + EstimateRowCount *template.Template + GetPrimaryKey *template.Template + GetColumnTypes *template.Template + GetColumns *template.Template + CheckUserPrivileges *template.Template + SpockNodeAndSubInfo *template.Template + SpockRepSetInfo *template.Template + EnsurePgcrypto *template.Template + GetSpockNodeNames *template.Template + CheckSchemaExists *template.Template + GetTablesInSchema *template.Template + GetForeignTablesInSchema *template.Template + GetViewsInSchema *template.Template + GetFunctionsInSchema *template.Template + GetIndicesInSchema *template.Template + CheckRepSetExists *template.Template + GetTablesInRepSet *template.Template + GetPkeyColumnTypes *template.Template + GetRelationTree *template.Template - CreateMetadataTable *template.Template - GetPkeyOffsets *template.Template - CreateSimpleMtreeTable *template.Template - CreateIndex *template.Template - CreateCompositeType *template.Template - DropCompositeType *template.Template - CreateCompositeMtreeTable *template.Template - InsertCompositeBlockRanges *template.Template - CreateXORFunction *template.Template - GetPkeyType *template.Template - UpdateMetadata *template.Template - InsertBlockRanges *template.Template - InsertBlockRangesBatchSimple *template.Template - InsertBlockRangesBatchComposite *template.Template - TDBlockHashSQL *template.Template - MtreeLeafHashSQL *template.Template - UpdateLeafHashes *template.Template - UpdateLeafHashesBatch *template.Template + CreateMetadataTable *template.Template + GetPkeyOffsets *template.Template + CreateSimpleMtreeTable *template.Template + CreateIndex *template.Template + CreateCompositeType *template.Template + DropCompositeType *template.Template + CreateCompositeMtreeTable *template.Template + InsertCompositeBlockRanges *template.Template + CreateXORFunction *template.Template + GetPkeyType *template.Template + UpdateMetadata *template.Template + InsertBlockRanges *template.Template + InsertBlockRangesBatchSimple *template.Template + InsertBlockRangesBatchComposite *template.Template + TDBlockHashSQL *template.Template + MtreeLeafHashSQL *template.Template + UpdateLeafHashes *template.Template + UpdateLeafHashesBatch *template.Template - GetDirtyAndNewBlocks *template.Template - ClearDirtyFlags *template.Template - MarkLeavesDirtyByPositions *template.Template - BuildParentNodes *template.Template - GetRootNode *template.Template - GetNodeChildren *template.Template - GetLeafRanges *template.Template - GetLeafRangesExpanded *template.Template - GetRowCountEstimate *template.Template - GetMaxValComposite *template.Template - UpdateMaxVal *template.Template - GetMaxValSimple *template.Template - GetCountComposite *template.Template - GetCountSimple *template.Template + GetDirtyAndNewBlocks *template.Template + ClearDirtyFlags *template.Template + MarkLeavesDirtyByPositions *template.Template + BuildParentNodes *template.Template + GetRootNode *template.Template + GetNodeChildren *template.Template + GetLeafRanges *template.Template + GetLeafRangesExpanded *template.Template + GetRowCountEstimate *template.Template + GetMaxValComposite *template.Template + UpdateMaxVal *template.Template + GetMaxValSimple *template.Template + GetCountComposite *template.Template + GetCountSimple *template.Template - DeleteParentNodes *template.Template - GetMaxNodePosition *template.Template - UpdateBlockRangeEnd *template.Template - UpdateNodePositionsTemp *template.Template - DeleteBlock *template.Template - UpdateNodePositionsSequential *template.Template - FindBlocksToSplit *template.Template - FindBlocksToMerge *template.Template - FindBlocksToMergeExpanded *template.Template - GetBlockCountComposite *template.Template - GetBlockCountSimple *template.Template - GetBlockSizeFromMetadata *template.Template - GetMaxNodeLevel *template.Template - CompareBlocksSQL *template.Template + DeleteParentNodes *template.Template + GetMaxNodePosition *template.Template + UpdateBlockRangeEnd *template.Template + UpdateNodePositionsTemp *template.Template + DeleteBlock *template.Template + UpdateNodePositionsSequential *template.Template + FindBlocksToSplit *template.Template + FindBlocksToMerge *template.Template + FindBlocksToMergeExpanded *template.Template + GetBlockCountComposite *template.Template + GetBlockCountSimple *template.Template + GetBlockSizeFromMetadata *template.Template + GetMaxNodeLevel *template.Template + CompareBlocksSQL *template.Template DropXORFunction *template.Template DropMetadataTable *template.Template @@ -148,8 +149,8 @@ type Templates struct { SetupReplicationOriginXact *template.Template ResetReplicationOriginXact *template.Template - InitCDCMetadata *template.Template - CurrentWalInsertLSN *template.Template + InitCDCMetadata *template.Template + CurrentWalInsertLSN *template.Template } var SQLTemplates = Templates{ @@ -690,6 +691,16 @@ var SQLTemplates = Templates{ table_schema = $1 AND table_type = 'BASE TABLE'; `)), + GetForeignTablesInSchema: template.Must(template.New("getForeignTablesInSchema").Parse(` + SELECT + table_name + FROM + information_schema.tables + WHERE + table_schema = $1 + AND table_type = 'FOREIGN' + ORDER BY table_name; + `)), GetViewsInSchema: template.Must(template.New("getViewsInSchema").Parse(` SELECT table_name diff --git a/internal/consistency/diff/repset_diff.go b/internal/consistency/diff/repset_diff.go index eb97dc6..cf16874 100644 --- a/internal/consistency/diff/repset_diff.go +++ b/internal/consistency/diff/repset_diff.go @@ -181,17 +181,30 @@ func (c *RepsetDiffCmd) RunChecks(skipValidation bool) error { repsetNodeNames = append(repsetNodeNames, nodeName) tables, err := queries.GetTablesInRepSet(c.Ctx, pool, c.RepsetName) - pool.Close() if err != nil { + pool.Close() return fmt.Errorf("could not get tables in repset on node %s: %w", nodeName, err) } for _, t := range tables { + parts := strings.SplitN(t, ".", 2) + if len(parts) == 2 { + tree, terr := queries.GetRelationTree(c.Ctx, pool, parts[0], parts[1]) + if terr != nil { + pool.Close() + return fmt.Errorf("could not read relation kind for %s on node %s: %w", t, nodeName, terr) + } + if tree != nil && tree.Root.RelKind == "f" { + logger.Info("Skipping foreign table %s in repset %s on node %s", t, c.RepsetName, nodeName) + continue + } + } if tablePresence[t] == nil { tablePresence[t] = make(map[string]bool) } tablePresence[t][nodeName] = true } + pool.Close() } if len(repsetNodeNames) == 0 { diff --git a/internal/consistency/diff/schema_diff.go b/internal/consistency/diff/schema_diff.go index ff52175..2a33162 100644 --- a/internal/consistency/diff/schema_diff.go +++ b/internal/consistency/diff/schema_diff.go @@ -241,11 +241,21 @@ func (c *SchemaDiffCmd) RunChecks(skipValidation bool) error { } tables, err := queries.GetTablesInSchema(c.Ctx, pool, c.SchemaName) - pool.Close() if err != nil { + pool.Close() return fmt.Errorf("could not get tables in schema on node %s: %w", nodeName, err) } + foreign, ferr := queries.GetForeignTablesInSchema(c.Ctx, pool, c.SchemaName) + if ferr != nil { + pool.Close() + return fmt.Errorf("could not list foreign tables in schema on node %s: %w", nodeName, ferr) + } + if len(foreign) > 0 { + logger.Info("Skipping %d foreign table(s) in schema %s on node %s: %s", len(foreign), c.SchemaName, nodeName, strings.Join(foreign, ", ")) + } + pool.Close() + for _, t := range tables { if tablePresence[t] == nil { tablePresence[t] = make(map[string]bool) From 5246b644c1681ade1ecdad580115fc212d056a49 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Thu, 3 Sep 2026 13:49:18 -0700 Subject: [PATCH 4/7] feat: refuse foreign tables, views, and partitioned tables with foreign partitions with a clear message table-diff, table-repair, and mtree read the table's inheritance tree in one catalog query before the column and primary key checks, and refuse foreign tables, views, and any partitioned table or inheritance parent that contains a foreign relation, naming the relations involved. A view next to an underscore-prefixed table of the same name gets a hint mentioning that table, since it may be the table behind the view (the coldfront tiered layout). schema-diff lists the views it skips alongside the foreign tables. Docs cover the refused relation kinds and the coldfront watermark filter. --- db/queries/relations.go | 42 ++++++ db/queries/relations_test.go | 46 ++++++ docs/CHANGELOG.md | 12 ++ docs/commands/diff/table-diff.md | 34 +++++ internal/consistency/diff/schema_diff.go | 8 ++ internal/consistency/diff/table_diff.go | 18 +++ internal/consistency/mtree/merkle.go | 17 ++- internal/consistency/repair/table_repair.go | 18 +++ tests/integration/foreign_tables_test.go | 149 ++++++++++++++++++++ 9 files changed, 338 insertions(+), 6 deletions(-) create mode 100644 tests/integration/foreign_tables_test.go diff --git a/db/queries/relations.go b/db/queries/relations.go index b6aaa2d..a115c65 100644 --- a/db/queries/relations.go +++ b/db/queries/relations.go @@ -14,6 +14,7 @@ package queries import ( "context" "fmt" + "strings" ) // RelationInfo is one relation in an inheritance tree. @@ -151,3 +152,44 @@ func buildRelationTree(relations []RelationInfo) (*RelationTree, error) { } return tree, nil } + +// UnsupportedReason says why ACE cannot compare the tree's root relation, or +// returns "" for an ordinary heap or partitioned table whose tree holds no +// foreign relations. The text reads as a predicate on the table name, e.g. +// "'s.t' is a foreign table; ...". +func (t *RelationTree) UnsupportedReason() string { + switch t.Root.RelKind { + case "f": + return "is a foreign table; ACE does not compare foreign tables" + case "v", "m": + return "is a view; ACE compares tables" + } + if t.HasForeign() { + return fmt.Sprintf("has foreign relations in its inheritance tree (%s); ACE does not yet compare tables with foreign children or partitions", + strings.Join(t.ForeignRelations(), ", ")) + } + return "" +} + +// HotTableHint looks for a table named "_" next to a view. Some +// tiering extensions, coldfront among them, rename the real table that way +// and put a view in its place, so the underscore table may be the data the +// user meant to compare. When found, and ACE could compare it, it returns a +// sentence mentioning that table; otherwise "". +func HotTableHint(ctx context.Context, db DBQuerier, schema, table string) (string, error) { + hot, err := GetRelationTree(ctx, db, schema, "_"+table) + if err != nil { + return "", err + } + return hotTableHintText(schema, table, hot), nil +} + +// hotTableHintText renders the hint for a candidate hot table, or "" when +// there is none or ACE would refuse it anyway. +func hotTableHintText(schema, table string, hot *RelationTree) string { + if hot == nil || (hot.Root.RelKind != "r" && hot.Root.RelKind != "p") || hot.UnsupportedReason() != "" { + return "" + } + return fmt.Sprintf(" A table named '%s' also exists. It may be the table behind this view (tiering extensions such as coldfront use this layout); if so, compare '%s' instead.", + hot.Root.Qualified(), hot.Root.Qualified()) +} diff --git a/db/queries/relations_test.go b/db/queries/relations_test.go index baf66d3..666ece9 100644 --- a/db/queries/relations_test.go +++ b/db/queries/relations_test.go @@ -69,6 +69,30 @@ func TestRelationTree_IsInherited(t *testing.T) { assert.True(t, sampleTree().IsInherited()) assert.False(t, (&RelationTree{Root: RelationInfo{RelKind: "r"}}).IsInherited()) } + +func TestRelationTree_UnsupportedReason(t *testing.T) { + cases := []struct { + name string + tree *RelationTree + want string + }{ + {"heap table", &RelationTree{Root: RelationInfo{Schema: "s", Name: "t", RelKind: "r"}}, ""}, + {"partitioned, heap partitions only", &RelationTree{ + Root: RelationInfo{Schema: "s", Name: "p", RelKind: "p"}, + Descendants: []RelationInfo{{Schema: "s", Name: "p1", RelKind: "r"}}, + }, ""}, + {"foreign table", &RelationTree{Root: RelationInfo{Schema: "s", Name: "f", RelKind: "f"}}, "is a foreign table; ACE does not compare foreign tables"}, + {"view", &RelationTree{Root: RelationInfo{Schema: "s", Name: "v", RelKind: "v"}}, "is a view; ACE compares tables"}, + {"materialized view", &RelationTree{Root: RelationInfo{Schema: "s", Name: "mv", RelKind: "m"}}, "is a view; ACE compares tables"}, + {"heap parent with foreign child", sampleTree(), + "has foreign relations in its inheritance tree (s.child_fdw); ACE does not yet compare tables with foreign children or partitions"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, c.tree.UnsupportedReason()) + }) + } +} func TestBuildRelationTree_KeysBySchemaAndNameSeparately(t *testing.T) { // "a"."b.c" and "a.b"."c" both render as a.b.c. They must stay two // relations, so a foreign one among them is not dropped. @@ -102,3 +126,25 @@ func TestBuildRelationTree_DescendantBeforeRootIsAnError(t *testing.T) { _, err := buildRelationTree([]RelationInfo{{Schema: "s", Name: "c", RelKind: "r", Depth: 1}}) require.Error(t, err) } + +func TestHotTableHintText(t *testing.T) { + heap := &RelationTree{Root: RelationInfo{Schema: "public", Name: "_events", RelKind: "r"}} + assert.Contains(t, hotTableHintText("public", "events", heap), "compare 'public._events' instead") + + partitioned := &RelationTree{ + Root: RelationInfo{Schema: "public", Name: "_events", RelKind: "p"}, + Descendants: []RelationInfo{{Schema: "public", Name: "_events_p1", RelKind: "r", Depth: 1}}, + } + assert.Contains(t, hotTableHintText("public", "events", partitioned), "public._events") + + assert.Equal(t, "", hotTableHintText("public", "events", nil), "no underscore table") + + view := &RelationTree{Root: RelationInfo{Schema: "public", Name: "_events", RelKind: "v"}} + assert.Equal(t, "", hotTableHintText("public", "events", view), "underscore relation is itself a view") + + withForeignChild := &RelationTree{ + Root: RelationInfo{Schema: "public", Name: "_events", RelKind: "r"}, + Descendants: []RelationInfo{{Schema: "public", Name: "_events_cold", RelKind: "f", Depth: 1}}, + } + assert.Equal(t, "", hotTableHintText("public", "events", withForeignChild), "ACE would refuse this table too") +} diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index a9e4b1c..4818184 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,18 @@ All notable changes to ACE will be captured in this document. This project follo ## [v2.1.1] ### Added +- **Foreign tables, views, and partitioned tables with foreign partitions + are refused with a clear message.** `table-diff`, `table-repair`, and + `mtree` previously failed on these with "no primary key found", or in the + case of an inheritance parent with a foreign child, hashed the foreign rows + and then failed on a system column. ACE now reads the table's partitions and + children in one catalog query and says what the relation is and why it + cannot be compared. A view with an underscore-prefixed table of the same + name beside it gets a message mentioning that table, since it may be the + table behind the view (the coldfront tiered layout). `schema-diff` and + `repset-diff` list the foreign tables and views they skip. Comparing the + heap parts of a partitioned table that has foreign partitions is future + work. - **`repair` can now resolve `pick_freshest` by spock `commit_ts` (latest-commit-wins).** `pick_freshest` previously only compared ordinary data columns (e.g. `updated_at`); keying it on `commit_ts` silently fell back to diff --git a/docs/commands/diff/table-diff.md b/docs/commands/diff/table-diff.md index be9d79d..32379a4 100644 --- a/docs/commands/diff/table-diff.md +++ b/docs/commands/diff/table-diff.md @@ -74,6 +74,40 @@ ACE optimises comparisons with multiprocessing and block hashing: - It splits work into blocks (`--block-size`) and uses multiple workers per node (`--concurrency-factor`) to compute hashes. If hashes mismatch for a block, rows are materialised and, if necessary, recursively split using `--compare-unit-size`. - Runtime factors include host resources (CPU/memory), allowed parallelism, table size and row width (e.g., large JSON/bytea/embedding columns can slow hashing), distribution of differences (widely scattered diffs trigger more block fetches), and network latency to database nodes. +### Relations ACE does not compare + +ACE compares heap tables, including partitioned tables whose partitions are +all heap tables. It refuses everything else in the pre-checks, with a message +saying why: + +- **Foreign tables** (`file_fdw`, `postgres_fdw`, and so on) are refused. + `schema-diff` and `repset-diff` list the foreign tables they skip. +- **Views and materialized views** are refused. `schema-diff` compares views + as DDL only and lists the ones it skipped for data comparison. When a table + with the same name and a leading underscore exists next to the view (for + example `public._events` beside `public.events`), the message mentions it. + That table may be the one behind the view: tiering extensions such as + [coldfront](https://github.com/pgEdge/coldfront) rename the real table this + way and put a view in its place. If that is the case, compare the underscore + table. The cold tier behind a coldfront view is a shared Iceberg catalog, so + it has no per-node copies to compare, and a coldfront decoupled table has no + PostgreSQL data at all. +- **Partitioned tables with foreign partitions**, and inheritance parents with + foreign children, are refused, naming the foreign relations. Comparing the + heap parts of such a table while skipping the foreign parts is future work. + +If a view does stand in front of a coldfront tiered table, note that the +archiver moves rows from the hot table into Iceberg on a schedule, and the drop +of an archived partition reaches the other nodes through replicated DDL. In +that window the hot table legitimately differs between nodes. To compare only +rows still above the archive watermark, filter on the replicated watermark +table: + +``` +ace table-diff my-cluster public._events \ + --table-filter "ts >= (SELECT cutoff_time FROM coldfront.archive_watermark WHERE schema_name = 'public' AND table_name = 'events')" +``` + ### Tuning tips 1. Tune `--block-size` and `--concurrency-factor` for your hardware and data diff --git a/internal/consistency/diff/schema_diff.go b/internal/consistency/diff/schema_diff.go index 2a33162..a60dc04 100644 --- a/internal/consistency/diff/schema_diff.go +++ b/internal/consistency/diff/schema_diff.go @@ -254,6 +254,14 @@ func (c *SchemaDiffCmd) RunChecks(skipValidation bool) error { if len(foreign) > 0 { logger.Info("Skipping %d foreign table(s) in schema %s on node %s: %s", len(foreign), c.SchemaName, nodeName, strings.Join(foreign, ", ")) } + views, verr := queries.GetViewsInSchema(c.Ctx, pool, c.SchemaName) + if verr != nil { + pool.Close() + return fmt.Errorf("could not list views in schema on node %s: %w", nodeName, verr) + } + if len(views) > 0 { + logger.Info("Skipping %d view(s) in schema %s on node %s (views are compared as DDL only): %s", len(views), c.SchemaName, nodeName, strings.Join(views, ", ")) + } pool.Close() for _, t := range tables { diff --git a/internal/consistency/diff/table_diff.go b/internal/consistency/diff/table_diff.go index 47f7f2c..457d4c6 100644 --- a/internal/consistency/diff/table_diff.go +++ b/internal/consistency/diff/table_diff.go @@ -819,6 +819,24 @@ func (t *TableDiffTask) RunChecks(skipValidation bool) (err error) { } defer conn.Close() + tree, err := queries.GetRelationTree(t.Ctx, conn, schema, table) + if err != nil { + return fmt.Errorf("failed to read inheritance tree for %s.%s on node %s: %w", schema, table, hostname, err) + } + if tree == nil { + return fmt.Errorf("table '%s.%s' not found on %s, or the current user does not have adequate privileges", schema, table, hostname) + } + if reason := tree.UnsupportedReason(); reason != "" { + hint := "" + if tree.Root.RelKind == "v" || tree.Root.RelKind == "m" { + hint, err = queries.HotTableHint(t.Ctx, conn, schema, table) + if err != nil { + return fmt.Errorf("failed to look up the hot table for view %s.%s on node %s: %w", schema, table, hostname, err) + } + } + return fmt.Errorf("'%s.%s' %s (node %s).%s", schema, table, reason, hostname, hint) + } + currCols, err := queries.GetColumns(t.Ctx, conn, schema, table) if err != nil { return fmt.Errorf("failed to get columns for table %s.%s on node %s: %w", schema, table, hostname, err) diff --git a/internal/consistency/mtree/merkle.go b/internal/consistency/mtree/merkle.go index 578d43a..e336574 100644 --- a/internal/consistency/mtree/merkle.go +++ b/internal/consistency/mtree/merkle.go @@ -1583,12 +1583,17 @@ func (m *MerkleTreeTask) RunChecks(skipValidation bool) error { if err != nil { return fmt.Errorf("failed to read inheritance tree on node %s: %w", nodeInfo["Name"], err) } - if tree != nil && tree.Root.RelKind == "f" { - return fmt.Errorf("'%s' is a foreign table on node %s; ACE does not build Merkle trees for foreign tables", m.QualifiedTableName, nodeInfo["Name"]) - } - if tree != nil && tree.HasForeign() { - return fmt.Errorf("'%s' has foreign relations in its inheritance tree on node %s (%s); mtree does not yet track inheritance children, so it cannot build a tree that skips them. Use table-diff for this table", - m.QualifiedTableName, nodeInfo["Name"], strings.Join(tree.ForeignRelations(), ", ")) + if tree != nil { + if reason := tree.UnsupportedReason(); reason != "" { + hint := "" + if tree.Root.RelKind == "v" || tree.Root.RelKind == "m" { + hint, err = queries.HotTableHint(m.Ctx, tx, m.Schema, m.Table) + if err != nil { + return fmt.Errorf("failed to look up the hot table for view %s on node %s: %w", m.QualifiedTableName, nodeInfo["Name"], err) + } + } + return fmt.Errorf("'%s' %s (node %s).%s", m.QualifiedTableName, reason, nodeInfo["Name"], hint) + } } currentColsSlice, err := queries.GetColumns(m.Ctx, tx, m.Schema, m.Table) diff --git a/internal/consistency/repair/table_repair.go b/internal/consistency/repair/table_repair.go index 635514f..32768a4 100644 --- a/internal/consistency/repair/table_repair.go +++ b/internal/consistency/repair/table_repair.go @@ -459,6 +459,24 @@ func (t *TableRepairTask) ValidateAndPrepare() error { } t.Pools[nodeName] = connPool + tree, err := queries.GetRelationTree(t.Ctx, connPool, t.Schema, t.Table) + if err != nil { + return fmt.Errorf("failed to read inheritance tree for %s.%s on node %s: %w", t.Schema, t.Table, nodeName, err) + } + if tree == nil { + return fmt.Errorf("table '%s.%s' not found on node %s, or the current user does not have adequate privileges", t.Schema, t.Table, nodeName) + } + if reason := tree.UnsupportedReason(); reason != "" { + hint := "" + if tree.Root.RelKind == "v" || tree.Root.RelKind == "m" { + hint, err = queries.HotTableHint(t.Ctx, connPool, t.Schema, t.Table) + if err != nil { + return fmt.Errorf("failed to look up the hot table for view %s.%s on node %s: %w", t.Schema, t.Table, nodeName, err) + } + } + return fmt.Errorf("'%s.%s' %s (node %s).%s", t.Schema, t.Table, reason, nodeName, hint) + } + cols, err := queries.GetColumns(t.Ctx, connPool, t.Schema, t.Table) if err != nil { return fmt.Errorf("failed to get columns for %s.%s on node %s: %w", t.Schema, t.Table, nodeName, err) diff --git a/tests/integration/foreign_tables_test.go b/tests/integration/foreign_tables_test.go new file mode 100644 index 0000000..24f285e --- /dev/null +++ b/tests/integration/foreign_tables_test.go @@ -0,0 +1,149 @@ +// /////////////////////////////////////////////////////////////////////////// +// +// # ACE - Active Consistency Engine +// +// Copyright (C) 2023 - 2026, pgEdge (https://www.pgedge.com/) +// +// This software is released under the PostgreSQL License: +// https://opensource.org/license/postgresql +// +// /////////////////////////////////////////////////////////////////////////// + +package integration + +import ( + "context" + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + "github.com/pgedge/ace/internal/consistency/diff" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + fdwPlainSchema = "fdwplain" // heap tables, a foreign table, views + fdwMixedSchema = "fdwmixed" // heap parent with a foreign child + fdwPartSchema = "fdwpart" // partitioned parent with a foreign partition +) + +// setupNonHeapFixtures creates, on one node, the relations ACE must refuse +// or skip: a file_fdw table, a view over a renamed table (the coldfront +// layout), a plain view, a heap parent with a foreign child, a heap parent +// with only heap children, and a partitioned table with a foreign +// partition. The CSV the foreign tables read is written inside the +// container with a server-side COPY. +func setupNonHeapFixtures(t *testing.T, ctx context.Context, pool *pgxpool.Pool) { + t.Helper() + stmts := []string{ + "CREATE EXTENSION IF NOT EXISTS file_fdw", + "CREATE SERVER IF NOT EXISTS ace_test_csv FOREIGN DATA WRAPPER file_fdw", + "COPY (SELECT * FROM (VALUES (101,'c1'),(102,'c2')) v(id, val)) TO '/tmp/ace_fdw_rows.csv' CSV", + + fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", fdwPlainSchema), + fmt.Sprintf("CREATE SCHEMA %s", fdwPlainSchema), + fmt.Sprintf("CREATE TABLE %s.t_heap (id int PRIMARY KEY, val text)", fdwPlainSchema), + fmt.Sprintf("INSERT INTO %s.t_heap VALUES (1,'a'),(2,'b')", fdwPlainSchema), + fmt.Sprintf("CREATE FOREIGN TABLE %s.t_foreign (id int, val text) SERVER ace_test_csv OPTIONS (filename '/tmp/ace_fdw_rows.csv', format 'csv')", fdwPlainSchema), + fmt.Sprintf("CREATE TABLE %s._orders (id bigint PRIMARY KEY, ts timestamptz)", fdwPlainSchema), + fmt.Sprintf("CREATE VIEW %s.orders AS SELECT id, ts FROM %s._orders", fdwPlainSchema, fdwPlainSchema), + fmt.Sprintf("CREATE VIEW %s.plainview AS SELECT id FROM %s.t_heap", fdwPlainSchema, fdwPlainSchema), + fmt.Sprintf("CREATE TABLE %s.heap_parent (id int PRIMARY KEY, val text)", fdwPlainSchema), + fmt.Sprintf("INSERT INTO %s.heap_parent VALUES (1,'p1')", fdwPlainSchema), + fmt.Sprintf("CREATE TABLE %s.heap_child (PRIMARY KEY (id)) INHERITS (%s.heap_parent)", fdwPlainSchema, fdwPlainSchema), + fmt.Sprintf("INSERT INTO %s.heap_child VALUES (11,'h1')", fdwPlainSchema), + + fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", fdwMixedSchema), + fmt.Sprintf("CREATE SCHEMA %s", fdwMixedSchema), + fmt.Sprintf("CREATE TABLE %s.parent (id int PRIMARY KEY, val text)", fdwMixedSchema), + fmt.Sprintf("CREATE TABLE %s.child_heap (PRIMARY KEY (id)) INHERITS (%s.parent)", fdwMixedSchema, fdwMixedSchema), + fmt.Sprintf("CREATE FOREIGN TABLE %s.child_fdw () INHERITS (%s.parent) SERVER ace_test_csv OPTIONS (filename '/tmp/ace_fdw_rows.csv', format 'csv')", fdwMixedSchema, fdwMixedSchema), + + fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", fdwPartSchema), + fmt.Sprintf("CREATE SCHEMA %s", fdwPartSchema), + fmt.Sprintf("CREATE TABLE %s.part_parent (id int, val text) PARTITION BY RANGE (id)", fdwPartSchema), + fmt.Sprintf("CREATE TABLE %s.part_heap PARTITION OF %s.part_parent FOR VALUES FROM (0) TO (100)", fdwPartSchema, fdwPartSchema), + fmt.Sprintf("CREATE FOREIGN TABLE %s.part_fdw PARTITION OF %s.part_parent FOR VALUES FROM (100) TO (200) SERVER ace_test_csv OPTIONS (filename '/tmp/ace_fdw_rows.csv', format 'csv')", fdwPartSchema, fdwPartSchema), + } + for _, s := range stmts { + _, err := pool.Exec(ctx, s) + require.NoError(t, err, "statement: %s", s) + } +} + +// TestNativePGNonHeapRelations checks that ACE refuses foreign tables, +// views, and trees containing foreign relations with a message that says +// why, and that ordinary tables next to them are unaffected. +func TestNativePGNonHeapRelations(t *testing.T) { + state := setupNativeCluster(t) + t.Cleanup(func() { state.teardown(t) }) + state.writeClusterConfig(t) + ctx := context.Background() + env := newNativeEnv(state) + nodes := []string{env.ServiceN1, env.ServiceN2} + + setupNonHeapFixtures(t, ctx, state.n1Pool) + setupNonHeapFixtures(t, ctx, state.n2Pool) + + expectCheckError := func(t *testing.T, table string, fragments ...string) { + t.Helper() + task := env.newTableDiffTask(t, table, nodes) + err := task.RunChecks(false) + require.Error(t, err, "table-diff must refuse %s", table) + for _, f := range fragments { + assert.Contains(t, err.Error(), f) + } + } + + t.Run("ForeignTableRefused", func(t *testing.T) { + expectCheckError(t, fdwPlainSchema+".t_foreign", "is a foreign table") + }) + + t.Run("ViewRefusedWithHotTableHint", func(t *testing.T) { + expectCheckError(t, fdwPlainSchema+".orders", "is a view", fdwPlainSchema+"._orders") + }) + + t.Run("PlainViewRefusedWithoutHint", func(t *testing.T) { + task := env.newTableDiffTask(t, fdwPlainSchema+".plainview", nodes) + err := task.RunChecks(false) + require.Error(t, err) + assert.Contains(t, err.Error(), "is a view") + assert.NotContains(t, err.Error(), "_plainview") + }) + + t.Run("ParentWithForeignChildRefused", func(t *testing.T) { + expectCheckError(t, fdwMixedSchema+".parent", "foreign relations in its inheritance tree", fdwMixedSchema+".child_fdw") + }) + + t.Run("PartitionedWithForeignPartitionRefused", func(t *testing.T) { + expectCheckError(t, fdwPartSchema+".part_parent", "foreign relations in its inheritance tree", fdwPartSchema+".part_fdw") + }) + + t.Run("MtreeBuildRefusesParentWithForeignChild", func(t *testing.T) { + task := env.newMerkleTreeTask(t, fdwMixedSchema+".parent", nodes) + err := task.RunChecks(false) + require.Error(t, err) + assert.Contains(t, err.Error(), fdwMixedSchema+".child_fdw") + }) + + t.Run("HeapParentWithHeapChildrenStillDiffs", func(t *testing.T) { + env.assertNoTableDiff(t, fdwPlainSchema+".heap_parent") + }) + + t.Run("SchemaDiffSkipsForeignTablesAndViews", func(t *testing.T) { + task := diff.NewSchemaDiffTask() + task.ClusterName = env.ClusterName + task.DBName = env.DBName + task.SchemaName = fdwPlainSchema + task.Nodes = "all" + task.Output = "json" + task.Quiet = true + task.SkipDBUpdate = true + task.BlockSize = 10000 + task.CompareUnitSize = 100 + task.ConcurrencyFactor = 1 + require.NoError(t, task.RunChecks(false)) + require.NoError(t, task.SchemaTableDiff()) + }) +} From bdc1950aa3c07530c3e111e308bbcd58d972773c Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Thu, 3 Sep 2026 11:17:56 -0700 Subject: [PATCH 5/7] ci: run query, diff, and repair unit tests --- .github/workflows/test.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4c789fb..ee74365 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -43,6 +43,9 @@ jobs: - name: Run common unit tests run: go test -count=1 -v ./pkg/common + - name: Run query, diff, and repair unit tests + run: go test -count=1 -v ./db/queries ./internal/consistency/diff ./internal/consistency/repair + - name: Run mtree missing-tree fail-fast test run: go test -count=1 -v ./tests/integration -run 'TestMtreeDiffFailsFastWhenTreeNotBuilt' From 565de7b379b40b1a6fe6169e75ccf620f088e5b3 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Fri, 4 Sep 2026 13:12:14 -0700 Subject: [PATCH 6/7] fix: name every non-table relation kind, and say why a foreign table cannot be compared Review feedback on the relation pre-check: - UnsupportedReason allowed only r and p implicitly and named only f, v, and m. A sequence, index, composite type, partitioned index, or TOAST table fell through to the column lookup and came back as "not found, or inadequate privileges". The switch is now an allow-list: r and p pass, every other relkind gets a message naming what it is, with a generic fallback for kinds this code has not seen. - mtree wrapped the check in `if tree != nil` and only reported a missing table two queries later through the column count. It now returns the not-found error at the tree check, the same as table-diff and table-repair. - The foreign-table message stated the refusal without the reason. It now says the rows live outside PostgreSQL and the table cannot have a primary key, so ACE has nothing to compare. Docs list the additional refused kinds. --- db/queries/relations.go | 21 +++++++++++++++++---- db/queries/relations_test.go | 8 +++++++- docs/commands/diff/table-diff.md | 4 +++- internal/consistency/mtree/merkle.go | 19 ++++++++++--------- 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/db/queries/relations.go b/db/queries/relations.go index a115c65..e58590b 100644 --- a/db/queries/relations.go +++ b/db/queries/relations.go @@ -154,15 +154,28 @@ func buildRelationTree(relations []RelationInfo) (*RelationTree, error) { } // UnsupportedReason says why ACE cannot compare the tree's root relation, or -// returns "" for an ordinary heap or partitioned table whose tree holds no -// foreign relations. The text reads as a predicate on the table name, e.g. -// "'s.t' is a foreign table; ...". +// returns "" for a heap or partitioned table whose tree holds no foreign +// relations. Only relkinds r and p are comparable; every other kind gets a +// message naming what it is. The text reads as a predicate on the table +// name, e.g. "'s.t' is a foreign table; ...". func (t *RelationTree) UnsupportedReason() string { switch t.Root.RelKind { + case "r", "p": + // comparable; fall through to the foreign-descendant check below case "f": - return "is a foreign table; ACE does not compare foreign tables" + return "is a foreign table; its rows live outside PostgreSQL and it cannot have a primary key, so ACE has nothing to compare" case "v", "m": return "is a view; ACE compares tables" + case "S": + return "is a sequence; ACE compares tables" + case "c": + return "is a composite type; ACE compares tables" + case "i", "I": + return "is an index; ACE compares tables" + case "t": + return "is a TOAST table; ACE compares tables" + default: + return fmt.Sprintf("is not a table (relkind %q); ACE compares tables", t.Root.RelKind) } if t.HasForeign() { return fmt.Sprintf("has foreign relations in its inheritance tree (%s); ACE does not yet compare tables with foreign children or partitions", diff --git a/db/queries/relations_test.go b/db/queries/relations_test.go index 666ece9..4c7b2cc 100644 --- a/db/queries/relations_test.go +++ b/db/queries/relations_test.go @@ -81,9 +81,15 @@ func TestRelationTree_UnsupportedReason(t *testing.T) { Root: RelationInfo{Schema: "s", Name: "p", RelKind: "p"}, Descendants: []RelationInfo{{Schema: "s", Name: "p1", RelKind: "r"}}, }, ""}, - {"foreign table", &RelationTree{Root: RelationInfo{Schema: "s", Name: "f", RelKind: "f"}}, "is a foreign table; ACE does not compare foreign tables"}, + {"foreign table", &RelationTree{Root: RelationInfo{Schema: "s", Name: "f", RelKind: "f"}}, "is a foreign table; its rows live outside PostgreSQL and it cannot have a primary key, so ACE has nothing to compare"}, {"view", &RelationTree{Root: RelationInfo{Schema: "s", Name: "v", RelKind: "v"}}, "is a view; ACE compares tables"}, {"materialized view", &RelationTree{Root: RelationInfo{Schema: "s", Name: "mv", RelKind: "m"}}, "is a view; ACE compares tables"}, + {"sequence", &RelationTree{Root: RelationInfo{Schema: "s", Name: "seq", RelKind: "S"}}, "is a sequence; ACE compares tables"}, + {"composite type", &RelationTree{Root: RelationInfo{Schema: "s", Name: "ct", RelKind: "c"}}, "is a composite type; ACE compares tables"}, + {"index", &RelationTree{Root: RelationInfo{Schema: "s", Name: "ix", RelKind: "i"}}, "is an index; ACE compares tables"}, + {"partitioned index", &RelationTree{Root: RelationInfo{Schema: "s", Name: "pix", RelKind: "I"}}, "is an index; ACE compares tables"}, + {"toast table", &RelationTree{Root: RelationInfo{Schema: "s", Name: "pg_toast_1", RelKind: "t"}}, "is a TOAST table; ACE compares tables"}, + {"unknown relkind", &RelationTree{Root: RelationInfo{Schema: "s", Name: "x", RelKind: "z"}}, `is not a table (relkind "z"); ACE compares tables`}, {"heap parent with foreign child", sampleTree(), "has foreign relations in its inheritance tree (s.child_fdw); ACE does not yet compare tables with foreign children or partitions"}, } diff --git a/docs/commands/diff/table-diff.md b/docs/commands/diff/table-diff.md index 32379a4..4bf941c 100644 --- a/docs/commands/diff/table-diff.md +++ b/docs/commands/diff/table-diff.md @@ -95,6 +95,8 @@ saying why: - **Partitioned tables with foreign partitions**, and inheritance parents with foreign children, are refused, naming the foreign relations. Comparing the heap parts of such a table while skipping the foreign parts is future work. +- **Sequences, indexes, composite types, and TOAST tables** are refused with a + message naming what the relation is. If a view does stand in front of a coldfront tiered table, note that the archiver moves rows from the hot table into Iceberg on a schedule, and the drop @@ -103,7 +105,7 @@ that window the hot table legitimately differs between nodes. To compare only rows still above the archive watermark, filter on the replicated watermark table: -``` +```sh ace table-diff my-cluster public._events \ --table-filter "ts >= (SELECT cutoff_time FROM coldfront.archive_watermark WHERE schema_name = 'public' AND table_name = 'events')" ``` diff --git a/internal/consistency/mtree/merkle.go b/internal/consistency/mtree/merkle.go index e336574..ac05e3d 100644 --- a/internal/consistency/mtree/merkle.go +++ b/internal/consistency/mtree/merkle.go @@ -1583,17 +1583,18 @@ func (m *MerkleTreeTask) RunChecks(skipValidation bool) error { if err != nil { return fmt.Errorf("failed to read inheritance tree on node %s: %w", nodeInfo["Name"], err) } - if tree != nil { - if reason := tree.UnsupportedReason(); reason != "" { - hint := "" - if tree.Root.RelKind == "v" || tree.Root.RelKind == "m" { - hint, err = queries.HotTableHint(m.Ctx, tx, m.Schema, m.Table) - if err != nil { - return fmt.Errorf("failed to look up the hot table for view %s on node %s: %w", m.QualifiedTableName, nodeInfo["Name"], err) - } + if tree == nil { + return fmt.Errorf("table '%s' not found on %s, or the current user does not have adequate privileges", m.QualifiedTableName, nodeInfo["Name"]) + } + if reason := tree.UnsupportedReason(); reason != "" { + hint := "" + if tree.Root.RelKind == "v" || tree.Root.RelKind == "m" { + hint, err = queries.HotTableHint(m.Ctx, tx, m.Schema, m.Table) + if err != nil { + return fmt.Errorf("failed to look up the hot table for view %s on node %s: %w", m.QualifiedTableName, nodeInfo["Name"], err) } - return fmt.Errorf("'%s' %s (node %s).%s", m.QualifiedTableName, reason, nodeInfo["Name"], hint) } + return fmt.Errorf("'%s' %s (node %s).%s", m.QualifiedTableName, reason, nodeInfo["Name"], hint) } currentColsSlice, err := queries.GetColumns(m.Ctx, tx, m.Schema, m.Table) From 606f08a209aca21e8126771f83c7221990437a97 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Mon, 7 Sep 2026 18:25:49 -0700 Subject: [PATCH 7/7] fix(repset-diff): leave out relations ACE cannot compare instead of failing the run on them Review feedback. repset-diff gated only on relkind f, so a partitioned parent with a foreign partition (which Spock accepts into an insert-only set along with its partitions) went on to table-diff and failed the whole run. It now skips anything UnsupportedReason refuses and logs the reason. GetTablesInRepSet returned nspname and relname joined with a dot and repset-diff split them again, so a schema containing a dot resolved to the wrong pair and the check was skipped silently. The query now returns the two parts separately. Integration test on the Spock stack: partitioned parent with a file_fdw partition in default_insert_only; the parent is skipped, a control table still diffs. --- db/queries/queries.go | 24 ++++++-- db/queries/templates.go | 2 +- internal/consistency/diff/repset_diff.go | 28 +++++---- tests/integration/repset_diff_test.go | 76 ++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 18 deletions(-) diff --git a/db/queries/queries.go b/db/queries/queries.go index 4b74b96..bad88ed 100644 --- a/db/queries/queries.go +++ b/db/queries/queries.go @@ -1259,7 +1259,21 @@ func CheckRepSetExists(ctx context.Context, db DBQuerier, repSet string) (bool, return exists, nil } -func GetTablesInRepSet(ctx context.Context, db DBQuerier, repSet string) ([]string, error) { +// QualifiedName is a relation's schema and name kept apart, so a schema or +// name containing a dot cannot be mistaken for the separator. +type QualifiedName struct { + Schema string + Name string +} + +// String renders schema.name without quoting, the form the rest of ACE +// passes around as a qualified table name. +func (q QualifiedName) String() string { + return q.Schema + "." + q.Name +} + +// GetTablesInRepSet lists the relations Spock has in a replication set. +func GetTablesInRepSet(ctx context.Context, db DBQuerier, repSet string) ([]QualifiedName, error) { sql, err := RenderSQL(SQLTemplates.GetTablesInRepSet, nil) if err != nil { return nil, fmt.Errorf("failed to render GetTablesInRepSet SQL: %w", err) @@ -1271,13 +1285,13 @@ func GetTablesInRepSet(ctx context.Context, db DBQuerier, repSet string) ([]stri } defer rows.Close() - var tables []string + var tables []QualifiedName for rows.Next() { - var tableName string - if err := rows.Scan(&tableName); err != nil { + var q QualifiedName + if err := rows.Scan(&q.Schema, &q.Name); err != nil { return nil, fmt.Errorf("failed to scan table name: %w", err) } - tables = append(tables, tableName) + tables = append(tables, q) } if err := rows.Err(); err != nil { diff --git a/db/queries/templates.go b/db/queries/templates.go index dd9f8f7..7c741eb 100644 --- a/db/queries/templates.go +++ b/db/queries/templates.go @@ -725,7 +725,7 @@ var SQLTemplates = Templates{ `SELECT EXISTS(SELECT 1 FROM spock.replication_set WHERE set_name = $1);`, )), GetTablesInRepSet: template.Must(template.New("getTablesInRepSet").Parse( - `SELECT concat_ws('.', nspname, relname) FROM spock.tables where set_name = $1;`, + `SELECT nspname, relname FROM spock.tables WHERE set_name = $1;`, )), GetPkeyColumnTypes: template.Must(template.New("getPkeyColumnTypes").Parse(` SELECT diff --git a/internal/consistency/diff/repset_diff.go b/internal/consistency/diff/repset_diff.go index cf16874..4b8c1f4 100644 --- a/internal/consistency/diff/repset_diff.go +++ b/internal/consistency/diff/repset_diff.go @@ -186,18 +186,22 @@ func (c *RepsetDiffCmd) RunChecks(skipValidation bool) error { return fmt.Errorf("could not get tables in repset on node %s: %w", nodeName, err) } - for _, t := range tables { - parts := strings.SplitN(t, ".", 2) - if len(parts) == 2 { - tree, terr := queries.GetRelationTree(c.Ctx, pool, parts[0], parts[1]) - if terr != nil { - pool.Close() - return fmt.Errorf("could not read relation kind for %s on node %s: %w", t, nodeName, terr) - } - if tree != nil && tree.Root.RelKind == "f" { - logger.Info("Skipping foreign table %s in repset %s on node %s", t, c.RepsetName, nodeName) - continue - } + for _, rel := range tables { + t := rel.String() + // Spock lets foreign tables into a set, directly or as partitions + // of a partitioned table it adds. Skip whatever the table-diff + // pre-check would refuse, so one such relation does not fail the + // whole run. + tree, terr := queries.GetRelationTree(c.Ctx, pool, rel.Schema, rel.Name) + if terr != nil { + pool.Close() + return fmt.Errorf("could not read relation kind for %s on node %s: %w", t, nodeName, terr) + } + if tree == nil { + logger.Warn("Table %s is in repset %s on node %s but was not found in the catalog", t, c.RepsetName, nodeName) + } else if reason := tree.UnsupportedReason(); reason != "" { + logger.Info("Skipping %s in repset %s on node %s: %s", t, c.RepsetName, nodeName, reason) + continue } if tablePresence[t] == nil { tablePresence[t] = make(map[string]bool) diff --git a/tests/integration/repset_diff_test.go b/tests/integration/repset_diff_test.go index a26abdd..bade227 100644 --- a/tests/integration/repset_diff_test.go +++ b/tests/integration/repset_diff_test.go @@ -301,3 +301,79 @@ func TestRepsetDiff_MultipleTables(t *testing.T) { assert.Equal(t, json.Number("99"), id) } } + +// TestRepsetDiff_SkipsForeignRelations covers a partitioned table with a +// file_fdw partition in the replication set. spock.repset_add_table on the +// parent adds the parent and every partition, so the set holds a partitioned +// parent whose tree contains a foreign relation, the foreign partition itself, +// and a heap partition. repset-diff must skip the first two and diff the +// rest, instead of failing the whole run on the parent. +func TestRepsetDiff_SkipsForeignRelations(t *testing.T) { + ctx := context.Background() + // The default set replicates UPDATE and DELETE and so demands a replica + // identity, which a partitioned table with a foreign partition cannot + // have. The insert-only set has no such requirement. + const repsetName = "default_insert_only" + parent := fmt.Sprintf("%s.rs_part_parent", testSchema) + heapPart := fmt.Sprintf("%s.rs_part_heap", testSchema) + fdwPart := fmt.Sprintf("%s.rs_part_fdw", testSchema) + + pools := []*pgxpool.Pool{pgCluster.Node1Pool, pgCluster.Node2Pool} + for _, pool := range pools { + for _, s := range []string{ + "CREATE EXTENSION IF NOT EXISTS file_fdw", + "CREATE SERVER IF NOT EXISTS ace_test_csv FOREIGN DATA WRAPPER file_fdw", + "COPY (SELECT * FROM (VALUES (101,'c1'),(102,'c2')) v(id, val)) TO '/tmp/ace_rs_rows.csv' CSV", + fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s (id int, val text) PARTITION BY RANGE (id)", parent), + fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s PARTITION OF %s FOR VALUES FROM (0) TO (100)", heapPart, parent), + // A leaf partition may carry its own key even though the parent + // cannot; ACE needs it to diff the partition on its own. + fmt.Sprintf("ALTER TABLE %s ADD PRIMARY KEY (id)", heapPart), + fmt.Sprintf("CREATE FOREIGN TABLE IF NOT EXISTS %s PARTITION OF %s FOR VALUES FROM (100) TO (200) SERVER ace_test_csv OPTIONS (filename '/tmp/ace_rs_rows.csv', format 'csv')", fdwPart, parent), + fmt.Sprintf("INSERT INTO %s VALUES (1, 'same') ON CONFLICT DO NOTHING", heapPart), + } { + _, err := pool.Exec(ctx, s) + require.NoError(t, err, "statement: %s", s) + } + // Try the parent first (Spock adds its partitions with it). If Spock + // refuses, add the foreign partition directly; the test only needs + // one non-heap relation in the set, and the log says which way got it in. + if _, err := pool.Exec(ctx, fmt.Sprintf(`SELECT spock.repset_add_table('%s', '%s');`, repsetName, parent)); err != nil { + t.Logf("repset_add_table(%s) refused the partitioned parent: %v", repsetName, err) + _, err = pool.Exec(ctx, fmt.Sprintf(`SELECT spock.repset_add_table('%s', '%s');`, repsetName, fdwPart)) + require.NoError(t, err, "add foreign partition to repset directly") + _, err = pool.Exec(ctx, fmt.Sprintf(`SELECT spock.repset_add_table('%s', '%s');`, repsetName, heapPart)) + require.NoError(t, err, "add heap partition to repset") + } else { + t.Logf("repset_add_table(%s) accepted the partitioned parent", repsetName) + } + } + t.Cleanup(func() { + for _, pool := range pools { + for _, rel := range []string{parent, heapPart, fdwPart} { + pool.Exec(ctx, fmt.Sprintf(`SELECT spock.repset_remove_table('%s', '%s');`, repsetName, rel)) + } + pool.Exec(ctx, fmt.Sprintf(`DROP TABLE IF EXISTS %s CASCADE`, parent)) + } + }) + + // Confirm the premise: the set now holds a relation that is not a heap table. + var nonHeap int + err := pgCluster.Node1Pool.QueryRow(ctx, + `SELECT count(*) FROM spock.tables s JOIN pg_class c ON c.relname = s.relname + JOIN pg_namespace n ON n.oid = c.relnamespace AND n.nspname = s.nspname + WHERE s.set_name = $1 AND c.relkind IN ('f', 'p')`, repsetName).Scan(&nonHeap) + require.NoError(t, err) + require.Greater(t, nonHeap, 0, "repset_add_table on a partitioned parent should put non-heap relations in the set") + + // A control table proves ordinary tables are still diffed. + control := createRepsetDiffTable(t, "rs_fdw_control", repsetName, true) + + task := newTestRepsetDiffTask(repsetName) + require.NoError(t, diff.RepsetDiff(task), "repset-diff must not fail because the set contains a partitioned parent with a foreign partition") + + files := repsetDiffFilesForTable(t, "rs_fdw_control") + require.Len(t, files, 1, "the control table should still be diffed: %s", control) + assert.Empty(t, repsetDiffFilesForTable(t, "rs_part_parent"), "the parent must be skipped, not diffed") + assert.Empty(t, repsetDiffFilesForTable(t, "rs_part_fdw"), "the foreign partition must be skipped, not diffed") +}