Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
👀 |
| describe.anonymize(); | ||
| } | ||
| if let Some(describe) = describe { | ||
| return Ok(HandleResult::PrependProtocolMessageRewrite { |
There was a problem hiding this comment.
Q: why do we need this? I'm not too familiar with this piece of code, would be good to have a small example explaining.
| })) | ||
| } | ||
|
|
||
| fn internal_describe(&mut self, name: &str) -> Option<ProtocolMessage> { |
There was a problem hiding this comment.
Oh I see. The cross-shard Describe has different columns that direct-to-shard, so we need to rewrite it. I'm remembering now.
jkaczman
left a comment
jkaczman
left a comment
There was a problem hiding this comment.
LGTM!
It would be great if you could document certain decisions e.g. why you chose to move from an explicit A_Const node with the limit + offset to an A_Expr executing on Postgres. I think I understand the tradeoffs, but it would be great to see the author's POV when reading the code.
I would also be curious to know if the benchmark from before vs after changed at all.
|
Benchmarked release builds of main and this PR using pgbench prepared protocol against the same three shards. Each case used a 3s warmup followed by three 15s runs at 10 and 50 clients. Results were effectively neutral: direct and cross-shard AVG ranged from −0.6% to +1.0%; comparable ORDER BY was ~1% slower (expected from post-route bookkeeping). CPU was unchanged (+0.13%), with ~476 KiB higher peak RSS. |
| continue; | ||
| } | ||
|
|
||
| let projected_column = select.target_list().len() + helpers.len(); |
There was a problem hiding this comment.
I think we shouldn't rely on this column index; it gives us the parse tree entries, but not the actual output columns (considering when * is used). I wonder if it would be better to use what you do in aggregate.rs again (decoder.row_description().field_index(&helper.alias) else {). Right now, I think there are edge cases where this can accidentally delete user columns / sort on the wrong column.
There was a problem hiding this comment.
yes, I thought this for the above case handling
3 participants
Aggregate helpers, ORDER BY helpers, and LIMIT/OFFSET pushdown now run only after the route is known, so direct shard execution keeps original SQL and does not leak helper columns.
closes #1425 closes #1135