You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/14 20:58:00 UTC
[PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
mikebridge opened a new pull request, #44261:
URL: https://github.com/apache/superset/pull/44261
### SUMMARY
The activity fetch executed its streaming SELECTs via `db.session.connection().execution_options(stream_results=True)` — and `Connection.execution_options` **mutates the connection in place**, permanently flipping the request session into server-side-cursor mode. On PostgreSQL, every later statement on the same request — Continuum's `version_transaction` INSERT, SAVEPOINTs, and notably the **DBEventLogger write that follows any activity read** — then gets wrapped in `DECLARE ... CURSOR WITHOUT HOLD FOR ...` and fails with `psycopg2.errors.SyntaxError` (the event write is swallowed by the logger's error handler, so activity-endpoint telemetry is silently lost on PG deployments today). Surfaced deterministically by sc-120488's tests, which write through the same session after `get_activity` — CI traceback: [run 34888050672 / job 104123743848](https://github.com/apache/superset/actions/runs/34888050672/job/104123743848). The fix rides the option on each SELECT statement inste
ad — dialect-neutral, one-line, no connection mutation.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — connection semantics.
### TESTING INSTRUCTIONS
`pytest tests/integration_tests/versioning/activity_write_after_read_tests.py` — an activity read followed by an INSERT + commit on the same session (the DBEventLogger shape). Control-verified on PostgreSQL 16: reverting the fix fails the test with the DECLARE-CURSOR syntax error; green on SQLite/MySQL either way (only PG maps stream_results to server-side cursors).
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-120955
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5705734309
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | c3add97bf50f1280876ae00e4aff94dbd6255885 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aab1e7e0dad600008ba822d |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44261--superset-docs-preview.netlify.app](https://deploy-preview-44261--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DYtJDoAgDAD_0rMkSCIuP_AZhTbxIEJoPRjj3-2cZg7zQuu1NIUNYIDU8cqHuWQ3Br9OkxPtjMV1lvtUsUTlwpc6ybWxPcTtrM9OdkXENPLMnpCiN5aESwgE3w8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44261--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44261:
URL: https://github.com/apache/superset/pull/44261#discussion_r4034916897
##########
superset/versioning/activity/queries.py:
##########
@@ -418,10 +418,17 @@ def _select_change_rows_for_kinds(
vc.c.sequence.desc(),
vc.c.entity_id.desc(),
)
+ # stream_results rides the STATEMENT, never the connection:
+ # ``Connection.execution_options`` mutates the session's
+ # connection in place, permanently flipping every later
+ # statement on the request into a server-side cursor — on
+ # PostgreSQL that wraps subsequent INSERTs/SAVEPOINTs in
+ # ``DECLARE ... CURSOR FOR`` and they fail with a syntax
+ # error (observed: Continuum's transaction insert and the
+ # DBEventLogger write after an activity read; sc-120955).
result = (
db.session.connection()
- .execution_options(stream_results=True)
- .execute(stmt)
+ .execute(stmt.execution_options(stream_results=True))
Review Comment:
Verified on PG16/psycopg2 2.9.12: `create_cursor()` runs in `_execute_context` before `_exec_single_context` fires `before_cursor_execute`, so `cursor.name` is set — fix gives `['c_..._1']`, dropping `stream_results` gives `[None]`. Keep the named-cursor assertion; the timing objection doesn't hold. Resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44261:
URL: https://github.com/apache/superset/pull/44261#discussion_r4024252815
##########
superset/versioning/activity/queries.py:
##########
@@ -418,10 +418,17 @@ def _select_change_rows_for_kinds(
vc.c.sequence.desc(),
vc.c.entity_id.desc(),
)
+ # stream_results rides the STATEMENT, never the connection:
+ # ``Connection.execution_options`` mutates the session's
+ # connection in place, permanently flipping every later
+ # statement on the request into a server-side cursor — on
+ # PostgreSQL that wraps subsequent INSERTs/SAVEPOINTs in
+ # ``DECLARE ... CURSOR FOR`` and they fail with a syntax
+ # error (observed: Continuum's transaction insert and the
+ # DBEventLogger write after an activity read; sc-120955).
result = (
db.session.connection()
- .execution_options(stream_results=True)
- .execute(stmt)
+ .execute(stmt.execution_options(stream_results=True))
Review Comment:
Verified on PG16/SA 2.0.52 that statement-level `stream_results` really does stream — the fix holds. But deleting the option entirely still passes this test and the whole versioning suite, so nothing pins that this path streams. A `before_cursor_execute` spy asserting `cursor.name` catches it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5694829290
<!-- Bito Reply -->
The observation is correct. Moving `stream_results=True` from the connection level to the statement level is the necessary fix to prevent the connection from being permanently mutated into server-side cursor mode. Using a `before_cursor_execute` event listener to inspect `cursor.name` or the statement itself is a robust way to verify that the streaming option is correctly applied to the specific statement rather than the connection.
**superset/versioning/activity/queries.py**
```
result = (
db.session.connection()
- .execution_options(stream_results=True)
- .execute(stmt)
+ .execute(stmt.execution_options(stream_results=True))
.mappings()
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5707265623
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5670774525
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44261?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.53%. Comparing base ([`eeafd65`](https://app.codecov.io/gh/apache/superset/commit/eeafd65b746c8cff1c424906d385fe911175661b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`345fd33`](https://app.codecov.io/gh/apache/superset/commit/345fd33abdb3534e5ce29cee7cf59d298440c079?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (eeafd65) and HEAD (345fd33). Click for more details.
>
> <details><summary>HEAD has 61 uploads less than BASE</summary>
>
>| Flag | BASE (eeafd65) | HEAD (345fd33) |
>|------|------|------|
>|python|48|2|
>|hive|9|1|
>|presto|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44261 +/- ##
===========================================
- Coverage 80.18% 57.53% -22.66%
===========================================
Files 2925 2925
Lines 172692 172692
Branches 40090 40090
===========================================
- Hits 138474 99359 -39115
- Misses 31623 72393 +40770
+ Partials 2595 940 -1655
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.51% <ø> (+0.17%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.40% <ø> (+0.17%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.45% <ø> (-45.13%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44261/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44261?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #44261:
URL: https://github.com/apache/superset/pull/44261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5670714516
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44261:
URL: https://github.com/apache/superset/pull/44261#discussion_r4031515073
##########
superset/versioning/activity/queries.py:
##########
@@ -418,10 +418,17 @@ def _select_change_rows_for_kinds(
vc.c.sequence.desc(),
vc.c.entity_id.desc(),
)
+ # stream_results rides the STATEMENT, never the connection:
+ # ``Connection.execution_options`` mutates the session's
+ # connection in place, permanently flipping every later
+ # statement on the request into a server-side cursor — on
+ # PostgreSQL that wraps subsequent INSERTs/SAVEPOINTs in
+ # ``DECLARE ... CURSOR FOR`` and they fail with a syntax
+ # error (observed: Continuum's transaction insert and the
+ # DBEventLogger write after an activity read; sc-120955).
result = (
db.session.connection()
- .execution_options(stream_results=True)
- .execute(stmt)
+ .execute(stmt.execution_options(stream_results=True))
Review Comment:
Addressed in c3add97bf50f1280876ae00e4aff94dbd6255885. The observer sees the actual version_changes SELECT independently of its execution options and requires named cursors on PostgreSQL. On disposable PostgreSQL 17, removing streaming failed with all([None, None]); putting it back on the connection failed with DECLARE CURSOR FOR INSERT. Restoring statement scope passed, with clean connection options and the later write intact. Also replaced the admin-helper Any return with FAB User. Fresh 161 versioning unit tests and changed-file pre-commit passed; independent final-snapshot review found no publication blockers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(versioning): scope stream_results to the activity SELECTs instead of mutating the session connection [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44261:
URL: https://github.com/apache/superset/pull/44261#issuecomment-5708772162
Prepared by Codex at Mike's request. Following up on Bito's cursor-name timing note (https://github.com/apache/superset/pull/44261#issuecomment-5707265623): could we retain the named-cursor assertion? SQLAlchemy 2.0.52's `_PGExecutionContext_common_psycopg.create_server_side_cursor` constructs the identifier and calls `self._dbapi_connection.cursor(ident)` before execution; it is not first assigned inside `do_execute`. I inspected that installed implementation and exercised its cursor-creation call.
The actual PostgreSQL CI job at tested merge `268ec0434d3ab355587ea03d4679019fe38534c0` logs `tests/integration_tests/versioning/activity_write_after_read_tests.py .` (not skipped), then `2791 passed, 165 skipped`: https://github.com/apache/superset/actions/runs/35166198716/job/105027834221 . Published head remains `bfd6f58fc7d2d2631a2b8809ba4881a2a6cd0e06`. The earlier negative controls and fix are linked in https://github.com/apache/superset/pull/44261#discussion_r4031515073 . An execution-options-only assertion would weaken the regression the reviewer requested. This disposes the timing objection as a false positive; it does not claim reviewer approval.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org