Critical Performance and Design Issues in PostgreSQL Storage Layer

[jigar-joshi-nirmata]

🚨 Summary

The current PostgreSQL storage implementation has critical performance bottlenecks and design issues

📊 Impact

• Kyverno policy enforcement is delayed (waiting on report storage)
• API server experiences high load due to slow queries

---

🐛 Critical Issues

Issue #1: Global Mutex Locks Serialize All Operations 🔴

Location: pkg/storage/db/polr.go:19-23, cpolr.go, ephr.go, cephr.go, report.go, clusterreport.go

Problem:

type polrdb struct {
    sync.Mutex  // ← ONE LOCK FOR ALL POLICY REPORTS
    MultiDB   *MultiDB
}

func (p *polrdb) Create(ctx context.Context, report *PolicyReport) error {
    p.Lock()    // ← Blocks ALL other operations
    defer p.Unlock()
    // ... database work ...
}

---

Issue #2: Code Duplication (969 Lines × 6 Times) 🔴

Location: All files in pkg/storage/db/

Problem:

polr.go           - 181 lines (PolicyReport CRUD)
cpolr.go          - 170 lines (ClusterPolicyReport CRUD) - 95% identical
ephr.go           - 172 lines (EphemeralReport CRUD) - 95% identical
cephr.go          - 172 lines (ClusterEphemeralReport CRUD) - 95% identical  
report.go         - 137 lines (Report CRUD) - 95% identical
clusterreport.go  - 137 lines (ClusterReport CRUD) - 95% identical

Impact:

• Bug fixes must be applied in 6 places
• Already has inconsistent error handling across files
• Adding new features requires changing 6 files
• High maintenance burden

Evidence of Problems:

• Migration watchers have return instead of continue (bug in all 6 types)
• Error handling differs between implementations
• Some have nil checks, others don't

---

Issue #3: Postgres should support read replica 🟠

Location: pkg/storage/db/multiDB.go:26-47

Problem:
Every query is diverged to primaryDB, however read queries must be handled via readReplica

---

Issue #4: Migration Watchers Exit After First Event 🔴

Location: pkg/server/config.go:180-227 (and 3 other watch loops)

Problem:

go func() {
    for event := range cpolrWatchInterface.ResultChan() {
        switch event.Type {
        case watch.Added:
            cpolr := event.Object.(*ClusterPolicyReport)
            if _, ok := cpolr.Annotations[ServedByAnnotation]; ok {
                return  // ← BUG: Should be 'continue'!
            }
            store.Create(ctx, cpolr)
        }
    }
}()

Impact:

• Watcher exits after processing FIRST report with annotation
• Subsequent reports are NOT synced to database
• Silent failure (no error logged)
• Data inconsistency between etcd and PostgreSQL

Fix: Change return to continue

---

Issue #5: No Pagination in List Operations 🔴

Location: All List() methods in pkg/storage/db/

Problem:

func (p *polrdb) List(ctx context.Context, namespace string) ([]*PolicyReport, error) {
    // Fetches ALL reports in namespace
    rows, err := p.MultiDB.ReadQuery(ctx, "SELECT report FROM policyreports WHERE namespace = $1")
    
    for rows.Next() {
        // Deserializes ALL reports
        json.Unmarshal([]byte(jsonb), &report)
        res = append(res, &report)
    }
    return res  // Returns ALL reports
}

Impact:

• With 10,000 reports @ 100KB each = 1GB transferred and deserialized per LIST
• Guaranteed OOM (out of memory) in large clusters
• No way to paginate through results
• API server times out on large responses

---

Issue #7: Missing Database Indexes 🟠

Current indexes:

CREATE INDEX policyreportnamespace ON policyreports(namespace)
CREATE INDEX policyreportcluster ON policyreports(clusterId)

Missing:

• Composite index on (clusterId, namespace, name) for Get queries
• No index on resource_version for Watch
• No JSONB GIN indexes for label queries

Impact:

• Get queries may do full table scans
• List queries are slow
• Can't efficiently query by labels

---

🏗️ Architecture Issues

No Separation of Concerns

• Storage layer knows about Kubernetes (should be generic)
• Manual SQL string building (error-prone)
• No query builder or repository pattern
• Metrics mixed with business logic

No Abstraction

• Can't swap storage backends (PostgreSQL, MySQL, etc.)
• Storage code duplicated for each resource type
• No generic interface

---

📚 References

• PostgreSQL best practices: https://wiki.postgresql.org/wiki/Don%27t_Do_This
• Go database/sql is thread-safe: https://pkg.go.dev/database/sql#DB
• Kubernetes API conventions: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md

[jigar-joshi-nirmata]

Proposal: v2 PostgreSQL Storage Layer Refactor

🎯 Approach

I propose a complete refactor of the PostgreSQL storage layer using modern Go patterns (generics, repository pattern) while maintaining 100% backward compatibility.

