-
Notifications
You must be signed in to change notification settings - Fork 491
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
PostgreSQL state store v2 #3250
Conversation
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
…B too Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
/ok-to-test |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…o postgres-v2 Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
/ok-to-test |
Complete Build MatrixThe build status is currently not updated here. Please visit the action run below directly. Commit ref: 9586773 |
Components certification testCommit ref: 9586773 ✅ All certification tests passedAll tests have reported a successful status |
Components conformance testCommit ref: 9586773 ✅ All conformance tests passedAll tests have reported a successful status |
// This is the only way to also ensure we are not running multiple "CREATE TABLE IF NOT EXISTS" at the exact same time | ||
// See: https://www.postgresql.org/message-id/CA+TgmoZAdYVtwBfp1FL2sMZbiHCWT4UPrzRLNnX1Nb30Ku3-gg@mail.gmail.com | ||
const lockID = 42 | ||
// Ensure the metadata table exists |
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.
Reviewers: I had to change how we lock before migrations because advisory locks aren't available in CockroachDB. This is functionally equivalent (and the certification tests are validating it), but because we call EnsureMetadataTable regardless of the lock, that operation may fail and may need to be retried automatically (see implementation for EnsureMetadataTable)
Fix the conflict please :) Also just to confirm, v2 components don't have query API right? |
…o postgres-v2 Signed-off-by: ItalyPaleAle <[email protected]>
Done!
Correct, they do not. |
Signed-off-by: ItalyPaleAle <[email protected]>
Does this mean there is no convenient way to inspect the value of what has been stored against a key using something like PgAdmin? I mean, I get it, and I get the reasoning, but this is a -1 for convenience. My developers have often relied on inspecting the state directly when getting to grips with Dapr, and still do dive into the data layer at times when debugging. Sounds like this would not be possible on V2? |
Still can, it shouldn't be significantly more inconvenient. In some cases it may even be more convenient. First, one thing to keep in mind, is that Dapr is a bit of a mess in the way it's passing data to the state stores.
In v1 of the component, data from gRPC would be interpreted as "binary" and stored in a JSONB column base64-encoded. So if you sent In v2 everything is stored as-is in a BYTEA column. If the data is a pgAdmin will show the column as binary by default, but assuming you know the value is UTF-8-encoded text (as it the case for JSON data), then you can do: SELECT convert_from(value, 'utf-8') FROM state |
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 mostly LGTM.
I have some small comments only.
@@ -37,33 +37,58 @@ type Migrations struct { | |||
|
|||
// Perform the required migrations | |||
func (m Migrations) Perform(ctx context.Context, migrationFns []commonsql.MigrationFn) 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.
So, is this migration meant to migrate data from some existing db to postgres?
If it is doing that kind of migration, so should we use something like https://github.com/golang-migrate/migrate, instead of devising our locking strategy?
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, this is for schema migration. They are uncommon and normally happen on major Dapr updates only. We were already using it, just had to make some changes to support non-Postgres databases.
} | ||
|
||
// Init sets up Postgres connection and performs migrations | ||
func (p *PostgreSQL) Init(ctx context.Context, meta state.Metadata) 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.
Can we include somewhere here (apart from the docs) - the diff with v1 and what are the benefits of using v2 - just, so that a developer can upfront see the reason for a v2 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.
Done, in the comment on the NewPostgreSQLStateStore method
Signed-off-by: ItalyPaleAle <[email protected]>
Fixes #2956
This PR introduces the v2 of the PostgreSQL state store component, which is meant to live side-by-side with v1. The implementation is very different and the data stored in the state store will not be compatible. However, to use the "v2" of the state store, users will need to explicitly opt into that, so existing apps will not break.
The main changes compared to v1 are:
The new component has conformance and certification tests enabled.
PS: In both "v2" and "v1", there are some aliases for metadata properties:
cleanupInterval = cleanupIntervalInSeconds
(and it accepts a Go duration),timeout = timeoutInSeconds
(and it accepts a Go duration),connectionString = url