Skip to content

feat: Batch Postgres online reads across feature views - #6832

Open
DKilkenny wants to merge 8 commits into
feast-dev:masterfrom
DKilkenny:feat/postgres-batched-online-read
Open

DKilkenny wants to merge 8 commits into
feast-dev:masterfrom
DKilkenny:feat/postgres-batched-online-read

Conversation

@DKilkenny

Copy link
Copy Markdown

What this PR does / why we need it:

The generic OnlineStore._read_features_per_fv loop issues one online_read per feature view, so a request spanning N feature views costs N round trips. Redis already overrides that seam to batch; Postgres now does too, which is the follow-up described in #3259.

Postgres keeps each feature view in its own table, so the per-view reads can go out as one UNION ALL. Each branch carries a constant tag column and the combined result is split back apart by it. The tag is doing real work here: the table name is not in the result set, and two feature views can return both the same entity key and the same feature name. Both the sync and async seams are overridden.

When this helps, and when it does not. Batching trades round trips for holding more rows at once, so it only pays where round trips dominate. Measured against Postgres 16 with a 2 ms delay injected per execute to stand in for a remote database:

shape batched generic speedup
10 views, 1 entity 6.1 ms, 1 query 34.3 ms, 10 queries 5.6x
10 views, 10 entities 13.4 ms, 1 query 41.1 ms, 10 queries 3.1x
25 views, 10 entities 27.6 ms, 1 query 108.7 ms, 25 queries 3.9x
10 views, 5000 entities 4417 ms 4357 ms 0.99x

At 5000 entities there is no saving left, and holding every view's rows before emitting any of them took peak memory to 85 MB against a flat 9.6 MB for the generic path. That is the wrong trade for a concurrent feature server, so requests above MAX_BATCHED_READ_KEYS entity keys fall through to the generic path, which processes and releases one view at a time. Memory at that size is now identical to before. The check reads the request shape before any keys are serialized, so declining to batch costs nothing.

That 2048 threshold is my number from the measurements above, taken on a laptop against a container. Happy to move it, make it configurable, or drop it entirely if you would rather this always batch.

Two things worth flagging. Postgres does not advertise async_supported, so _read_features_per_fv_async is not reachable from the feature server today; it is implemented and verified against a real database so that it behaves whenever that changes. And the docs diff is slightly wider than the feature, because four lines described Redis and Postgres as overriding get_online_features when both override _read_features_per_fv. Three of those were already there, and correcting them seemed better than adding a fifth alongside.

No changes to online_store.py, so the base class contract is untouched.

Which issue(s) this PR fixes:

Refs #3259

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

10 unit tests and 6 integration tests. The integration tests run the same request through the batched path and through OnlineStore._read_features_per_fv and assert the protobuf responses are identical, including the case where an entity exists in one view but not another.

I mutation tested these rather than trusting a green run. Ignoring the tag, executing the query twice, cross wiring one view's entity index mapping onto another, and disabling the size guard each fail exactly the tests meant to catch them. Two of those slipped past an earlier version of the tests, which is why the demux tests give both feature views the same feature name: with different names a crossed bucket still resolves the right value and the test proves nothing.

The tests/unit/infra suite is unchanged at 41 failed / 279 passed before and 41 failed / 289 passed after, same pre-existing failures plus the new tests. Those failures and the 27 collection errors are missing optional extras in my environment, not related to this change. ruff and mypy are clean.

The numbers in the table above come from a manual benchmark against a Postgres 16 testcontainer, counting executes and tracemalloc peak across view and entity counts with and without an injected per-execute delay. It is a throwaway script rather than part of the suite; happy to clean it up into sdk/python/tests/benchmarks/ if that is useful to keep.

One caveat on coverage: I ran all of this on Python 3.11. There is no py3.13 requirements file and the pinned pytest-timeout does not import on 3.12 or later, so 3.10 and 3.12 are unverified on my side and CI will be the first run against them.

Misc

Release note: PostgreSQL online store now reads all feature views in a single query, reducing round trips for requests that span multiple feature views.

@DKilkenny
DKilkenny requested a review from a team as a code owner September 12, 2026 04:17
@codecov-commenter