🏗️ Solution Architecture

1. Use Go Generics to Eliminate Duplication

Single generic implementation for all 6 resource types:

// One implementation works for ALL types
type PostgresRepository[T metav1.Object] struct {
    router       *DBRouter
    clusterID    string
    tableName    string
    // NO mutex!
}

// Create typed repositories
polrRepo := NewPostgresRepository[*v1alpha2.PolicyReport](...)
ephrRepo := NewPostgresRepository[*reportsv1.EphemeralReport](...)
// ... all 6 types use same code

Benefits:

• 70% code reduction (500 lines vs 969 * 6 lines)
• Bug fixes apply to all types automatically
• Type-safe at compile time

---

2. Remove All Mutexes

Database connection pools are already thread-safe:

// v1 (wrong):
func (p *polrdb) Create(...) {
    p.Lock()  // ❌ Unnecessary and harmful
    db.ExecContext(...)
    p.Unlock()
}

// v2 (correct):
func (p *PostgresRepository[T]) Create(...) {
    // No lock - sql.DB is thread-safe!
    db.ExecContext(...)
}

---

3. Implement DBRouter for Read/Write Splitting

type DBRouter struct {
    primary      *sql.DB
    readReplicas []*sql.DB
    replicaIndex atomic.Uint32  // Lock-free round-robin
}

// Reads go to replicas
db := router.GetReadDB(ctx)  // Round-robin, health-checked

// Writes go to primary
db := router.GetWriteDB()

---

4. QueryBuilder for Consistent SQL

qb := NewQueryBuilder()
qb.ApplyFilter(filter, clusterID)

// All operations use QueryBuilder
query := qb.BuildSelect(tableName)
query := qb.BuildInsert(tableName, columns, values, upsert)
query := qb.BuildUpdate(tableName, column, value)
query := qb.BuildDelete(tableName)

Benefits:

• SQL injection protection (parameterized queries)
• Consistent query generation
• Easy to add features (ORDER BY, LIMIT, etc.)

---

5. Storage-Agnostic Interface

// Generic interface that ANY backend can implement
type IRepository[T metav1.Object] interface {
    Get(ctx context.Context, filter Filter) (T, error)
    List(ctx context.Context, filter Filter) ([]T, error)
    Create(ctx context.Context, obj T) error
    Update(ctx context.Context, obj T) error
    Delete(ctx context.Context, filter Filter) error
}

// Filter struct ready for pagination, labels, etc.
type Filter struct {
    Name            string
    Namespace       string
    LabelSelector   labels.Selector  // Future
    Limit           int               // Future
    Continue        string            // Future
}

---

6. Strict Kubernetes Semantics

// Create - Fails if exists (proper K8s behavior)
func Create(...) error {
    if exists {
        return ErrAlreadyExists
    }
    // INSERT (no ON CONFLICT)
}

// Update - Fails if not exists
func Update(...) error {
    result := db.Exec("UPDATE ...")
    if rowsAffected == 0 {
        return ErrNotFound
    }
}

Benefits:

• Catches duplicate creates (bugs in watchers)
• Standard error handling
• Matches kubectl create/update behavior

---

📦 Implementation Plan

Phase 1: New Package Structure

pkg/v2/
├── config/postgres/        # Configuration
│   ├── config.go          # Main config with validation
│   ├── host.go            # Primary + replica hosts  
│   └── ssl.go             # SSL/TLS config
│
└── storage/
    ├── IRepository.go     # Generic interface
    ├── filter.go          # Query filter
    ├── errors.go          # Sentinel errors
    │
    └── postgres/
        ├── storage.go      # Entry point
        ├── repository.go   # Core struct
        ├── router.go       # Read/write routing
        ├── schema.go       # Table management
        ├── query_builder.go # SQL generation
        ├── retrieve.go     # Get/List
        ├── create.go       # Create
        ├── update.go       # Update
        └── delete.go       # Delete

Phase 2: Backward Compatibility

• Use same table schema (column name: clusterId)
• Use same JSON format (JSONB column)
• Use same indexes
• Works with existing v1 databases (no migration!)

Phase 3: Gradual Rollout

• Deploy v2 alongside v1
• Feature flag to route traffic
• Canary: 1% → 10% → 50% → 100%
• Monitor performance and errors

---

🔒 Risk Mitigation

Low Risk Because:

• ✅ Side-by-side deployment (v1 and v2 coexist)
• ✅ Same database schema (no migration)
• ✅ Feature flag rollout (gradual adoption)
• ✅ Easy rollback (just switch flag back)

Testing Strategy:

• Unit tests for QueryBuilder
• Integration tests with testcontainers
• Load tests to verify performance claims
• Canary deployment in production

---

I'm ready to implement this refactor. Please let me know if you have feedback or concerns.

[jigar-joshi-nirmata]

Hey @aerosouund
I have fixed the issues via: #304

Can you please review the PR?