-
Notifications
You must be signed in to change notification settings - Fork 366
feat: Initial implementation of the PostgresFDW #2806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (74.53%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2806 +/- ##
==========================================
- Coverage 75.78% 74.53% -1.24%
==========================================
Files 460 483 +23
Lines 55026 56691 +1665
==========================================
+ Hits 41694 42251 +557
- Misses 10456 11490 +1034
- Partials 2876 2950 +74 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
docs/spicedb.md
Outdated
| - [spicedb datastore](#reference-spicedb-datastore) - datastore operations | ||
| - [spicedb lsp](#reference-spicedb-lsp) - serve language server protocol | ||
| - [spicedb man](#reference-spicedb-man) - Generate man page | ||
| - [spicedb postgres-fdw](#reference-spicedb-postgres-fdw) - serve a Postgres Foreign Data Wrapper for SpiceDB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - [spicedb postgres-fdw](#reference-spicedb-postgres-fdw) - serve a Postgres Foreign Data Wrapper for SpiceDB | |
| - [spicedb postgres-fdw](#reference-spicedb-postgres-fdw) - serve a Postgres Foreign Data Wrapper for SpiceDB |
whitespace change
| Serves a Postgres-compatible interface for querying SpiceDB data using foreign data wrappers | ||
|
|
||
| ``` | ||
| spicedb postgres-fdw [flags] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: does this mean the postgres-fdw is now a part of the SpiceDB binary? When do we start to get concerned about the size of the binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and the plan is any SpiceDB-adjacent systems will be as well.
I think, for now, we won't be worrying about it
internal/fdw/common/errors_test.go
Outdated
| // Verify error is properly wrapped with psql codes | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
internal/fdw/common/errors_test.go
Outdated
| // Verify error is properly wrapped with psql codes | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
internal/fdw/common/errors_test.go
Outdated
| // Verify error is properly wrapped with psql codes | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
|
||
| // slog.SetLogLoggerLevel(slog.LevelDebug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // slog.SetLogLoggerLevel(slog.LevelDebug) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment
| SELECT resource_type, resource_id, relation, subject_type, subject_id | ||
| FROM relationships | ||
| WHERE resource_type = 'document' | ||
| AND resource_id = 'readme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is a "now" issue, but is there a way that we can help ensure that we don't have index misses on ReadRels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No; We do not have full coverage on all shapes of read rels that are possible (and likely never will). The best we could do is warn, but that's unrelated to these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. My concern is that this implementation detail is kinda hidden, and that it might be relatively easy to write a postgres query that accidentally misses the indices and then it leaves the user wondering why SpiceDB is slow. I'm okay with saying that's a future problem, though.
| ```sql | ||
| -- Add a new relationship | ||
| INSERT INTO relationships (resource_type, resource_id, relation, subject_type, subject_id) | ||
| VALUES ('document', 'readme', 'viewer', 'user', 'alice'); | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I wanna make sure we clarify in external documentation if I've got it right: the transaction support here doesn't sidestep the dual-write problem, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because postgres_fdw sadly does not support two-phase commit
|
|
||
| // Run starts the Postgres wire protocol server on the specified endpoint. | ||
| // It blocks until the context is cancelled or an error occurs. | ||
| func (p *PgBackend) Run(ctx context.Context, endpoint string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be in this PR, but I'd wanna see observability and metrics for this as a fast follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
| serverReady := make(chan bool) | ||
| go (func() { | ||
| serverReady <- true | ||
| _ = runnableServer.Run(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can use assert.NoError here if desired
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah; I don't like error checking after cleanup in tests. It likely is returning an error (context canceled) when its forcibly shutdown at the end of the time
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments; otherwise this is looking good to me for the first implementation.
internal/fdw/pgserver_e2e_test.go
Outdated
|
|
||
| // Verify server is ready | ||
| retryCount := 0 | ||
| for retryCount < 10 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use https://pkg.go.dev/github.com/stretchr/testify/require#EventuallyWithT for this if desired
| prevEnv := os.Environ() | ||
| restore := func() { | ||
| restoreEnv(prevEnv) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary with t.Setenv?
|
|
||
| var trailerMD metadata.MD | ||
|
|
||
| checkResult, err := client.CheckPermission(ctx, &v1.CheckPermissionRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the caveat context also a todo here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it has TODOs saying so
| ObjectType: valuesByColumnName["subject_type"], | ||
| ObjectId: valuesByColumnName["subject_id"], | ||
| }, | ||
| OptionalRelation: valuesByColumnName["optional_subject_relation"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the optional subject relation has to be defined, or is the assumption that it'll typically come through as an empty string and be handled as an unset value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string
| return nil | ||
| } | ||
|
|
||
| type valueOrRef struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring on this one? Also maybe pull it towards the top or into a separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Its used internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just cuz it's used widely enough in this package. I ended up inferring it from usage as I was reading and then finding it in one of the last files I looked at and thinking that it would have been nicer to have it up front. I guess that's a review-time concern, though, so it's probably fine.
| t.Parallel() | ||
|
|
||
| require.Equal(t, tc.expectedName, tc.stmt.TableName()) | ||
| require.Equal(t, tc.stmt.isUnsupported, tc.stmt.IsUnsupported()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this asserting? Is it functionally asserting that the interface is satisfied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its just making sure the accessors return what was defined
281f677 to
70a611b
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments, otherwise LGTM
| // Give PGServer time to start | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a condition that we can explicitly poll here? I don't want this to become a flake source.
70a611b to
94e6043
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
c53bb4b to
c3fef24
Compare
c3fef24 to
4260757
Compare
6ddcc43 to
17d5534
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm curious to see what the ramifications of CGO are.
The PostgresFDW allows SpiceDB to be used from within Postgres via the Postgres Foreign Data Wrapper protocol, as-if Postgres was speaking to another Postgres. It supports querying (Check, LookupResources, LookupSubjects), as well as reads (RealRelationships), writes (WriteRelationships) and schema updates (ReadSchema/WriteSchema). These operations are performed against "virtual" tables (`permissions`, `relationships` and `schema`) that are placed into the Postgres and translated by this proxy. For more information, see the README.md This is currently experimental and subject to changes
17d5534 to
1d80dac
Compare
The PostgresFDW allows SpiceDB to be used from within Postgres via the Postgres Foreign Data Wrapper protocol, as-if Postgres was speaking to another Postgres.
It supports querying (Check, LookupResources, LookupSubjects), as well as reads (RealRelationships), writes (WriteRelationships) and schema updates (ReadSchema/WriteSchema).
These operations are performed against "virtual" tables (
permissions,relationshipsandschema) that are placed into the Postgres and translated by this proxy.For more information, see the README.md
This is currently experimental and subject to changes