Skip to content

Conversation

@mistahj67
Copy link
Contributor

@mistahj67 mistahj67 commented Dec 30, 2025

Description

Plumb memory limit config support for the postgres driver

Motivation and Context

Currently pg driver hardcodes the memory limit for queries to 1Gb

Resolves BED-7082

How Has This Been Tested?

Updated build.config.json on BHE side and verified the limit was reflected in dawgs queries

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Summary by CodeRabbit

  • Improvements
    • Introduced configurable graph query memory limits during driver initialization, enabling dynamic resource allocation instead of using a previously fixed default value.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Walkthrough

The changes introduce a configurable graph query memory limit parameter that propagates through the PostgreSQL driver initialization stack, replacing a previously hardcoded constant with a dynamically configurable value throughout the driver hierarchy.

Changes

Cohort / File(s) Summary
Constructor and initialization updates
drivers/pg/pg.go, drivers/pg/driver.go, drivers/pg/manager.go
Updated NewDriver() signature to accept graphQueryMemoryLimit parameter; propagates limit to NewSchemaManager(); NewSchemaManager() now stores the limit as a public field; initialization call in pg.go passes cfg.GraphQueryMemoryLimit to NewDriver(). Added import of util/size package.
Memory limit access
drivers/pg/transaction.go
GraphQueryMemoryLimit() method changed to return the stored schemaManager.graphQueryMemoryLimit value instead of a hardcoded constant.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Through the driver stack we hop with cheer,
With memory limits configured clear,
From config cascade down each layer,
Dynamic limits everywhere!
No hardcoded constants here! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding configurable memory control to the PostgreSQL driver instead of hardcoding it, which directly aligns with the changeset modifications across driver.go, manager.go, pg.go, and transaction.go files.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22c000d and af5fd34.

📒 Files selected for processing (4)
  • drivers/pg/driver.go
  • drivers/pg/manager.go
  • drivers/pg/pg.go
  • drivers/pg/transaction.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-27T15:32:33.244Z
Learnt from: zinic
Repo: SpecterOps/DAWGS PR: 4
File: drivers/neo4j/v2/schema.go:136-140
Timestamp: 2025-06-27T15:32:33.244Z
Learning: The Neo4j Go driver v5 may not have the same transaction rollback error handling patterns as suggested. Check the specific v5 documentation before recommending `errors.Is(err, neo4j.ErrTransactionTerminated)` or similar patterns.

Applied to files:

  • drivers/pg/transaction.go
🧬 Code graph analysis (2)
drivers/pg/pg.go (1)
drivers/pg/driver.go (1)
  • NewDriver (44-49)
drivers/pg/driver.go (1)
drivers/pg/manager.go (2)
  • SchemaManager (35-44)
  • NewSchemaManager (46-56)
🔇 Additional comments (7)
drivers/pg/transaction.go (1)

89-91: LGTM! Memory limit is now dynamically configurable.

The change correctly retrieves the memory limit from the schema manager's field, replacing the previous hardcoded constant. This aligns with the PR objective of making query memory limits configurable.

drivers/pg/manager.go (3)

15-15: LGTM! Import added for memory size configuration.

The util/size import is necessary for the new graphQueryMemoryLimit parameter.


36-44: LGTM! Field addition is well-integrated.

The graphQueryMemoryLimit field is appropriately added to the struct. Since it's immutable after construction, no additional locking is required beyond the existing sync.RWMutex used for other mutable fields.


46-56: Constructor validation is not needed for this parameter.

The graphQueryMemoryLimit parameter doesn't require validation in the constructor. The size.Size type is defined as an unsigned integer (uintptr), which naturally prevents negative values. Zero is an intentional and valid value meaning "no memory limit" (seen throughout the codebase with if tx.GraphQueryMemoryLimit() > 0 checks). Any positive value is acceptable, and the limit is enforced at runtime during query execution where it matters most.

drivers/pg/driver.go (2)

10-10: LGTM! Import added for memory size type.

The util/size import is required for the new graphQueryMemoryLimit parameter.


44-49: LGTM! Constructor correctly propagates memory limit configuration.

The updated constructor signature and implementation properly thread the graphQueryMemoryLimit parameter through to NewSchemaManager. The change maintains clean separation of concerns by having the SchemaManager own the memory limit value.

drivers/pg/pg.go (1)

84-88: Field exists with sensible default.

dawgs.Config includes the GraphQueryMemoryLimit field (line 25 of dawgs.go) with the type size.Size. Go's zero-value initialization ensures a default of 0 when not specified, which disables memory limiting by default. This is appropriate for backwards compatibility, and the codebase correctly checks if tx.GraphQueryMemoryLimit() > 0 before enforcing limits.


Comment @coderabbitai help to get the list of available commands and usage tips.

@mistahj67 mistahj67 changed the title feat(pg): add cfg memory control to driver feat(pg): add cfg memory control to driver BED-7082 Jan 5, 2026
@mistahj67 mistahj67 marked this pull request as ready for review January 5, 2026 18:21
Copy link

@StephenHinck StephenHinck left a comment

Choose a reason for hiding this comment

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

Confirmed that this works fantastically!

@mistahj67 mistahj67 merged commit 5e44343 into main Jan 5, 2026
1 check passed
@mistahj67 mistahj67 deleted the plumb-memory-cfg-to-pg branch January 5, 2026 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants