Use upb for protobufs - #349
Conversation
bca0678 to
55e13aa
Compare
The ubp library is developed as part of the main Protobuf project, to support other language bindings (e.g. Ruby), but is not officially released as a C library. However, it provides two main benefits that make it useful for our use case: (1) It allows limiting parse depth for complex protobuf structures, avoiding crashes, (2) it is substantially faster, in part due to having a built-in arena allocation mechanism. Since the upstream protobuf-c project appears to not be actively maintained (and ubp certainly is) this seems worth it from that perspective as well. The main risks are incompatible API changes done by upstream ubp (which they explicitly note the in their README), and the larger amount of code we have to vendor.
55e13aa to
2ba04e3
Compare
|
Here's a benchmark I put together (with AI help, but then I did an edit pass to try and tone down the typical AI drama). There are 2 main findings:
Setup
All three builds produce identical deparsed SQL on every payload they all accept. Timings are medians of repeated rounds, not single means. 1. Depth limit: a behavioral regression vs stock
Fails in ubp are due to hitting the depth limit. 2. C-level deparse (
|
| payload | stock | patched | upb | patched/stock | upb/patched |
|---|---|---|---|---|---|
| 168 B | 6423 ns | 1600 ns | 1574 ns | 4.01× | 1.02× |
| 297 B | 12392 ns | 2990 ns | 2794 ns | 4.14× | 1.07× |
| 2.6 KB | 112064 ns | 23103 ns | 20404 ns | 4.85× | 1.13× |
upb is 4.1–5.5× faster than stock
3. End to end from a JS parse tree
JS-side protobuf encode + the native call. The three native columns share one encoder, so
differences between them are C-side only. js is
pgsql-deparser 18.3.6, a hand-written
TypeScript reimplementation of deparseRawStmt — included because it's what we're trying to replace for correctness.
Microseconds per call, median of 7 rounds:
| case | stock | patched | upb | js | upb/patched | patched/js |
|---|---|---|---|---|---|---|
SELECT 1 |
2.0 | 1.0 | 1.1 | 0.8 | 0.99× | 1.36× |
| simple select | 8.4 | 3.4 | 3.3 | 3.2 | 1.05× | 1.07× |
| CREATE TABLE | 7.9 | 3.9 | 3.5 | 4.0 | 1.10× | 0.96× |
| join | 11.5 | 4.5 | 4.4 | 2.7 | 1.02× | 1.66× |
| CTE | 16.4 | 6.5 | 5.9 | 5.1 | 1.09× | 1.27× |
| 100-column projection | 136.9 | 43.3 | 40.3 | 69.0 | 1.07× | 0.63× |
| 50-way UNION ALL | 86.1 | 50.1 | 47.4 | 40.7 | 1.06× | 1.23× |
| ~340 KB tree | 3391 | 1054 | 965 | 1686 | 1.09× | 0.63× |
4. Memory
Peak RSS deparsing a 2.5 MB shallow parse tree (1500 independent statements — shallow so
that all three builds actually decode it):
| C-level maxrss | end-to-end RSS growth, 20 deparses | |
|---|---|---|
| stock | 57.2 MB | 86 MB |
| patched | 90.7 MB | 158 MB |
| upb | 49.6 MB | 97 MB |
| js | — | 162 MB |
upb is best at the C level but only by ~1.15× over stock, and slightly behind stock end to end (not by much).
5. What "patched" is
Two local patches, included only to answer "how much of upb's win is protobuf-c being fixable?" (aside from other issues that ubp is fixing)
-
palloc patch in Deparse: unpack the protobuf with palloc #360
-
Skip protobuf-c's post-scan field loop when it's a no-op (here and here)
Caveats
- Single machine, darwin-arm64 only. No Linux or musl numbers.
- Deparse only. We encode protobuf in JS, so
pg_query_parse_protobufis untested here; the
PR's own encode numbers are not something we can confirm. - The upb build of our Node addon has
scan()stubbed — it was our only protobuf-c consumer and
is unrelated to deparse.
|
@benasher44 Thanks for testing - glad to see the positive CPU/memory impact of upb reproduced! (if I read the results correctly) On the depth limit: That's actually one motivating factor for utilizing upb (to solve a bunch of OSS-fuzz complaints), but I don't see a problem with making that customizable. You had a notion of "suggested fix" for that above, but looks like that was edited out. Can you clarify what would be ideal for you on that end? |
|
Oh yep sorry. There was an LLM suggested fix, but IIRC it was vague. Yeah it'd be great to expose an API to change the depth limit. So I agree making it customizable would be great. I'm not 100% sure, but it sounds like protobuf-c had no depth limit? I agree it makes sense to have some depth limit… maybe set the default really high for existing users to avoid breakage |
Makes sense!
Yeah, protobuf-c did not limit the depth at all. I think we could set it to some value between 200 and 500 safely - the main challenge is not running into an out of stack error with the default setting (but I'm fairly certain 200 would be safe vs 100). |
|
I'm not sure what default limit makes the most sense, but we use a recursion limit of 1,000 when deserializing the protobuf in the Rust crate: pganalyze/pg_query.rs#17 |
|
500-1000 would be great, but as long as it could be configured, I think we'd be happy. Excited for this to land! |
TODO
Benchmark
Before:
After: