sql_lab.get_sql_results encountering psycopg2.errors.SerializationFailure #30064
Comments
|
@mapledan Hello! I'm here to assist you with any bugs or questions you have. If you need any help, feel free to ask. The # Commit the connection so CTA queries will create the table and any DML.
should_commit = (
not db_engine_spec.is_select_query(parsed_query) # check if query is DML
or apply_ctas
)
if should_commit:
conn.commit()This commit operation can lead to serialization failures in PostgreSQL, especially in high-concurrency environments or when using certain isolation levels. The To mitigate this issue, you can set the if make_url(SQLALCHEMY_DATABASE_URI).get_backend_name() in ("postgresql", "mysql"):
SQLALCHEMY_ENGINE_OPTIONS["isolation_level"] = "READ COMMITTED" # noqa: F405This configuration can be added to your Superset configuration file [2]. |
dosubot
bot
added
data:connect:postgres
|
My PostgreSQL configuration |
|
@mapledan I linked your issue here as instructed by our release process. If you find any other issues while testing 4.1.0 RC2, please add the link to it there so we can track. Thanks for testing the RC! |
|
Curious, does this error occur if you switch to read-uncommitted? I don't know much about
If people want to maintain a higher level for transaction integrity I'd suggest lowering the transaction level selectively on the scans across the app, particularly around the Anyone can think of potential issues using |
@villebro Will the shared lock you implemented be affected by this change? #28628 made both Postgres and MySQL isolation level consistent so we if decide to change this configuration, let's keep it consistent. |
curious to learn more about that
Personally I think read-uncommitted is probably a more suited default |
|
@mistercrunch @michael-s-molina I think we're getting things mixed up here - AFAIK this error doesn't have anything to do with Global Async Queries (I believe that part of the config was unnecessarily pasted here). Therefore, this regression appears to be related to how async execution is happening in SQL Lab (not in scope for current GAQ implementation). |
|
Yeah the error is triggered on the creation of the |
|
One idea that may help if you want to try it @mapledan from sqlalchemy import text
from sqlalchemy.engine import Connection
@staticmethod
def get_queries_changed_after(last_updated_ms: Union[float, int]) -> list[Query]:
# UTC date time, same that is stored in the DB.
last_updated_dt = datetime.utcfromtimestamp(last_updated_ms / 1000)
# Check the database dialect and apply AUTOCOMMIT only for supported databases
if db.session.bind.dialect.name in ["postgresql", "mysql"]:
return (
db.session.query(Query)
.filter(Query.user_id == get_user_id(), Query.changed_on >= last_updated_dt)
.execution_options(isolation_level="AUTOCOMMIT") # Use AUTOCOMMIT for compatible databases
.all()
)
else:
return (
db.session.query(Query)
.filter(Query.user_id == get_user_id(), Query.changed_on >= last_updated_dt)
.all()
)in this module -> superset/models/sql_lab.py , for the record this was generated by GPT but LGTM - could be more DRY though if we decided to open a PR with this idea. Wanna try it and report-back? BTW I double checked that we have a proper index on |
Thanks for providing the information and assistance! However, after adjusting the following settings in config.py, everything started working as expected: It seems the issue might not be related to |
|
@mistercrunch @mapledan I believe there's an error on #28628: If you read the comment, it seems the isolation level should be set to In that same PR, there's a test with the following: @sadpandajoe added a comment about this but never got a reply. In summary, I think the fix is to use |
|
@michael-s-molina your sqlite comment make sense, though now I'm confused. @mapledan reports that changing his Postgres to |
The PR that I linked is NOT setting Postgres to |
|
I think it would be a better idea to provide sane defaults in the extensions of |
@villebro That's what I meant with
A global configuration won't work because you may have multiple database connections with different transaction isolation levels. |
|
Oh right! I had assumed given the title of the PR, but clearly sets a default that doesn't match the PR title. Good catch. |
Mmmh, I get the idea, but I'd say the I'd open PR with a dynamic default for |
|
Submitting this DRAFT PR for review -> #30174 |
Yeah you're absolutely right, my bad 👍 However, I'm still not entirely buying into having this in |
Good point 👍🏼 |
|
@mistercrunch @villebro Do you think this configuration could belong to the database connection modal? Maybe in the 
This would allow us to change the isolation level for for both metadata and analytical databases. |
I think this setting matters most for the metadata database settings as it relates to OLTP-type workloads, that db connection is not exposed in the UI. For the metadata db, we should just pick a solid default ( It matters much less for the analytics-type workload, especially for single query per session that are just |
|
I agree with @mistercrunch , IIUC the issue here is limited to the metastore connection, not analytical db connections (for which the modal is relevant). So I'm leaning towards having sane defaults that can be overridden if needed. I propose moving the discussion to Max's PR (I'll take a look now). |
|
link to PR for convenience -> #30174 |
Bug description
In version 4.0.2, my sql_lab async tasks were functioning correctly with Celery. However, after upgrading to version 4.1.0rc2, I started encountering errors when executing sql_lab.get_sql_results. The tasks are now failing with a psycopg2.errors.SerializationFailure.
My databse is PostgreSQL 15.
Here is my current super_config:
How to reproduce the bug
Screenshots/recordings
Superset version
master / latest-dev
Python version
3.10
Node version
16
Browser
Chrome
Additional context
No response
Checklist
The text was updated successfully, but these errors were encountered: