review: one text per usage and content query, rendered per dialect - #51
Merged
Conversation
ledger_usage, usage_by_user, usage_series, security_events, content_list_user and the rollup INSERT were spelled once per backend and differed only in four dialect tokens: the BIGINT cast on SUM, the ::text cast on a nullable parameter, GREATEST versus MAX and ''::text. Each query is now one macro that takes the dialect; the methods move into the shared impl body. usage_rollup_advance keeps its two bodies (Postgres takes an advisory lock) and shares only the INSERT text. pg_numbered keeps a numbered ?N as $N so the shared text can repeat a parameter and bind out of order; a bare ? still counts up. A test renders every shared query for both dialects and compares it, whitespace-normalised, with the text each backend carried before.
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.
What
crates/state/src/store.rs: the sixStoremethods that were written once for SQLite and once for Postgres (ledger_usage,usage_by_user,usage_series,security_events,content_list_user, and the INSERT ofusage_rollup_advance) now share one query text each. The two copies differed only in four dialect tokens —::BIGINTonSUM,::texton a nullable parameter,GREATESTvsMAX,''::text— so each query is amacro_rules!taking the dialect (sql_bigint!,sql_text!,sql_greatest!,sql_opt_eq!,usage_sums!), and the methods live in the sharedsql_store_impl!body.usage_rollup_advancekeeps its two bodies because Postgres takes an advisory lock; only its INSERT text is shared.content_erase_user,file_putand the batch methods stay dialect-bound (they differ in statements, not tokens).Net −81 production lines (+322/−403), +88 test lines.
The placeholder trap, handled
The shared text uses numbered
?Nplaceholders because two of the queries repeat?1/?2and bind?5before?4.pg_numberedused to renumber every?by position, which would have turned?1into$11; it now keeps a numbered?Nas$Nand only counts up bare?. Its unit test coversMAX(?3, ?5) AND ?4 OR ?1.Evidence that the SQL each backend runs is unchanged
shared_sql_renders_the_pre_share_text_per_dialect: renders all ten shared texts for both dialects and compares them, whitespace-normalised, with the literal text each backend carried before this change (the expected strings in the test are the pre-share literals).cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace: clean on macOS and in the pinnedrust:1.98.0linux/arm64 container (gw-state: 84 passed).GW_TEST_PG_URLagainst a freshpostgres:16-alpinecontainer —postgres_store_roundtrip,pg_rollup_and_erase_roundtrip,postgres_finalize_sees_result_committed_during_lock_wait,postgres_keystore_semantics_mirror_memory,publish_notifies_and_load_returns_latestall pass (84 passed, 0 failed); the run created databasegwtest_1788530106228119000on the server.