Fixes #6767: correctly rehash queries in a migration #7184

arikfr · 2024-10-07T07:28:59Z

What type of PR is this?

Bug Fix

Description

TBD.

How is this tested?

Manually

Related Tickets & Documents

Fixes #6767.

arikfr · 2024-10-07T07:30:06Z

redash/metrics/database.py

@@ -18,7 +18,7 @@ def _table_name_from_select_element(elt):
    if isinstance(t, Alias):
        t = t.original.froms[0]

-    while isinstance(t, _ORMJoin):
+    while isinstance(t, _ORMJoin) or isinstance(t, Join):


Drive by fix as otherwise this would generate an error message in the logs when running the migration.

justinclift · 2024-10-07T07:42:15Z

The concept sounds ok, though we'll need someone with a decent understanding of SQLAlchemy and Alembic (not me yet) to do the technical review. 😄

justinclift · 2024-10-07T08:47:48Z

If no-one else gets to it sooner, I'll do some testing of this tomorrow against a production instance that was having upgrade issues due to this and verify it works there. 😄

arikfr · 2024-10-07T08:50:13Z

@justinclift note that it will likely not fix the issue. This is only half the solution. The other part is #7111. But I'm not sure about the fix there (will soon publish comments).

The other option is to update the hash of the query result referenced by latest_query_data_id. Might not be big of a deal on most deployments, but can take time on bigger deployments.

arikfr · 2024-10-07T09:27:33Z

@justinclift after reviewing it again, the issue with different hash in latest_query_data shouldn't be a problem as we don't verify it in Queries.outdated_queries. So once you apply this fix the scheduler should refresh your queries and the embeds will work.

justinclift · 2024-10-08T09:01:44Z

Thanks @arikfr. I didn't get up to testing this today, so it'll have to be a tomorrow job. I'll take a look at this in combination with #7111 as well then, to get a better understanding of things. 😄

arikfr · 2024-10-16T06:49:50Z

@justinclift did you have a chance to test this?

justinclift · 2024-10-16T10:05:51Z

It was on today's ToDo list, but other stuff happened.

ie: due to a change in IP addressing I found I couldn't access the required VMs from the local co-working place (now fixed), so instead used the time there to get other stuff done.

It's on tomorrow's ToDo list now 😉

justinclift · 2024-10-17T06:10:26Z

Testing this today. It's taking a bit of time to do the setup, and the testing might run into tomorrow as well (will find out), but it's finally at the top of the ToDo list. 🕺

justinclift · 2024-10-18T02:15:18Z

