Repository navigation
Conversation
8d1ef85 to
5beed02
Compare
danielballan
left a comment
There was a problem hiding this comment.
This looks good to go save for my naming nit-pick.
| connection.execute( | ||
| text( | ||
| """ | ||
| CREATE STATISTICS IF NOT EXISTS node_access_tags_association_parent_id_tag_id_node_id_stats |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This adjustment brings the migration in harmony to what we deployed in prod. This is currently the intended configuration for all deployments. We also in this PR make GraphQL queries use literal tag IDs, matching catalog queries. Finally, a new migration script is added which renames the grants table to something less verbose.
Note on forcing custom plans:
force_custom_planduring migration because for this to be set fortiledrole that role must exist, which is not defined/guaranteed by the ORM (CI doesn't use this role, for example)<current user>either, because that may have unintended effects (e.g. if someone is usingpostgresuser for Tiled connections, it will force custom plans everywhere in the postgres instance instead of just for Tiled)Other notes:
Checklist