Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions drivers/pg/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,13 @@ func (s *Driver) RefreshKinds(ctx context.Context) error {
s.SchemaManager.kindIDsByKind = map[int16]graph.Kind{}
return s.SchemaManager.Fetch(ctx)
}

func (s *Driver) Optimize(ctx context.Context, opts ...graph.OptimizeOption) error {
config := &graph.OptimizeConfig{}
for _, opt := range opts {
opt(config)
}

// PostgreSQL VACUUM/ANALYZE implementation
return nil
}
Comment on lines +180 to +188
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

No-op Optimize silently reports success.

The method builds config from the options but discards it and returns nil without performing any work. This is fine as an intentional placeholder, but as written it advertises the StorageMaintainer capability while doing nothing, so callers cannot distinguish "optimized" from "not yet implemented." Consider marking the placeholder as a TODO so it surfaces in tracking and isn't mistaken for a completed implementation.

📝 Suggested placeholder marker
-	// PostgreSQL VACUUM/ANALYZE implementation
-	return nil
+	// TODO(BED-8353): implement PostgreSQL VACUUM/ANALYZE using config.Targets.
+	// A nil/empty config.Targets means "optimize every region" per graph.OptimizeConfig.
+	return nil

Want me to open a follow-up issue to track the VACUUM/ANALYZE implementation?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@drivers/pg/driver.go` around lines 180 - 188, The Optimize method currently
builds graph.OptimizeConfig but returns nil (no-op) which falsely signals
success; update Driver.Optimize to mark it as an explicit placeholder by adding
a TODO comment and returning a clear not-implemented error (e.g.,
errors.New("Driver.Optimize: not implemented") or a package-specific
ErrNotImplemented) instead of nil, and ensure any capability advertising
(StorageMaintainer) is removed or gated until the VACUUM/ANALYZE implementation
using graph.OptimizeConfig is completed so callers can distinguish implemented
vs placeholder behavior.

57 changes: 57 additions & 0 deletions graph/optimize.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package graph

import (
"context"
)

// TargetStorage identifies a region of graph storage that an optimization
// pass may target.
type TargetStorage int

const (
Nodes TargetStorage = iota
Relationships
)

// OptimizeConfig is the resolved configuration for a single Optimize call.
// Drivers apply every OptimizeOption to this struct before reading it.
type OptimizeConfig struct {
// Targets is the set of storage regions to optimize. A nil or empty
// slice instructs the driver to optimize every region it knows about.
Targets []TargetStorage
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: While convenient, an empty slice representing "all" supported storage targets may be misleading

}

// OptimizeOption mutates an OptimizeConfig and is applied in the order
// passed to Optimize.
type OptimizeOption func(*OptimizeConfig)

// OptimizeTargets restricts the optimization pass to the given storage
// regions. Repeated calls append targets rather than replacing them.
// Calling OptimizeTargets with no arguments does not change the resolved
// target set; if no targets are configured by the time Optimize runs, the
// driver treats that as "all regions it knows about".
func OptimizeTargets(targets ...TargetStorage) OptimizeOption {
return func(c *OptimizeConfig) {
c.Targets = append(c.Targets, targets...)
}
}

// StorageMaintainer is an optional capability implemented by drivers that
// can perform storage maintenance. Drivers that cannot perform meaningful maintenance
// must not implement this interface; a failed type assertion is the
// signal for "this driver does not support storage maintenance".
// A non-nil error returned from Optimize signals a driver-specific failure
// during a supported call.
//
// Consumers detect support with a type assertion against a graph.Database:
//
// if m, ok := db.(graph.StorageMaintainer); ok {
// err := m.Optimize(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
// ...
// }
type StorageMaintainer interface {
// Optimize runs a storage maintenance pass against the regions
// identified by opts. With no options, every region the driver
// recognizes is optimized.
Optimize(ctx context.Context, opts ...OptimizeOption) error
}
Comment on lines +52 to +57
Copy link
Copy Markdown
Member

@ddlees ddlees Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really dig that this interface features Options as closures. I think it has to potential to be really powerful.

For example, we could potentially pass driver specific options and only apply them when applicable. We'll want to feel something like that out a bit but here's the general idea:

m.Optimize(ctx,
  graph.Targets(targets),
  pg.Partitions(partitions),
  pg.SkipReindex(),
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context of having zero options is different than declaring zero storage targets.

On one hand, m.Optimize(ctx) optimizing all DAWGS storage makes perfect sense. If you're casting that from the graph db/driver, it says "optimize everything managed by this struct."

Where the expected behavior becomes murky is something like m.Optimize(OptimizeTargets(nil)). That to me could translate to, "of everything managed by this struct only optimize none of the targets."

We'll want to spend time considering how to expose targeting individual storage targets and all storage targets.

Copy link
Copy Markdown
Contributor Author

@brandonshearin brandonshearin Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this API gracefully handles the edge case you are mentioning.

The foundational rule is: when OptimizeConfig.Targets is nil, the API should treat that as optimize all StorageTargets. The specific case you are describing is actually captured by the simple table test i made in optimize_test.go (test case #2).

Screenshot 2026-06-01 at 2 51 22 PM

This test shows that calling m.Optimize(OptimizeTargets()) will evaluate to an OptimizeConfig struct with a nil Targets property, which is what I think we want.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to reframe my last comment as a question. Does my understanding of this edge case align with your idea of a good design here?

54 changes: 54 additions & 0 deletions graph/optimize_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package graph_test

import (
"testing"

"github.com/specterops/dawgs/graph"
"github.com/stretchr/testify/assert"
)

func TestOptimizeOptions(t *testing.T) {
t.Parallel()

tests := []struct {
name string
opts []graph.OptimizeOption
want []graph.TargetStorage
}{
{
name: "no options yields nil Targets",
opts: nil,
want: nil,
},
{
name: "OptimizeTargets with no args leaves Targets nil",
opts: []graph.OptimizeOption{graph.OptimizeTargets()},
want: nil,
},
{
name: "single call preserves order",
opts: []graph.OptimizeOption{graph.OptimizeTargets(graph.Nodes, graph.Relationships)},
want: []graph.TargetStorage{graph.Nodes, graph.Relationships},
},
{
name: "repeated calls accumulate",
opts: []graph.OptimizeOption{
graph.OptimizeTargets(graph.Nodes),
graph.OptimizeTargets(graph.Relationships),
},
want: []graph.TargetStorage{graph.Nodes, graph.Relationships},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

var cfg graph.OptimizeConfig
for _, o := range tc.opts {
o(&cfg)
}
assert.Equal(t, tc.want, cfg.Targets)
})
}
}
Loading