ateapi: Support Postgres as alternative persistence backend - #640
Conversation
f47b205 to
cddd2e0
Compare
| REFERENCES atespaces(name) ON DELETE RESTRICT, | ||
| name text NOT NULL, | ||
| version bigint NOT NULL, | ||
| status integer NOT NULL, |
There was a problem hiding this comment.
Do we really need status to be a column?
We need to define some clear rules on what promotes a field from the proto to become a column (vs just being in the opaque proto blob.
I think at very least we need atespace, name, version and UID (we don't have CAS checks for UID but we will add them soon). This is basically what's in ResourceMetadata, although I'm not sure if we need create and update time (maybe for list methods)?
There was a problem hiding this comment.
I think a reasonable rule for now is that fields become columns when needed for identity/relationships, SQL querying or ordering, or atomic concurrency checks; otherwise the proto remains authoritative.
Status is currently used as an atomic delete precondition, so I think it makes sense to keep it in a column as it keeps deletion to a single conditional SQL statement rather than needing an explicit transaction. I've added uid for future CAS checks.
There was a problem hiding this comment.
Status is used in some preconditions, but note that we will rework the mutation endpoints so that they take a closure to perform transactional read-modify-write ops 1. That means that this check will move to the caller side (which is arguably where it should have always been). That removes the need for it here.
There was a problem hiding this comment.
I see, I wasn't aware of this proposed rework. This makes sense and I've removed the status field.
| worker_namespace text NOT NULL, | ||
| worker_pool text NOT NULL, | ||
| worker_pod text NOT NULL, | ||
| ip text NOT NULL, |
There was a problem hiding this comment.
Same comment. Do we need ip as a column? It sounds like we don't? I know the store is enforcing that some fields don't get updated in valkey implementation today, but I think we can move that to the caller side.
There was a problem hiding this comment.
Agreed. IP is not queried, indexed, or used as identity, so it should stay solely in the proto
| ` | ||
|
|
||
| // applySchema idempotently creates atepg's tables. | ||
| func applySchema(ctx context.Context, pool *pgxpool.Pool) error { |
There was a problem hiding this comment.
Not for this PR, but we'll need to think how we will maintain / evolve the schema (versioning, rollout machinery, etc).
e9e6303 to
e070b11
Compare
| CREATE TABLE IF NOT EXISTS actors ( | ||
| atespace text NOT NULL | ||
| REFERENCES atespaces(name) ON DELETE RESTRICT, | ||
| name text NOT NULL, | ||
| uid text NOT NULL UNIQUE, | ||
| version bigint NOT NULL, | ||
| proto bytea NOT NULL, | ||
| PRIMARY KEY (atespace, name) | ||
| ); |
There was a problem hiding this comment.
This table keeps status and the template inside proto, so counting actors by either means decoding every row. The comment above says fields get projected into columns when Postgres needs them for queries, which seems like what I'd need in #796 where we need to count by status and template, and a projected column plus an index sounds a lot better than the maintained counters we'd have otherwise.
Not necessarily asking you to add it here. But since migrations are explicitly deferred, is adding a projected column later cheap, or does backfilling it out of the proto for an existing table make it painful enough that it's worth getting this right now?
There was a problem hiding this comment.
I agree these fields may become necessary, but I think they’re better added as part of #796 or a follow-up. This PR is intended to establish only the initial schema, and we expect many more further schema changes. During this phase, users may need to recreate their database after incompatible changes, so backfilling existing data should not yet be a concern.
78daa77 to
7971e79
Compare
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
…h backends Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
…om schema Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
…gent-substrate#854 Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
7971e79 to
1fc91ce
Compare
Eitan Yarmush (EItanya)
left a comment
There was a problem hiding this comment.
one small thing we should fix in a follow-up: malformed or mismatched tokens currently bubble up as gRPC Unknown; they should return InvalidArgument.
4b12ce6
into
agent-substrate:main
Adds Postgres as an alternative persistence backend for ateapi as proposed in #731. Currently this is opt-in with flags in ateapi or use the
--store-backend=postgresoption in the install script.Slack discussion: https://cloud-native.slack.com/archives/C0B6M3E2J3D/p1785183769252679
Performance benchmark results for Postgres vs Redis: https://docs.google.com/document/d/12-ko_BFHcBo_nJkx9f4B7zMbiiWKC2saGhMhZG3aQ-s/edit?usp=sharing
Benchmarks are not included in this PR, but you can reproduce the results by following the doc above
Limitations and questions:
testcontainersfor testing Postgres, but as a result it adds a lot of dependencies