Proposals from the OneSales/CCMT2/MDMApp rollout: fetch_one/fetch_scalar/execute, pg_retrieve caching, lazy pool, readonly_transaction, ... #51

Activity

1. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   Working on this in session dd3520fe-363b-5088-ab76-5e87fad68be0 — draft PR: #52 (items 1, 2, 3, 5, 6 (opt-in), 7, 8 implemented; 4 partly, see PR).
2. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   Additions from the OneSales rollout (PR bmsuisse/OneSales on Azure DevOps, 32429):

   • PgPool options: OneSales could not replace its own pool with PgPool because PgPool exposes no configure hook (statement/idle timeouts set on each new connection), no acquire timeout, no min_size, no check override, and no application_name/TLS knobs beyond dsn_params. Proposal: pass-through of min_size, timeout, configure, check, and kwargs to the underlying psycopg pool. (The unquoted password in PgPool._dsn is a bug, filed separately.)
   • pg_* CRUD helpers build ComplexHelper(con) by hand at every call site: OneSales has four thin wrappers (pg_insert, pg_update_dict, pg_upsert_dict, pg_insert_many) that only bind a ComplexHelper. Proposal: create/cache one automatically when complex_helper is omitted (ties in with item 2, caching), or accept complex_helper="auto".
   • where_and(conditions): OneSales has ~9 hand-written " and ".join(...) optional-filter builders (e.g. task_repository.list_tasks). A small helper returning a psycopg.sql fragment (+ params) from a list of optional conditions would remove the string joining (and its lint noise).
   • paginate(query, limit, offset, order_by=): ~5 limit/offset boilerplates with a stable ordering tiebreak.
   • readonly_transaction variant: besides the context manager from item 5, OneSales would use fetch_all(..., read_only=True) for LLM-supplied SQL.
   • PostgresJsonResponse:
     • row_to_json emits NaN/Infinity as the strings "NaN"/"Infinity"; document, or offer a normalisation option.
     • The error key is fixed to error; a configurable key (or detail) would let existing frontends keep reading .detail (see item 8).
     • Behind BaseHTTPMiddleware a failure after the first byte still ends as a clean 200 with a truncated body (the closing ] is missing, so clients can detect it, but status can't); worth documenting next to the middleware caveat.
   • Usage numbers for item 1: OneSales has 62 fetch_one and 11 fetch_scalar call sites.
3. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   PR #52 is ready for review (CI green, version 0.15.0, also Fixes #53).

   Done: 1 fetch_one/fetch_scalar/execute; 2 ComplexHelper per-connection cache (+ complex_helper="auto" on the pg_* helpers); 3 pg_retrieve_many(where=, params=, order_by=, limit=); 4 (partly) set_default_pool accepts a plain callable; 5 readonly_transaction; 6 opt-in statement_timeout=at_most(s); 7 % escaping for sqlglot expressions; 8 docs (pool exhaustion, error vs detail + error_key, +00:00 vs Z, NaN/Infinity, middleware caveat); OneSales: PgPool pass-through of min_size/timeout/configure/check.

   Deferred / proposed in the PR description: lazy default pool (lifecycle question), where_and, paginate, fetch_all(read_only=True) (note: readonly_transaction is a safety net, not a sandbox, because multi-statement text can COMMIT out of it; documented), explicit complex_fields=.
4. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   Additions from the CCMT2 rollout (CCMT2 PR 32427 on Azure DevOps). Items that overlap with #52 are marked.

   • fetch_one / fetch_scalar (Proposals from the rollout (fetch_one/fetch_scalar/execute, ComplexHelper cache, readonly_transaction, ...) #52): CCMT2 has 77 fetchone() sites, 48 of them the simple single-statement form (13 in backend/api/bonus_rules.py, 8 each in bonus_customer_groups.py and bonus_admin.py).
   • Ignore unused params: {k: v for k, v in prms.items() if k in sql_p.params} is repeated at 23 sites (queries are built from sqlglot ASTs that only use some of the available params; psycopg ignores extra dict keys but sqlglot-style callers carry large shared param dicts anyway), and the same Placeholder walk backs get_sql_with_prm_list (53 uses). Proposal: SqlLoader.load_sql_with_params(topic, name) -> (sql, param_names) and/or a helper that filters a params dict to the names a query uses.
   • SQL statement splitter: split_sql (database/proc_schema.py:19, dollar-quote aware) and _split_sql_statements (tests/create_test_db.py:27, naive) are two copies of the same idea. Proposal: pgdevkit.sql_text.split_statements(sql) (dollar-quote and comment aware; pgdevkit already has sql_text.py).
   • Debug SQL: get_sql_replace_params renders a query with its params inlined (used by an endpoint's debug_sql flag). Proposal: render_with_params(query, params) (dev/test diagnostics only, never for execution) and/or PostgresJsonResponse.debug_sql().
   • params=None + %%: psycopg only un-doubles %% when params is not None; CCMT2's custom dialect doubles % always, so its callers must pass {} instead of None. Proposals from the rollout (fetch_one/fetch_scalar/execute, ComplexHelper cache, readonly_transaction, ...) #52 escapes % itself when params are given; confirm that None vs {} can't differ for string queries (normalise None to {} when the query contains %%, or document it).
   • Rendering sqlglot with a custom dialect: CCMT2 renders with its own FixedPostgres dialect and hands pgdevkit a string wrapped in SQL(...), which bypasses the exp.Command/placeholder-name guards in _render. If Proposals from the rollout (fetch_one/fetch_scalar/execute, ComplexHelper cache, readonly_transaction, ...) #52's % escaping makes the custom dialect unnecessary, say so in the docs so apps can drop theirs; otherwise offer a dialect= parameter.
5. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   MDMApp rollout, pgdevkit 0.15.0: findings.

   1. pg_retrieve / pg_retrieve_many (with a ComplexHelper) register every enum/composite of the table on the connection, which changes what other code on that pooled connection reads (bug / design gap).
   _select_list wraps complex columns in to_jsonb(...), so the result does not need any registered type, but load_all_complex_types -> _recurse_register still calls register_enum(info, con) / register_composite. That registration is global for the connection and sticks to it in the pool. Afterwards any plain SELECT of that enum type on the same connection returns psycopg's auto-numbered Enum member instead of the label:

   con = await psycopg.AsyncConnection.connect(...)
   (await (await con.execute("select 'idle'::imp.delta_compute_status")).fetchone())   # ('idle',)
   await pg_retrieve_many(con, M, {}, limit=1, complex_helper=ComplexHelper(con))      # M maps imp.file_import
   (await (await con.execute("select 'idle'::imp.delta_compute_status")).fetchone())   # (<Delta_Compute_Status.idle: 1>,)

   In MDMApp this made unrelated endpoints/tests return 3 instead of "done" depending on pool order (3 tests failed in the full akeneo_editor suite, all passing in isolation), so I could not adopt pg_retrieve* there (kept mdm_common's own, which only touches jsonb/dict columns). Suggestion: don't register types for the read path that already goes through to_jsonb (only register where a Python value must be converted for a write), or register enums with a str-valued enum (register_enum(info, con, enum=<StrEnum>)) / not at all (the default text loader already returns labels), and document that register_* is connection-global.

   2. Not generic enough to need new API, but worth a doc line: fetch_scalar/fetch_one use tuple_row/dict_row and are therefore affected by (1) too; complex_helper="auto" would widen the blast radius of (1) to every pg_* call.

   3. Still missing for the ~160 fetchone sites: nothing blocking: fetch_one/fetch_scalar/execute covered 31 sites mechanically (DELETE/INSERT ... RETURNING included, since the pool commits on exit). Sites that remain are multi-statement / transactional, or tests that count get_pg_connection checkouts (they patch the module attribute, which fetch_* via the default pool bypasses, an intentional trade-off there).

   4. readonly_transaction, at_most, PostgresJsonResponse t-strings: adopted at_most for the streamed endpoints; nothing to report.
6. aersam commented on Oct 9, 2026

   

   aersam

   on Oct 9, 2026

   ContributorAuthor

   More actions

   CCMT2 rollout (PR 32427 on pgdevkit 0.15.0), new generic gaps:

   1. bdt lint does not check execute(): an f-string passed to execute(...) (CCMT2 _write_rule_change, UPDATE ... SET {set_clause}) is not flagged, although fetch_* is. The same SQL rules should apply to execute (devtools).
   2. fetch_one/fetch_scalar cover the dict-row and single-value cases (48 sites converted). Remaining cursor blocks that need a helper: cursor.executemany and cursor.copy (bulk insert in backend/sql/postgres_schema.py), and several statements in one transaction with the cursor reused (e.g. INSERT ... ON CONFLICT followed by a conditional second insert on rowcount). A pgdevkit.db.transaction(pool) async context manager yielding con would make the get_pg_connection() + commit boilerplate unnecessary at ~90 async with get_pg_connection() as con sites.
   3. FixedPostgres (own sqlglot dialect) is still needed in CCMT2 only because stock sqlglot renders ArrayContains as CASE WHEN ... COALESCE(x = ANY(col), FALSE) instead of x = ANY(col); not pgdevkit's concern, mentioning it because _render uses the plain postgres dialect (a dialect= argument on fetch_*/PostgresJsonResponse would let such apps drop their own render step).
   4. Unused-parameter filtering ({k: v for k, v in prms.items() if k in sql_p.params}, 23 sites): fetch_all(..., ignore_extra_params=True) or SqlLoader returning the parameter names would remove it.