Skip to content

Conversation

@melton-jason
Copy link
Contributor

Addresses part of #5337
Fixes #4148

Some raw ideas:

Make this accept two args: one for read and one for writes.

def lock_tables(*tables):

Make this return 2 sets, one for read and one for writes. Repetition isn't allowed, so if a table is in both, elevate it to a write (just remove it from read). Put all the discipline, collection, and uniqueness rule tables, and the scope_table in the read set (basically, everything we know we won't write to)

def get_tables_to_lock(collection, obj, field_names) -> Set[str]:

From #5337 (comment)

Now when autonumbering, Specify will only issue a write lock on the table which will be updated, read-locking all other tables. This will allow other users of Specify to read from common tables (collection, discipline, etc.) while some other user is in the process of autonumbering one or more records.
Previously, these tables would be inaccessible to other users until the autonumbering process completed. This created a massive problem when using the WorkBench and relying on autonumbering: a WorkBench validation or upload could take a significant amount of time to complete, meaning Specify would not be usable for other users.

In the future, we can extend the capability further and leverage Row Locks for even more fine-tuned control for some tables.

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

Coming Soon

@grantfitzsimmons grantfitzsimmons added this to the 7.9.10 milestone Nov 25, 2024
@grantfitzsimmons grantfitzsimmons modified the milestones: 7.9.10, 7.13+ Jun 27, 2025
@grantfitzsimmons grantfitzsimmons requested a review from Copilot June 30, 2025 17:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refines the table‐locking mechanism to use separate read and write locks during the autonumbering process, reducing unnecessary full‐table write locks on shared tables.

  • lock_tables now accepts distinct read/write sets and constructs the LOCK TABLES SQL accordingly.
  • do_autonumbering is updated to only write‐lock the target table and read‐lock all others.
  • tree_extras helper functions have been cleaned up and replaced with metadata‐based checks.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
specifyweb/specify/tree_extras.py Removed unused imports, replaced regex checks with db_table suffix logic, added is_treetable
specifyweb/specify/lock_tables.py Updated lock_tables signature to (read_tables, write_tables), built separate read/write clauses
specifyweb/specify/autonumbering.py Switched to new lock_tables API, renamed helpers and added get_tables_to_read_lock and get_tree_tables_to_lock
Comments suppressed due to low confidence (2)

specifyweb/specify/autonumbering.py:47

  • [nitpick] The variable name writing_to is inconsistent with the function parameter write_tables. Consider renaming it to write_tables for clarity and consistency.
    writing_to =  set([obj._meta.db_table])

specifyweb/specify/lock_tables.py:12

  • The new read/write locking API has several edge cases (no write tables, no read tables, overlapping sets). Please add unit or integration tests to verify correct SQL generation and behavior across all combinations.
def lock_tables(read_tables: Set[str], write_tables: Set[str]):

write_statement = ','.join(
[table + ' write' for table in write_tables]) + ',' if len(write_tables) > 0 else ''
read_statement = ','.join(
[table + ' read' for table in final_read_tables])
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

When write_tables is non-empty and read_tables is empty, write_statement ends with a trailing comma and read_statement is empty, resulting in invalid SQL (e.g., LOCK TABLES t1 write,;). Handle the empty-read case explicitly or strip off the extra comma before executing.

Suggested change
[table + ' read' for table in final_read_tables])
[table + ' read' for table in final_read_tables])
# Remove trailing comma from write_statement if read_statement is empty
if not read_statement:
write_statement = write_statement.rstrip(',')

Copilot uses AI. Check for mistakes.

return tables

def get_tree_tables_to_lock(tree_table: str) -> set[str]:
Copy link

Copilot AI Jun 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Hardcoding related table names (f'{tree_table}def', f'{tree_table}defitem') may break if conventions evolve. Consider deriving these names from Django model metadata or a centralized utility.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📋Back Log

Development

Successfully merging this pull request may close these issues.

Autoincrementing formatter not locking all required tree tables

4 participants