feat(postgres): add stock-PostgreSQL storage backend (KPEP-0001)#5
Open
cc-ocelotai wants to merge 1 commit into
Open
feat(postgres): add stock-PostgreSQL storage backend (KPEP-0001)#5cc-ocelotai wants to merge 1 commit into
cc-ocelotai wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a PostgreSQL implementation of
storage.Interfacebehind--storage-backend=postgres, alongside the existing etcd3 and Spanner backends. It targets stock PostgreSQL, no extensions, no superuser, no replication slots, no server configuration changes, so it runs unmodified on a managed instance (RDS, Cloud SQL, Neon) or a laptopdocker run postgres.Registration is the one-line change KPEP-0001 promised: a single
b.Register(postgres.NewOptions())inbackends/register.go. Nothing outsidebackends/postgres/changes, and the apiserver needs no code change to select it.It passes the full applicable upstream conformance battery (23 cases, 0 failures, 1 self-skip, the compaction case, same as Spanner/Cockroach) and is verified against a live PostgreSQL 16.
Motivation
KPEP-0001 establishes the pluggable-backend registry and argues we'll want "at least three backends (Spanner, kine -> postgres for ergonomics-first deployments, postgres-native for perf-sensitive ones)." This is that third one, built as a direct
storage.Interfaceimplementation rather than a kine shim.Why not just use kine-over-Postgres? The
kplane-kinebenchmarks already in the KPEP make the case: 1.5–3× lower throughput, 2–14× wider p99, and a 1-second watch floor from the polling driver. Postgres is the most widely operated database on earth; a first-class backend that talks to it directly, with etcd-class watch latency and no operational prerequisites, is worth the implementation cost. Spanner proved the direct-storage.Interfaceapproach; this extends it to the database most teams already run.Design, and why it deliberately diverges from the Spanner/Cockroach shape
This is the part worth reviewers' attention. The Spanner and Cockroach backends keep one row per key and lean on the storage engine for two things PostgreSQL simply does not expose:
ReadTimestampboundAS OF SYSTEM TIMECREATE CHANGEFEEDcrdbpgx.ExecuteTxSERIALIZABLE+ 40001 retryBecause Postgres has neither a commit clock nor time-travel, the single-row model can't work here. The idiomatic answer, the one the Kubernetes ecosystem already validated via kine, is an append-only MVCC log: every mutation
INSERTs a new row, and the row'sBIGSERIAL idis the resource version. That one decision discharges three obligations at once:idsequence, monotonic by construction.DISTINCT ON (name) … WHERE id <= rv ORDER BY name, id DESC. No engine time-travel required, we keep the history and query it.WHERE id > cursor), which is durably replayable across restarts because the cursor lives in the log's own ids.The remaining sub-decisions, each with its rationale:
Watch transport: poll-the-log +
LISTEN/NOTIFY, not logical replication.Logical replication would be the closest analog to Cockroach's changefeed, but it requires
wal_level=logical, replication privileges, and slot lifecycle management, and an unconsumed slot pins WAL and can fill the disk. That's the wrong default for a backend meant to run anywhere. Instead a single per-process reader polls the log;LISTEN/NOTIFY(via anAFTER INSERTtrigger) is only a sub-100ms wakeup, and a 1s timer is the backstop. The poll is authoritative, so a missed notification loses nothing. the next poll from the cursor recovers it. This also makes the feed multi-writer correct (it sees every apiserver's rows), which the in-process broadcaster approach is not.Gap-safe watermark, the one genuinely subtle piece. A
BIGSERIALsequence hands out ids at
INSERTtime, but transactions commit in an unrelated order: writer A can reserveid=5while B reservesid=6and commits first. A naive poller readingWHERE id > cursor ORDER BY idwoulddeliver 6, advance its cursor past 5, and silently drop 5 when A commits. This is the same hazard the Spanner broadcaster's ticket/heap machinery and Cockroach's resolved-timestamp both exist to close. Here it's closed the
stock-Postgres way: the feed only delivers up to a safe watermark derived from transaction visibility, the highest id all of whose predecessors have committed, computed from
txid_snapshot_xmin(txid_current_snapshot()). It is,functionally, a resolved timestamp built from xids instead of a clock. See
currentSafeRVinrv.goand the delivery loop infeed.go.Writes run
SERIALIZABLEwith an automatic 40001/40P01 retry (retry.go)— stock-Postgres's answer to
crdbpgx.ExecuteTx. The append-only model needs it for correctness, not just throughput: two racingCreates of one key must not both observe "no live row," and serializable isolation + retry is what makes the loser see the winner's row and returnKeyExists.TTL is a scanner that appends tombstones (
ttl.go). Postgres has nobuilt-in row-level TTL, so the scanner is the only expiry path, but it fits the log model cleanly: it
INSERTs a tombstone (never a physicalDELETE), the feed picks it up on the next poll, and watchers see an ordinaryDELETED. ANOT EXISTS (newer row)guard plus the serializable re-read is the CAS that keeps a concurrently-renewed lease from being wrongly deleted.name TEXT COLLATE "C". Keys are byte strings and every prefix scan(
name >= start AND name < prefixEnd(start)) assumes byte ordering. Postgres's default locale collation reorders punctuation like/, which silently breaks every prefix list.COLLATE "C"forces bytewise comparison; the(name, id DESC)index inherits it. Calling this out explicitly because it's invisible in review and only surfaces against a real server (it did, see Testing).Non-goals / deliberately deferred
Scoped out to keep this reviewable, each a clean follow-up rather than a gap:
CompactRevision()returns 0 for now (matching Spanner and Cockroach). The schema and RV math already leave the seam. A background compactor that prunes superseded rows below a horizon is the obvious next PR.wal_level=logicaland want to shed the poll.Testing
RunTest*battery fromk8s.io/apiserver/pkg/storage/testing, the same suite etcd3 itself is held to, delegated case-by-case inconformance_test.go. 23 pass, 0 fail, 1 self-skip. The skip isWatchFromZero, which self-skips when the compaction callback is nil; Spanner and Cockroach skip it identically.GetListreturned empty despite correct rows, because the DB's default collation isn't byte-ordered and broke every prefix range. Fixed withCOLLATE "C"(above). Called out because it's exactly the class of defect that passes code review and fails in production.-count=3runs; watch latency isNOTIFY-driven, not bound to the poll backstop.FILTER, thetxidsnapshot helpers,DISTINCT ON,COLLATE "C"), so PostgreSQL 12+ is the intended floor, not yet CI-verified on older majors.Compatibility & operations
wal_levelchange, no replication role, no extensions. A defaultdocker run postgres(or managed instance) works as-is.--postgres-dsn(required), plus--postgres-database,--postgres-max-conns,--postgres-max-conn-lifetime,--postgres-max-conn-idle-time,--postgres-health-check-period.LISTEN(a connection blocked inWaitForNotificationcan't be pooled); everything else uses the sharedpgxpool.How it's laid out
Mirrors the Cockroach/Spanner file structure 1:1 so it reads the same way.
~2,350 source lines + ~300 test lines.
doc.goconfig.gopgxpoolconstruction, DSN, defaultsschema.goNOTIFYtrigger, init sentinelretry.goSERIALIZABLE+ 40001/40P01 retry helperrv.goid<-> RV, the gap-safe watermark, decode helpersstore.goCreate/Get/Deletelist.goGetList,DISTINCT ONsnapshot reads, pagingupdate.goGuaranteedUpdate, optimistic concurrencyfeed.goLISTEN/NOTIFYwakeup, watermark, fanoutwatcher.gowatch.Interfaceover a feed subscriberttl.gofactory.go/factory_backend.goregister.goOptionsimplementingregistry.Backendstubs.goStats,ReadinessCheck,RequestWatchProgress, …conformance_test.go,testutil_test.goSuggested reading order for review:
doc.go->schema.go->rv.go(the watermark) ->feed.go(the watch kernel) ->store.go/update.go, then the wiring. I'm happy to split this into the same small, self-contained commits the Cockroach PR used if that's easier to walk.Test plan
docker run -d -e POSTGRES_HOST_AUTH_METHOD=trust -p 5432:5432 postgres:16)POSTGRES_TEST_DSN=postgres://postgres@localhost:5432/postgres?sslmode=disable go test ./backends/postgres/...go test ./backends/postgres/... -run TestConformance_Watch -count=3--storage-backend=postgres --postgres-dsn=…Open questions for reviewers
I'd genuinely value a second opinion on the two load-bearing calls:
LISTEN/NOTIFYover logical replication for zero-config portability and durable resume. If we expect deployments to already runwal_level=logical, an opt-in logical-replication transport could be a worthwhile follow-up, but I don't think it should be the default.txid_snapshot_xminapproach is correct and index-cheap on the recent tail, but it's the subtlest code in the PR. Extra eyes oncurrentSafeRVand thefeed.polldelivery bound are welcome.The append-only-log-over-SQL pattern is also distinct enough from the single-row engine-MVCC backends that a short design note (or a KPEP-0001 addendum) may be worth writing, so the next SQL backend, kine-native, MySQL, whatever, follows the same shape deliberately rather than by archaeology. Happy to author that if maintainers agree it's warranted.