@arikfr This is throwing an error in my testing while doing a version 10.x upgrade to the latest source (with this PR and #7111 cherry-picked on top):

$ docker compose run --rm server manage db upgrade
[+] Creating 3/1
 ✔ Network redash_default       Created                                                                                      0.2s 
 ✔ Container redash-postgres-1  Created                                                                                      0.0s 
 ✔ Container redash-redis-1     Created                                                                                      0.0s 
[+] Running 2/2
 ✔ Container redash-postgres-1  Started                                                                                      0.4s 
 ✔ Container redash-redis-1     Started                                                                                      0.4s 
[2024-10-18 02:04:44,623][PID:1][INFO][xmlschema] Resource 'XMLSchema.xsd' is already loaded
[2024-10-18 02:04:45,417][PID:1][INFO][alembic.runtime.migration] Context impl PostgresqlImpl.
[2024-10-18 02:04:45,417][PID:1][INFO][alembic.runtime.migration] Will assume transactional DDL.
[2024-10-18 02:04:45,439][PID:1][INFO][alembic.runtime.migration] Running upgrade 89bc7873a3e0 -> fd4fc850d7ea, Convert user details to jsonb and move user profile image url into details column
[2024-10-18 02:04:45,465][PID:1][INFO][alembic.runtime.migration] Running upgrade fd4fc850d7ea -> 1038c2174f5d, Make case insensitive hash of query text
[2024-10-18 02:04:45,552][PID:1][INFO][alembic.runtime.migration] Running upgrade 1038c2174f5d -> 7ce5925f832b, create sqlalchemy_searchable expressions
[2024-10-18 02:04:45,559][PID:1][INFO][alembic.runtime.migration] Running upgrade 7ce5925f832b -> 7205816877ec, change type of json fields from varchar to json
[2024-10-18 02:04:48,504][PID:1][INFO][alembic.runtime.migration] Running upgrade 7205816877ec -> 9e8c841d1a30, fix_hash
Updating hash for query 5 from 209f28b6c5571340d7aed506d707a6a9 to 209f28b6c5571340d7aed506d707a6a9
Updating hash for query 4 from ffacd8e169e795a5a2d35482d301bb49 to ffacd8e169e795a5a2d35482d301bb49
Updating hash for query 6 from e9c93eeb5976135b5f81c262cb9d6438 to e9c93eeb5976135b5f81c262cb9d6438
Updating hash for query 7 from bd85101354567c12ef56d767e1d9e4f5 to e777f339433f10c9fdedbe8fc7d4e76c
Updating hash for query 8 from b8e158310114b5d440e2cd060cbf9346 to b8e158310114b5d440e2cd060cbf9346
Updating hash for query 10 from aa51de6c3447152a013a746431205f29 to aa51de6c3447152a013a746431205f29
Updating hash for query 9 from e9c93eeb5976135b5f81c262cb9d6438 to e9c93eeb5976135b5f81c262cb9d6438
Updating hash for query 12 from 6bcc859314ae910e9df438805f3f9b6e to 6bcc859314ae910e9df438805f3f9b6e
Updating hash for query 11 from dfee82d57d1a83510d866a68db3fe3ea to dfee82d57d1a83510d866a68db3fe3ea
Updating hash for query 33 from 4ee98762b0db77a456ee6c8ae7063322 to d41d8cd98f00b204e9800998ecf8427e
Updating hash for query 26 from 7febe307184eb936d0dd9a33e6a387b9 to 54396c36d46312a19e3d9cc960ef7193
Updating hash for query 14 from 9cc129d86490d58ef12b7e5ee0908c39 to 9cc129d86490d58ef12b7e5ee0908c39
Updating hash for query 13 from 71f56f14471767cb2c8017c2023d61ac to 71f56f14471767cb2c8017c2023d61ac
Updating hash for query 16 from 4d56fdfb4fa183471c2247a394484b69 to 4d56fdfb4fa183471c2247a394484b69
Updating hash for query 15 from ce3e3cee5869f05c6e9aa9f735bfa8be to ce3e3cee5869f05c6e9aa9f735bfa8be
Updating hash for query 35 from 926732926f1631c09f2128df445c45ee to 31863bdaa2478f2e1b54357978099c2b
Traceback (most recent call last):
  File "/app/manage.py", line 9, in <module>
    manager()
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/flask/cli.py", line 357, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/flask_migrate/cli.py", line 134, in upgrade
    _upgrade(directory, revision, sql, tag, x_arg)
  File "/usr/local/lib/python3.10/site-packages/flask_migrate/__init__.py", line 95, in wrapped
    f(*args, **kwargs)
  File "/usr/local/lib/python3.10/site-packages/flask_migrate/__init__.py", line 280, in upgrade
    command.upgrade(config, revision, sql=sql, tag=tag)
  File "/usr/local/lib/python3.10/site-packages/alembic/command.py", line 403, in upgrade
    script.run_env()
  File "/usr/local/lib/python3.10/site-packages/alembic/script/base.py", line 583, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/usr/local/lib/python3.10/site-packages/alembic/util/pyfiles.py", line 95, in load_python_file
    module = load_module_py(module_id, path)
  File "/usr/local/lib/python3.10/site-packages/alembic/util/pyfiles.py", line 113, in load_module_py
    spec.loader.exec_module(module)  # type: ignore
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/app/migrations/env.py", line 93, in <module>
    run_migrations_online()
  File "/app/migrations/env.py", line 85, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/usr/local/lib/python3.10/site-packages/alembic/runtime/environment.py", line 948, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/usr/local/lib/python3.10/site-packages/alembic/runtime/migration.py", line 627, in run_migrations
    step.migration_fn(**kw)
  File "/app/migrations/versions/9e8c841d1a30_fix_hash.py", line 55, in upgrade
    new_hash = update_query_hash(record)
  File "/app/migrations/versions/9e8c841d1a30_fix_hash.py", line 31, in update_query_hash
    print(f"Query {record.id} has parameters. Hash might be incorrect.")
AttributeError: Could not locate column in row for column 'id'

This is happening in a clone of the VM from last time. Want to take a look? 😄

(I'll need to add your IP address to the firewall again though, as I forgot to write it down... 🤦)

migrations/versions/9e8c841d1a30_fix_hash.py

arikfr · 2024-10-19T19:27:16Z

@justinclift I think I fixed it in: 54c3130, but if it persists I can take a look in the VM.

justinclift · 2024-10-20T01:20:18Z

No worries. I should be good to test that in a few hours. 😄

justinclift · 2024-10-20T05:54:15Z

That looks a lot happier:

# docker compose run --rm server manage db upgrade
[+] Creating 3/3
 ✔ Network redash_default       Created                                                                                                                                                      0.2s 
 ✔ Container redash-postgres-1  Created                                                                                                                                                      0.0s 
 ✔ Container redash-redis-1     Created                                                                                                                                                      0.0s 
[+] Running 2/2
 ✔ Container redash-postgres-1  Started                                                                                                                                                      0.4s 
 ✔ Container redash-redis-1     Started                                                                                                                                                      0.4s 
[2024-10-20 05:47:12,375][PID:1][INFO][xmlschema] Resource 'XMLSchema.xsd' is already loaded
[2024-10-20 05:47:13,714][PID:1][INFO][alembic.runtime.migration] Context impl PostgresqlImpl.
[2024-10-20 05:47:13,714][PID:1][INFO][alembic.runtime.migration] Will assume transactional DDL.
[2024-10-20 05:47:13,739][PID:1][INFO][alembic.runtime.migration] Running upgrade 89bc7873a3e0 -> fd4fc850d7ea, Convert user details to jsonb and move user profile image url into details column
[2024-10-20 05:47:13,766][PID:1][INFO][alembic.runtime.migration] Running upgrade fd4fc850d7ea -> 1038c2174f5d, Make case insensitive hash of query text
[2024-10-20 05:47:13,853][PID:1][INFO][alembic.runtime.migration] Running upgrade 1038c2174f5d -> 7ce5925f832b, create sqlalchemy_searchable expressions
[2024-10-20 05:47:13,860][PID:1][INFO][alembic.runtime.migration] Running upgrade 7ce5925f832b -> 7205816877ec, change type of json fields from varchar to json
[2024-10-20 05:47:18,181][PID:1][INFO][alembic.runtime.migration] Running upgrade 7205816877ec -> 9e8c841d1a30, fix_hash
Updating hash for query 5 from 209f28b6c5571340d7aed506d707a6a9 to 209f28b6c5571340d7aed506d707a6a9
[...]
Query 79 has parameters. Hash might be incorrect.
Updating hash for query 79 from 1a669d77457deac81ea4bda25be82cbc to b44167a0c358aca04a2b99c73dee81d0
Query 51 has parameters. Hash might be incorrect.
Updating hash for query 51 from 47002ee32bd658a4ebaea47f0da7498b to abab013e068c74ef7ee7d753e2f99440
Query 28 has parameters. Hash might be incorrect.
Updating hash for query 28 from 8d8acd3405f3023eb2058941cb6cd6cb to 885e7c01891a0e0a936c0a8d5d3d708a
[...]
Updating hash for query 2 from 65cd9109d7decb3e8061210104ea1795 to 023240adf80d5c44dbe3fb6a1707e010
Updating hash for query 75 from ca9478c82f47db00d80d6e130a6513e5 to 63043dc0193684393b67a0513321e1b9
Updating hash for query 72 from efa133919fef5d1397ff5aed0e1284ce to 98357f178f81b79fdd1f3fabdfab8b6d

The Query N has parameters. Hash might be incorrect. warnings are a bit of a concern, but I'll check in more detail later if things are broken when those queries are used for embedded visualisations.

While I'm at it, I might as well check some other production Redash databases too, and see if anything shows up with them. Best to catch any weirdness early before other people start trying this in earnest. 😄

That further testing will happen over the coming days, but shouldn't take too much time.

Thanks for putting time into this @arikfr. 😄

justinclift · 2024-10-24T07:57:52Z

Just tried this out with every production Redash database we're hosting, and no further errors were thrown. 😄

I have not yet checked if the Query 28 has parameters. Hash might be incorrect. warnings results in any problems for queries used in embeds though. That still needs doing.

justinclift · 2024-10-24T22:24:10Z

Came across something a bit unexpected when prodding this while setting things up to do some more testing. It looks like the rehashing done by database migrations is a bit different to the rehashing when triggered from the command line.

Example from database migration

# docker compose run --rm server manage db upgrade
...
Updating hash for query 35 from 926732926f1631c09f2128df445c45ee to 31863bdaa2478f2e1b54357978099c2b
Query 78 has parameters. Hash might be incorrect.
Updating hash for query 78 from bf54630da6f0db33c1d879d37aab3c20 to 6f1d59c77fb1e7d66937e8acff1b78f0
Query 44 has parameters. Hash might be incorrect.
Updating hash for query 44 from 2f06b167b7e4e03e18286cdc380946cd to 6db929d33f42c86464ee0c971d3cd933
...

Example from command line

# docker compose run --rm server manage queries rehash
...
Query 78 has changed hash from 6f1d59c77fb1e7d66937e8acff1b78f0 to e9322b09eac96c1a46a9be4aaf10b247
Query 44 has changed hash from 6db929d33f42c86464ee0c971d3cd933 to c4dd2f284b1bb73c4f258fadb12a9491
...

Manually running the rehashing operation a second time reports no hashes changing:

# docker compose run --rm server manage queries rehash
[+] Creating 2/0
 ✔ Container redash-redis-1     Running           0.0s 
 ✔ Container redash-postgres-1  Running           0.0s 
[2024-10-24 22:21:10,747][PID:1][INFO][xmlschema] Resource 'XMLSchema.xsd' is already loaded
#

What seems to be happening is the manual rehash from the cli is only reporting when hashes change, and the first time it runs it changes exactly (and only) the hashes which the database migration said might be incorrect.

Haven't yet tested how the pieces work in each instance when the corresponding query is used in an embed. Will be doing so today. 😄

justinclift · 2024-10-24T23:02:52Z

k, this PR looks good to me now. When testing the embeds they all seem to be happy now, even the queries that had hash warnings appear are working ok. 😄

arikfr · 2024-10-27T10:48:42Z

@justinclift thank you for your effort with testing this! The hash is different because rehash uses the model code which is actually correct for all use cases (including queries with parameters).

We should probably document the need to run rehash if you see this warning (or maybe update the warning to say this?).

justinclift · 2024-10-27T22:27:19Z

Yeah, updating the warning is probably the right move. That should help minimise the number of support issues people open about it. 😁

arikfr commented Oct 7, 2024

View reviewed changes

arikfr mentioned this pull request Oct 7, 2024

A db migration does not take Query#apply_auto_limit in count #6767

Closed

arikfr requested review from justinclift and eradman October 7, 2024 07:42

arikfr commented Oct 19, 2024

View reviewed changes

migrations/versions/9e8c841d1a30_fix_hash.py Outdated Show resolved Hide resolved

justinclift approved these changes Oct 24, 2024

View reviewed changes

justinclift force-pushed the fix-refresh branch from 54c3130 to b0db978 Compare October 24, 2024 23:05

justinclift enabled auto-merge (squash) October 24, 2024 23:06

arikfr added 4 commits October 25, 2024 10:29

Fix: metrics wasn't handling non ORM join correctly

7ecd629

Rehash queries correctly

4633fe2

add manage.py queries rehash cli command to rehash queries

baaf3c5

Update migrations/versions/9e8c841d1a30_fix_hash.py

a5ae950

justinclift force-pushed the fix-refresh branch from b0db978 to a5ae950 Compare October 25, 2024 00:29

justinclift merged commit ba973eb into master Oct 25, 2024
14 checks passed

justinclift deleted the fix-refresh branch October 25, 2024 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6767: correctly rehash queries in a migration #7184

Fixes #6767: correctly rehash queries in a migration #7184

arikfr commented Oct 7, 2024

arikfr Oct 7, 2024

justinclift commented Oct 7, 2024

justinclift commented Oct 7, 2024

arikfr commented Oct 7, 2024

arikfr commented Oct 7, 2024

justinclift commented Oct 8, 2024

arikfr commented Oct 16, 2024

justinclift commented Oct 16, 2024

justinclift commented Oct 17, 2024

justinclift commented Oct 18, 2024

arikfr commented Oct 19, 2024

justinclift commented Oct 20, 2024

justinclift commented Oct 20, 2024 •

edited

Loading

justinclift commented Oct 24, 2024 •

edited

Loading

justinclift commented Oct 24, 2024 •

edited

Loading

justinclift commented Oct 24, 2024

arikfr commented Oct 27, 2024

justinclift commented Oct 27, 2024 •

edited

Loading

Fixes #6767: correctly rehash queries in a migration #7184

Fixes #6767: correctly rehash queries in a migration #7184

Conversation

arikfr commented Oct 7, 2024

What type of PR is this?

Description

How is this tested?

Related Tickets & Documents

arikfr Oct 7, 2024

Choose a reason for hiding this comment

justinclift commented Oct 7, 2024

justinclift commented Oct 7, 2024

arikfr commented Oct 7, 2024

arikfr commented Oct 7, 2024

justinclift commented Oct 8, 2024

arikfr commented Oct 16, 2024

justinclift commented Oct 16, 2024

justinclift commented Oct 17, 2024

justinclift commented Oct 18, 2024

arikfr commented Oct 19, 2024

justinclift commented Oct 20, 2024

justinclift commented Oct 20, 2024 • edited Loading

justinclift commented Oct 24, 2024 • edited Loading

justinclift commented Oct 24, 2024 • edited Loading

Example from database migration

Example from command line

justinclift commented Oct 24, 2024

arikfr commented Oct 27, 2024

justinclift commented Oct 27, 2024 • edited Loading

justinclift commented Oct 20, 2024 •

edited

Loading

justinclift commented Oct 24, 2024 •

edited

Loading

justinclift commented Oct 24, 2024 •

edited

Loading

justinclift commented Oct 27, 2024 •

edited

Loading