codecov-commenter commented Sep 14, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 48.15%. Comparing base (dd9f099) to head (4efa7da).
⚠️ Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
...ra/online_stores/postgres_online_store/postgres.py 98.66% 1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6832      +/-   ##
==========================================
+ Coverage   48.04%   48.15%   +0.10%     
==========================================
  Files         427      427              
  Lines       53591    53665      +74     
  Branches     7800     7809       +9     
==========================================
+ Hits        25749    25843      +94     
+ Misses      25986    25966      -20     
  Partials     1856     1856              
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.49% <98.66%> (+0.11%) ⬆️
Files with missing lines Coverage Δ
...ra/online_stores/postgres_online_store/postgres.py 33.18% <98.66%> (+16.65%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 023614b...4efa7da. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@DKilkenny
DKilkenny force-pushed the feat/postgres-batched-online-read branch from eba56b7 to a5f767c Compare September 14, 2026 15:24
@DKilkenny

Copy link
Copy Markdown
Author

Rebased to pick up the CI x86_64 fixes in #6835

@ntkathole
ntkathole force-pushed the feat/postgres-batched-online-read branch from a5f767c to f13e46a Compare September 15, 2026 12:22
The generic OnlineStore._read_features_per_fv loop issues one online_read per
feature view, so a request spanning N feature views costs N round trips. Redis
already overrides the seam to batch; Postgres now does too.

Each feature view has its own table, so the per-view reads are combined with
UNION ALL and split apart again by a constant tag column. The tag is what keeps
them separable: the table name is not in the result set, and two feature views
can legitimately return the same entity key AND the same feature name.

Batching only pays where round trips dominate. Measured against Postgres 16 with
a 2ms round trip, a 10-view request is ~5x faster at 1 entity and ~3x at 10, but
a wash at 5000 entities, where holding every view's rows at once costs tens of MB
per in-flight request. Requests above MAX_BATCHED_READ_KEYS entity keys therefore
fall back to the generic per-view path, which processes and releases one view at
a time. The threshold is checked from the request shape before any keys are
serialized, so choosing not to batch costs nothing.

Both the sync and async paths are overridden. The generic async path already runs
the per-view queries concurrently via asyncio.gather, so the async win is N
concurrent queries becoming one rather than a change from serial. Note that
Postgres does not advertise async_supported today, so the async path is not yet
reachable from the feature server.

Also corrects four doc references that described Redis and Postgres as overriding
get_online_features; both override _read_features_per_fv.

Refs feast-dev#3259

Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
@ntkathole
ntkathole force-pushed the feat/postgres-batched-online-read branch from f13e46a to 4b02e22 Compare September 16, 2026 14:45
Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
Fold the execute-count check into the matches-generic test, and drop two tests the unit suite already covers: the over-threshold fallback (it compares the generic path with itself) and the different-entities case.

Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
@DKilkenny

Copy link
Copy Markdown
Author

@ntkathole just wanted to follow up on your offer to review this. Happy to make any changes you need

# wash at 5000, where the combined result costs tens of MB per in-flight request.
# Past the threshold the generic one-query-per-view path is used instead, which
# processes and releases a single view at a time.
MAX_BATCHED_READ_KEYS = 2048

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it be MAX_BATCHED_RESULT_ROWS instead?

# wash at 5000, where the combined result costs tens of MB per in-flight request.
# Past the threshold the generic one-query-per-view path is used instead, which
# processes and releases a single view at a time.
MAX_BATCHED_READ_KEYS = 2048

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please consider making it configurable

in its own table here, so the per-view reads can be combined with UNION ALL
and split apart again afterwards.
"""
if self._too_large_to_batch(grouped_refs, join_key_values):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, it make sense to keep original flow when len(grouped_refs) < 2, this way batching only activates when there's a multi-view request.

Address review feedback: rename MAX_BATCHED_READ_KEYS to MAX_BATCHED_RESULT_ROWS, expose it as the max_batched_result_rows online store setting (0 disables batching), and keep the generic path for single-view requests. Cover the gate on both the sync and async paths, and check that the fallback reads each view.

Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
@DKilkenny

Copy link
Copy Markdown
Author

Thanks for the review here. Those changes made sense so they're updated. I wasn't sure if you'd want it configurable, but I made the change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants