Skip to content

feat: stream cross-shard ORDER BY results with k-way merge#1114

Open
murex971 wants to merge 5 commits into
pgdogdev:mainfrom
murex971:nagrawal-merge-sort
Open

feat: stream cross-shard ORDER BY results with k-way merge#1114
murex971 wants to merge 5 commits into
pgdogdev:mainfrom
murex971:nagrawal-merge-sort

Conversation

@murex971

@murex971 murex971 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description

  • Replace full in-memory buffering/sort for eligible cross-shard ORDER BY queries with a streaming k-way merge path.
  • Keep existing buffer/sort/aggregate path unchanged for non-ORDER BY-only cases (e.g. aggregate/distinct flows).
  • added unit/functionality tests and integration tests.

Related issues/comments

closes #207

@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.95128% with 39 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...gdog/src/backend/pool/connection/order_by_merge.rs 84.23% 32 Missing ⚠️
pgdog/src/backend/pool/connection/binding.rs 42.85% 4 Missing ⚠️
pgdog/src/backend/pool/connection/buffer.rs 94.11% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!


read = true;
if let Some(message) = state.forward(message)? {
if let Some(message) = state.forward_from(position, message)? {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: rename position to shard

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

since it's not shard_id, rather index in connected servers list, shard could be misleading.
would change this to shard_position

Comment thread pgdog/src/backend/pool/connection/buffer.rs Outdated
@levkk

levkk commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Looks good. We would need quite a few integration tests for this one though before it's in a mergeable state.

@murex971

murex971 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

We would need quite a few integration tests for this one though before it's in a mergeable state.

could you elaborate what kind of integration tests should be there ?

@levkk

levkk commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

I'm thinking, use our existing integration test setup with:

  • 2 shards
  • insert a bunch of rows in each table, at random
  • SELECT with a bunch of different ordering
  • assert that the order is always correct

Repeat this test with as many data types as you can think of; totally fine to use the same table, can just order on different columns, e.g. varchar, bigint, timestamp(tz), etc. Let me know if you hit any errors anywhere, we need to double check our decoding logic for some more complex types.

Could write them in integration/rust project if convenient.

@murex971

murex971 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@levkk
While validating the new cross-shard k-way merge path for ORDER BY, I found an existing limitation, when an ORDER BY key is not present in the projected output (e.g. SELECT id ... ORDER BY name), PgDog currently resolves sort columns from RowDescription and may silently skip unresolved named keys, leading to incorrect global ordering. This behavior existed in the previous full-buffer sort path as well; the k-way merge implementation inherits it. For this PR, tests are scoped to queries where sort keys are included in the select list to validate merge correctness. We should open a follow-up to support PostgreSQL-compatible ordering on non-projected keys (likely via hidden helper columns in shard queries and dropping them before returning results, or by returning an explicit error instead of silent fallback).

@levkk

levkk commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

@murex971 Thank you. Yes, this is a known limitation: https://docs.pgdog.dev/features/sharding/cross-shard-queries/select/#sorting. I agree, we should do a follow up and fix it; I'm thinking we use the same approach as we do for aggregates: add the columns temporarily into the query using the rewriter and remove them when sending the result back to the client.

@levkk

levkk commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

My apologies for delay on reviewing this. Will try to get through this by tomorrow or early next week. Please ping me again if you don't hear back in this timeframe.

@murex971

murex971 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

added an issue for this #1135

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Query engine] Merge/sort for sorting

2 participants