feat: postgres dialect suppport #82

mrsufgi · 2025-08-07T08:54:55Z

It was silently failing due to incorrect ID used, now, documents reuse the node list even if the document itself already exists.

mrsufgi · 2025-08-07T08:55:42Z

closes #48 ?

mrsufgi · 2025-08-10T08:18:10Z

I will clean this commit, I mistakenly used the same branch for addtional work (pg backend)

mrsufgi · 2025-08-13T12:38:10Z

backends/ent/annotations.go

 		annotations = append(annotations, &ent.Annotation{
-			DocumentID: &documentID,
-			NodeID:     &nodes[idx].ID,
-			Name:       name,
-			Value:      value,
-			IsUnique:   true,
+			NodeID:   &nodes[idx].ID,
+			Name:     name,
+			Value:    value,
+			IsUnique: true,


In Hooks, we never allowed setting both NodeID and DocumentID; they are mutually exclusive. This fixes it.

Great catch!

mrsufgi · 2025-08-13T12:57:00Z

backends/ent/backend.go

 func (backend *Backend) withTx(fns ...TxFunc) error {
+	return backend.withTxContext(backend.ctx, fns...)
+}
+
+// withTxContext creates a transaction with a specific context to avoid race conditions
+func (backend *Backend) withTxContext(ctx context.Context, fns ...TxFunc) error {
 	if backend.client == nil {
 		return fmt.Errorf("%w", errUninitializedClient)
 	}

-	tx, err := backend.client.Tx(backend.ctx)
+	// Create a NEW context for this transaction instead of modifying the shared one
+	txCtx := ctx
+	tx, err := backend.client.Tx(txCtx)
 	if err != nil {
 		return fmt.Errorf("creating transactional client: %w", err)
 	}

-	backend.ctx = ent.NewTxContext(backend.ctx, tx)
+	// Use transaction-local context instead of polluting backend.ctx
+	// This prevents concurrent transactions from interfering with each other
+	localTxCtx := ent.NewTxContext(txCtx, tx)
+	_ = localTxCtx // Mark as used for now

 	for _, fn := range fns {
 		if err := fn(tx); err != nil {


This is extremely important, as using Protobom with a real concurrent database (unlike SQLite) is crucial. Reusing the same context introduces a data leak, mainly where we set documentId on the context. There is a major race condition if "backend" is used by multiple consumers and we attempt to store in parallel. This isolates context per transaction.

mrsufgi · 2025-08-13T15:12:06Z

backends/ent/store.go

-				SetIsUnique(annotations[idx].IsUnique)
-
-			builders = append(builders, builder)
+		var (


Postgres does not support OnConflict ("any conflict"), therefore, we need to break it down into each possible conflict and use OnConflictColumns instead

mrsufgi · 2025-08-13T15:19:21Z

backends/ent/store.go

@@ -128,7 +197,7 @@ func (backend *Backend) saveDocumentTypes(docTypes []*sbom.DocumentType, opts ..
 				fn(newDocType)
 			}

-			if err := newDocType.OnConflict().Ignore().Exec(backend.ctx); err != nil && !ent.IsConstraintError(err) {
+			if err := newDocType.OnConflictColumns("type", "name", "description").Ignore().Exec(backend.ctx); err != nil && !ent.IsConstraintError(err) {


In cases where only one unique index exists, we can just use it as is.

jhoward-lm · 2025-08-15T17:27:39Z

internal/backends/ent/schema/mixin/proto_message.go

@@ -29,7 +29,6 @@ func (pm ProtoMessage[T]) Fields() []ent.Field {
 		field.Bytes(protoMessageField).
 			GoType(goType).
 			Nillable().
-			Unique().


Why remove the unique constraint here? Is there a use case for having the ability to store a raw wire format proto message twice in a table?

jhoward-lm · 2025-08-15T17:32:58Z

Makefile

+test-integration: # Run integration tests
+	@printf "Running integration tests for ${CYAN}backends/ent${RESET}...\n"
+	@go test -failfast -v -tags=integration -coverprofile=coverage.out -covermode=atomic ./backends/ent/...
+	@printf "${GREEN}DONE${RESET}\n\n"
+
+	${call coverage-report}


Use .PHONY since test-integration isn't a real file

nit: ensure file contains a single trailing newline

Suggested change

test-integration: # Run integration tests

@printf "Running integration tests for ${CYAN}backends/ent${RESET}...\n"

@go test -failfast -v -tags=integration -coverprofile=coverage.out -covermode=atomic ./backends/ent/...

@printf "${GREEN}DONE${RESET}\n\n"

${call coverage-report}

.PHONY: test-integration

test-integration: # Run integration tests

@printf "Running integration tests for ${CYAN}backends/ent${RESET}...\n"

@go test -failfast -v -tags=integration -coverprofile=coverage.out -covermode=atomic ./backends/ent/...

@printf "${GREEN}DONE${RESET}\n\n"

${call coverage-report}

jhoward-lm · 2025-08-15T17:33:59Z

backends/ent/annotations.go

 		annotations = append(annotations, &ent.Annotation{
-			DocumentID: &documentID,
-			NodeID:     &nodes[idx].ID,
-			Name:       name,
-			Value:      value,
-			IsUnique:   true,
+			NodeID:   &nodes[idx].ID,
+			Name:     name,
+			Value:    value,
+			IsUnique: true,


Great catch!

jhoward-lm · 2025-08-15T17:40:00Z

internal/backends/ent/schema/annotation.go

+		field.String("value_key").
+			Optional().
+			Immutable().
+			StorageKey("value_key"),


Do you mind explaining the purpose of this new field? I forget the syntax, but you can throw an ent annotation comment on there if you like

jhoward-lm · 2025-08-15T17:41:40Z

internal/backends/ent/schema/annotation.go

+			if u, ok := mutation.IsUnique(); ok && u {
+				mutation.SetValueKey("")
+			} else if v, ok := mutation.Value(); ok {
+				mutation.SetValueKey(v)
+			}
+


nit

Suggested change

if u, ok := mutation.IsUnique(); ok && u {

mutation.SetValueKey("")

} else if v, ok := mutation.Value(); ok {

mutation.SetValueKey(v)

}

if unique, ok := mutation.IsUnique(); ok && unique {

mutation.SetValueKey("")

} else if valueKey, ok := mutation.Value(); ok {

mutation.SetValueKey(valueKey)

}

jhoward-lm · 2025-08-15T17:44:15Z

backends/ent/annotations_test.go

@@ -634,7 +641,7 @@ func (as *annotationsSuite) getTestResult(annotationName string) ent.Annotations

 	result, err := as.Backend.Ent().QueryContext(
 		as.Context(),
-		"SELECT * FROM annotations WHERE name == ?",
+		"SELECT id, document_id, node_id, name, value, is_unique, value_key FROM annotations WHERE name = ?",


Changing == to = is still valid for SQLite right? Just making sure it was verified

jhoward-lm · 2025-08-15T17:46:36Z

backends/ent/backend.go

 	sqlite "github.com/glebarez/go-sqlite"
+	_ "github.com/lib/pq" // PostgreSQL driver


This is probably my main concern. Downstream code might not want the extra dependencies if only one specific database provider is desired.

Is there a way we can split into multiple go modules or something like that to keep the dependencies discrete?

jhoward-lm · 2025-08-15T17:48:32Z

backends/ent/backend.go

 	if backend.client == nil {
 		return fmt.Errorf("%w", errUninitializedClient)
 	}

-	tx, err := backend.client.Tx(backend.ctx)
+	// Create a NEW context for this transaction instead of modifying the shared one


nit

Suggested change

// Create a NEW context for this transaction instead of modifying the shared one

// Create a new context for this transaction instead of modifying the shared one.

jhoward-lm · 2025-08-15T18:02:14Z

backends/ent/backend.go

-	backend.ctx = ent.NewTxContext(backend.ctx, tx)
+	// Use transaction-local context instead of polluting backend.ctx
+	// This prevents concurrent transactions from interfering with each other
+	localTxCtx := ent.NewTxContext(txCtx, tx)


Does this variable need to be created if unused?

jhoward-lm · 2025-08-16T02:49:37Z

It was silently failing due to incorrect ID used, now, documents reuse the node list even if the document itself already exists.

I don't understand the problem statement, can you provide some background info?

Not sure if there's overlap here, and it's been a while since I was able to work with it, but at the beginning of the year I had some work in progress that would deduplicate all redundant proto messages in the database and allow reuse of identical messages by multiple edges that referenced it. IIRC it's a pretty big overhaul so I didn't want to stomp on each other's work if I ever am able to get back to it

jhoward-lm

You'll also need to generate the migration and commit it, as well as atlas.sum. Should happen automatically as part of make generate-ent. The problem is your branch is main, which is where the migration files pull their generated name from, and the migration files are intended to have descriptive names.

As a hacky workaround, you might be able to just temporarily set a static value for migrationName and comment out the first chunk of main() in internal/backends/ent/migrate/main.go.

func main() {
	// // Get name of current git branch.
	// output, err := exec.Command("git", "branch", "--show-current").Output()
	// if err != nil {
	// 	log.Fatal("failed getting current Git branch")
	// }

	// migrationName := string(output)

	// // Strip leading non-alpha characters, any set of characters ending with a slash, and trailing newline.
	// migrationName = regexp.MustCompile(`^([^A-Za-z]|[^\/]+?\/)+|\n$`).ReplaceAllString(migrationName, "")

	// // Replace non-alphanumeric characters with underscore.
	// migrationName = regexp.MustCompile(`[^\w]`).ReplaceAllString(migrationName, "_")

	migrationName := "postgres_dialect_support"

	// Register the SQLite driver as "sqlite3".
	if !slices.Contains(sql.Drivers(), "sqlite3") {
		sqlite.RegisterAsSQLITE3()
	}

	// ...
}

Although that alerted me to another significant issue. Currently migrations are only generated for sqlite3 as shown on the very next part after the static migration name. I'm not sure this will work without a major overhaul; the migrations are pretty critical

mrsufgi added 4 commits August 6, 2025 11:33

fix-nodelist-not-created

528bcbb

fix: duplicate list

0a19c18

fix: nit

ee66c35

fix: nit

cff3399

mrsufgi added 2 commits August 8, 2025 16:23

feat(backends): add pg driver

aab9030

fix: pg support in migrations

ae33034

mrsufgi added 4 commits August 11, 2025 12:43

fix: pg conflicts

b627e0b

fix: annotations in pg

acfecf2

fix: dont set documentid on annotation

540272e

fix: concurrent pg

3a72308

mrsufgi commented Aug 13, 2025

View reviewed changes

mrsufgi changed the title ~~fix: missing SetID call cause issues on duplocate NodeList~~ feat: postgres dialect suppport Aug 13, 2025

mrsufgi commented Aug 13, 2025

View reviewed changes

test: pg integration tests

903cca0

mrsufgi commented Aug 13, 2025

View reviewed changes

test: integration tests and cleanups

9936406

jhoward-lm reviewed Aug 15, 2025

View reviewed changes

jhoward-lm requested changes Aug 16, 2025

View reviewed changes

		sqlite "github.com/glebarez/go-sqlite"
		_ "github.com/lib/pq" // PostgreSQL driver

	// Create a NEW context for this transaction instead of modifying the shared one
	// Create a new context for this transaction instead of modifying the shared one.

feat: postgres dialect suppport #82

Are you sure you want to change the base?

feat: postgres dialect suppport #82

Uh oh!

Conversation

mrsufgi commented Aug 7, 2025

Uh oh!

mrsufgi commented Aug 7, 2025

Uh oh!

mrsufgi commented Aug 10, 2025

Uh oh!

mrsufgi Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhoward-lm commented Aug 16, 2025

Uh oh!

jhoward-lm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrsufgi Aug 13, 2025 •

edited

Loading