Skip to content

Add extended stats and force of custom query plans #1543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nmaytan wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add extended stats and force of custom query plans #1543

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5beed02
Add extended stats and force of custom plans to migration
nmaytan Oct 1, 2026
00a4729
Add ext stats to ORM, add force_custom_plan hook
nmaytan Oct 1, 2026
ca81ad2
Make GraphQL also use literal tag ids in queries
nmaytan Oct 9, 2026
eb4f154
Rename table to 'access_grants'
nmaytan Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion tiled/catalog/migrations/versions/0d4e1f2a3b4c_add_parent_to_node_access_tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
depends_on = None

INDEX_NAME = "ix_node_access_tags_association_parent_id_tag_id_node_id"
STATISTICS_NAME = "node_access_tags_association_parent_id_tag_id_node_id_stats"

# Tune these for the database host running the migration. These settings apply
# only to this migration's PostgreSQL connection. work_mem may be consumed by
Expand Down Expand Up @@ -165,7 +166,15 @@ def upgrade():
"node_access_tags_association",
["parent_id", "tag_id", "node_id"],
)
op.execute("ANALYZE node_access_tags_association")
op.execute(
f"CREATE STATISTICS IF NOT EXISTS {STATISTICS_NAME} "
"(dependencies, ndistinct, mcv) "
"ON parent_id, tag_id FROM node_access_tags_association"
)
# VACUUM cannot run inside a transaction. Creating the statistics before
# this also ensures that VACUUM ANALYZE populates them immediately.
with op.get_context().autocommit_block():
connection.execute(sa.text("VACUUM ANALYZE node_access_tags_association"))
else:
op.execute(
"""
Expand All @@ -189,6 +198,7 @@ def downgrade():

_drop_triggers(connection)
if dialect_name == "postgresql":
op.execute(f"DROP STATISTICS IF EXISTS {STATISTICS_NAME}")
op.drop_index(
INDEX_NAME,
table_name="node_access_tags_association",
Expand Down
14 changes: 14 additions & 0 deletions tiled/catalog/orm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,20 @@ def create_node_access_tag_parent_triggers(target, connection, **kw):
)


@event.listens_for(NodeAccessTagAssociation.__table__, "after_create")
def create_node_access_tag_statistics(target, connection, **kw):
if connection.engine.dialect.name == "postgresql":
connection.execute(
text(
"""
CREATE STATISTICS IF NOT EXISTS node_access_tags_association_parent_id_tag_id_node_id_stats

@danielballan danielballan Oct 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisiting this after a couple days, I'm feeling strongly we should just name this grants or access_grants (pick your preference). If the meaning/content is unclear, that's what schema introspection is more; we don't need the column names in the table name.

@nmaytan nmaytan Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to do this in separate PR, but it can be done here. This is also a database migration to go with this, of course.

@nmaytan nmaytan Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a commit incoming for this. However, I note that calling this access_grants breaks our convention of appending "association" to all association tables.

@nmaytan nmaytan Oct 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the stats highlighted by this thread are not on the grants table - they are on the node-tag associations table, so the name of this doesn't change. I've left it with the existing name.

(dependencies, ndistinct, mcv)
ON parent_id, tag_id
FROM node_access_tags_association"""
)
)


class AccessTagsPrincipal(Timestamped, Base):
"""
A principal (human user or service account) that can be granted access
Expand Down
11 changes: 11 additions & 0 deletions tiled/server/connection_pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ def open_database_connection_pool(database_settings: DatabaseSettings) -> AsyncE
# For SQLite, ensure that foreign key constraints are enforced.
if engine.dialect.name == "sqlite":
event.listens_for(engine.sync_engine, "connect")(_set_sqlite_pragma)
elif engine.dialect.name == "postgresql":
event.listens_for(engine.sync_engine, "connect")(
_set_postgresql_plan_cache_mode
)

return engine

Expand Down Expand Up @@ -102,6 +106,13 @@ def _set_sqlite_pragma(conn, record):
cursor.close()


def _set_postgresql_plan_cache_mode(conn, record):
cursor = conn.cursor()
cursor.execute("SET plan_cache_mode = force_custom_plan")
cursor.close()
conn.commit()


def is_memory_sqlite(url: Union[URL, str]) -> bool:
"""
Check if a SQLAlchemy URL is a memory-backed SQLite database.
Expand Down
Loading