mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-07-31 16:15:58 +00:00
Fix: apply final-review fix wave (safety guard, date spread, MariaDB explain, history fields, memory footprint, minors)
Final whole-branch review before this tooling branch settles: add a required --yes-i-know-this-wipes-the-database confirmation flag ahead of --reset (it deletes ALL users/groups/documents in the target DB, not just benchmark-created ones); spread seeded documents' created dates over a 3-year window instead of leaving them all on one default date; fix capture_explain() to use MariaDB's ANALYZE syntax instead of Postgres-only EXPLAIN ANALYZE (verified against real MariaDB 12.3 -- the old code was a silent 1064 syntax error); record db_vendor/document_count in run/profile history entries; shrink SeededData's memory footprint at scale by returning counts instead of full ORM instance tuples; and a handful of minor fixes (storage_path assignment, --explain warning instead of silent no-op, harness.py repeat<1 guard, type annotations). All changes verified against real Postgres and MariaDB containers on the VM, not just SQLite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
2712c3fe48
commit
fa2d7f660b
@@ -31,6 +31,14 @@ def _reset_table_names() -> list[str]:
|
||||
|
||||
|
||||
def _delete_all_users_and_groups() -> None:
|
||||
# ASSUMPTION: this tool assumes a disposable benchmark database, never
|
||||
# point it at a real install. This deletes EVERY user and group in the
|
||||
# database (not just benchmark-created ones) -- there is no way to
|
||||
# distinguish "real" users from seeded ones, so this is only safe against
|
||||
# a database that exists solely to run this benchmarking tool. The
|
||||
# `benchmark seed --reset` CLI path requires an explicit
|
||||
# `--yes-i-know-this-wipes-the-database` flag before reaching here; do
|
||||
# not remove that guard.
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.contrib.auth.models import Group
|
||||
|
||||
@@ -95,17 +103,34 @@ def reset_benchmark_data() -> None:
|
||||
def capture_explain(queryset: QuerySet) -> str:
|
||||
"""
|
||||
Return the query plan for `queryset` using the current backend's
|
||||
explain facility. PostgreSQL and MariaDB both support EXPLAIN ANALYZE
|
||||
(real execution stats). SQLite only supports EXPLAIN QUERY PLAN (the
|
||||
chosen plan, not real timing/row counts) -- that output is clearly
|
||||
labeled rather than silently looking equivalent to the other two
|
||||
backends' output.
|
||||
explain facility. PostgreSQL supports `EXPLAIN ANALYZE {sql}` (real
|
||||
execution stats). MariaDB does NOT accept that syntax -- verified
|
||||
against a real MariaDB 12.3 container: `EXPLAIN ANALYZE {sql}` raises a
|
||||
1064 syntax error, while MariaDB's own `ANALYZE {sql}` form (no
|
||||
`EXPLAIN` keyword) works and returns real per-row execution stats
|
||||
(`r_rows`, `r_filtered`, etc. columns) -- this is MariaDB's
|
||||
EXPLAIN-ANALYZE-equivalent, distinct from MySQL 8.0.18+'s
|
||||
`EXPLAIN ANALYZE` syntax, which MariaDB does not implement. SQLite only
|
||||
supports EXPLAIN QUERY PLAN (the chosen plan, not real timing/row
|
||||
counts) -- that output is clearly labeled rather than silently looking
|
||||
equivalent to the other two backends' output.
|
||||
"""
|
||||
sql, params = queryset.query.sql_with_params()
|
||||
with connection.cursor() as cursor:
|
||||
if connection.vendor in ("postgresql", "mysql"):
|
||||
if connection.vendor == "postgresql":
|
||||
cursor.execute(f"EXPLAIN ANALYZE {sql}", params)
|
||||
return "\n".join(str(row[0]) for row in cursor.fetchall())
|
||||
if connection.vendor == "mysql":
|
||||
# MariaDB also reports vendor == "mysql" under Django's mysql
|
||||
# backend. Unlike MySQL 8.0.18+, MariaDB has no `EXPLAIN
|
||||
# ANALYZE` syntax -- its equivalent is `ANALYZE <statement>`.
|
||||
cursor.execute(f"ANALYZE {sql}", params)
|
||||
columns = [c[0] for c in cursor.description]
|
||||
header = " | ".join(columns)
|
||||
rows = "\n".join(
|
||||
" | ".join(str(c) for c in row) for row in cursor.fetchall()
|
||||
)
|
||||
return f"{header}\n{rows}"
|
||||
cursor.execute(f"EXPLAIN QUERY PLAN {sql}", params)
|
||||
rows = "\n".join(" | ".join(str(c) for c in row) for row in cursor.fetchall())
|
||||
return f"(plan only -- no execution stats on SQLite)\n{rows}"
|
||||
|
||||
@@ -8,6 +8,7 @@ from typing import TYPE_CHECKING
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from django.contrib.auth.models import User
|
||||
from rest_framework.test import APIClient
|
||||
|
||||
ENDPOINTS: tuple[tuple[str, str], ...] = (
|
||||
("documents_default", "/api/documents/"),
|
||||
@@ -26,7 +27,7 @@ class EndpointTiming:
|
||||
max_ms: float
|
||||
|
||||
|
||||
def _timed_requests(client, url: str, n: int) -> list[float]:
|
||||
def _timed_requests(client: APIClient, url: str, n: int) -> list[float]:
|
||||
times = []
|
||||
for _ in range(n):
|
||||
t0 = time.perf_counter()
|
||||
@@ -40,7 +41,7 @@ def _timed_requests(client, url: str, n: int) -> list[float]:
|
||||
return times
|
||||
|
||||
|
||||
def _query_count(client, url: str) -> int:
|
||||
def _query_count(client: APIClient, url: str) -> int:
|
||||
from django.db import connection
|
||||
from django.test.utils import CaptureQueriesContext
|
||||
|
||||
|
||||
@@ -31,6 +31,9 @@ def run_profile(fn: Callable[[], T], *, repeat: int = 5) -> ProfileResult[T]:
|
||||
time across all repeats, since the first call(s) can be skewed by
|
||||
connection warm-up or cold caches.
|
||||
"""
|
||||
if repeat < 1:
|
||||
raise ValueError("repeat must be >= 1")
|
||||
|
||||
all_seconds: list[float] = []
|
||||
result: T | None = None
|
||||
query_count = 0
|
||||
@@ -41,7 +44,10 @@ def run_profile(fn: Callable[[], T], *, repeat: int = 5) -> ProfileResult[T]:
|
||||
all_seconds.append(time.perf_counter() - start)
|
||||
if i == repeat - 1:
|
||||
query_count = len(ctx.captured_queries)
|
||||
assert result is not None # repeat >= 1 guarantees at least one assignment
|
||||
# Purely a type-narrowing aid for the type checker: the `repeat < 1`
|
||||
# guard above already turns the one case that could leave `result`
|
||||
# unset into a clear ValueError, so this is unreachable in practice.
|
||||
assert result is not None
|
||||
return ProfileResult(
|
||||
best_seconds=min(all_seconds),
|
||||
all_seconds=tuple(all_seconds),
|
||||
|
||||
@@ -35,6 +35,17 @@ class Command(BaseCommand):
|
||||
default=False,
|
||||
help="For `seed`: truncate existing benchmark data first.",
|
||||
)
|
||||
parser.add_argument(
|
||||
"--yes-i-know-this-wipes-the-database",
|
||||
action="store_true",
|
||||
default=False,
|
||||
help=(
|
||||
"Required alongside --reset: confirms you understand `seed "
|
||||
"--reset` deletes ALL users, ALL groups, and ALL documents/"
|
||||
"tags/correspondents/document types/storage paths in this "
|
||||
"database, not just benchmark-created ones."
|
||||
),
|
||||
)
|
||||
parser.add_argument(
|
||||
"--seed",
|
||||
type=int,
|
||||
@@ -75,20 +86,32 @@ class Command(BaseCommand):
|
||||
from paperless_benchmark.seeding import seed_benchmark_dataset
|
||||
|
||||
if options["reset"]:
|
||||
if not options["yes_i_know_this_wipes_the_database"]:
|
||||
raise CommandError(
|
||||
"--reset requires --yes-i-know-this-wipes-the-database. "
|
||||
"This deletes ALL users, ALL groups, and ALL documents, "
|
||||
"tags, correspondents, document types, and storage paths "
|
||||
"in this database -- not just benchmark-created ones. "
|
||||
"Only run this against a disposable benchmark database, "
|
||||
"never a real install. Re-run with "
|
||||
"--reset --yes-i-know-this-wipes-the-database to proceed.",
|
||||
)
|
||||
self.stdout.write("Resetting existing benchmark data...")
|
||||
reset_benchmark_data()
|
||||
|
||||
data = seed_benchmark_dataset(options["tier"], seed=options["seed"])
|
||||
self.stdout.write(
|
||||
self.style.SUCCESS(
|
||||
f"Seeded tier={options['tier']!r}: {len(data.documents)} documents, "
|
||||
f"Seeded tier={options['tier']!r}: {data.documents} documents, "
|
||||
f"{len(data.users)} users, {len(data.groups)} groups.",
|
||||
),
|
||||
)
|
||||
|
||||
def _handle_run(self, options: dict[str, Any]) -> None:
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.db import connection
|
||||
|
||||
from documents.models import Document
|
||||
from paperless_benchmark.endpoints import run_endpoint_benchmarks
|
||||
from paperless_benchmark.results import append_history
|
||||
|
||||
@@ -101,6 +124,9 @@ class Command(BaseCommand):
|
||||
"No benchmark dataset found. Run `manage.py benchmark seed` first.",
|
||||
) from e
|
||||
|
||||
db_vendor = connection.vendor
|
||||
document_count = Document.objects.count()
|
||||
|
||||
results = run_endpoint_benchmarks(
|
||||
perf_target=perf_target,
|
||||
perf_admin=perf_admin,
|
||||
@@ -127,12 +153,16 @@ class Command(BaseCommand):
|
||||
"min_ms": r.min_ms,
|
||||
"median_ms": r.median_ms,
|
||||
"max_ms": r.max_ms,
|
||||
"db_vendor": db_vendor,
|
||||
"document_count": document_count,
|
||||
},
|
||||
)
|
||||
|
||||
def _handle_profile(self, options: dict[str, Any]) -> None:
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.db import connection
|
||||
|
||||
from documents.models import Document
|
||||
from paperless_benchmark.db import capture_explain
|
||||
from paperless_benchmark.harness import run_profile
|
||||
from paperless_benchmark.results import append_history
|
||||
@@ -162,9 +192,17 @@ class Command(BaseCommand):
|
||||
f"queries={profile.query_count}",
|
||||
)
|
||||
|
||||
if options["explain"] and scenario.queryset_for_explain is not None:
|
||||
plan = capture_explain(scenario.queryset_for_explain(perf_target))
|
||||
self.stdout.write(plan)
|
||||
if options["explain"]:
|
||||
if scenario.queryset_for_explain is not None:
|
||||
plan = capture_explain(scenario.queryset_for_explain(perf_target))
|
||||
self.stdout.write(plan)
|
||||
else:
|
||||
self.stdout.write(
|
||||
self.style.WARNING(
|
||||
f"--explain was requested but scenario {scenario.name!r} "
|
||||
"does not support it (no queryset_for_explain); skipping.",
|
||||
),
|
||||
)
|
||||
|
||||
append_history(
|
||||
{
|
||||
@@ -172,6 +210,8 @@ class Command(BaseCommand):
|
||||
"scenario": scenario.name,
|
||||
"best_seconds": profile.best_seconds,
|
||||
"query_count": profile.query_count,
|
||||
"db_vendor": connection.vendor,
|
||||
"document_count": Document.objects.count(),
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
# src/paperless_benchmark/seeding.py
|
||||
from __future__ import annotations
|
||||
|
||||
import datetime
|
||||
import random
|
||||
import time
|
||||
from dataclasses import dataclass
|
||||
@@ -11,12 +12,6 @@ if TYPE_CHECKING:
|
||||
from django.contrib.auth.models import Group
|
||||
from django.contrib.auth.models import User
|
||||
|
||||
from documents.models import Correspondent
|
||||
from documents.models import Document
|
||||
from documents.models import DocumentType
|
||||
from documents.models import StoragePath
|
||||
from documents.models import Tag
|
||||
|
||||
Tier = Literal["home", "medium", "large"]
|
||||
|
||||
CHUNK_SIZE = 5_000
|
||||
@@ -102,18 +97,28 @@ def log(msg: str) -> None:
|
||||
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class SeededData:
|
||||
"""
|
||||
Summary of a completed seed run. `documents`/`tags`/`correspondents`/
|
||||
`document_types`/`storage_paths` are counts, not the seeded ORM
|
||||
instances: at the `large` tier (360,000 documents) holding every
|
||||
instance in memory simultaneously is a real risk for zero benefit, since
|
||||
no caller consumes anything but the counts. `perf_target`/`perf_admin`/
|
||||
`users`/`groups` stay as real objects -- at most ~26 users/12 groups even
|
||||
at `large` tier, and small enough to be useful to a future caller.
|
||||
"""
|
||||
|
||||
perf_target: User
|
||||
perf_admin: User
|
||||
users: tuple[User, ...]
|
||||
groups: tuple[Group, ...]
|
||||
documents: tuple[Document, ...]
|
||||
tags: tuple[Tag, ...]
|
||||
correspondents: tuple[Correspondent, ...]
|
||||
document_types: tuple[DocumentType, ...]
|
||||
storage_paths: tuple[StoragePath, ...]
|
||||
documents: int
|
||||
tags: int
|
||||
correspondents: int
|
||||
document_types: int
|
||||
storage_paths: int
|
||||
|
||||
|
||||
def _grant_model_level_permissions(user) -> None:
|
||||
def _grant_model_level_permissions(user: User) -> None:
|
||||
"""
|
||||
Grant perf_target Django model-level view/add/change permissions on
|
||||
Document and Tag, on top of the per-object guardian grants seeding
|
||||
@@ -140,7 +145,9 @@ def _grant_model_level_permissions(user) -> None:
|
||||
user.user_permissions.add(*perms)
|
||||
|
||||
|
||||
def _create_users_and_groups(counts: _TierCounts):
|
||||
def _create_users_and_groups(
|
||||
counts: _TierCounts,
|
||||
) -> tuple[User, User, tuple[User, ...], tuple[Group, ...]]:
|
||||
from django.contrib.auth.models import Group
|
||||
|
||||
from documents.tests.factories import UserFactory
|
||||
@@ -192,7 +199,7 @@ def _create_lookup_tables(counts: _TierCounts):
|
||||
return tags, correspondents, document_types, storage_paths
|
||||
|
||||
|
||||
def _assign_owner(rng: random.Random, perf_target, other_users):
|
||||
def _assign_owner(rng: random.Random, perf_target: User, other_users: tuple[User, ...]):
|
||||
roll = rng.random()
|
||||
if roll < OWNED_BY_TARGET_FRACTION:
|
||||
return perf_target, "target"
|
||||
@@ -207,9 +214,18 @@ def _seed_documents(
|
||||
tags,
|
||||
correspondents,
|
||||
document_types,
|
||||
storage_paths,
|
||||
perf_target,
|
||||
other_users,
|
||||
):
|
||||
) -> tuple[int, list[int]]:
|
||||
"""
|
||||
Bulk-create `counts.documents` documents in chunks. Returns the total
|
||||
document count plus a lightweight list of pks for documents that ended
|
||||
up with an owner (target or other) -- that's all
|
||||
`_grant_general_permissions` needs to sample from, so full `Document`
|
||||
instances aren't accumulated across chunks (a real memory concern at the
|
||||
`large` tier's 360,000 documents).
|
||||
"""
|
||||
from django.contrib.auth.models import Permission
|
||||
from django.contrib.contenttypes.models import ContentType
|
||||
from guardian.models import UserObjectPermission
|
||||
@@ -220,6 +236,7 @@ def _seed_documents(
|
||||
tag_ids = [t.pk for t in tags]
|
||||
correspondent_ids = [c.pk for c in correspondents]
|
||||
document_type_ids = [d.pk for d in document_types]
|
||||
storage_path_ids = [s.pk for s in storage_paths]
|
||||
|
||||
doc_content_type = ContentType.objects.get_for_model(Document)
|
||||
view_perm = Permission.objects.get(
|
||||
@@ -232,7 +249,8 @@ def _seed_documents(
|
||||
)
|
||||
through_model = Document.tags.through
|
||||
|
||||
documents: list[Document] = []
|
||||
document_count = 0
|
||||
owned_document_pks: list[int] = []
|
||||
remaining = counts.documents
|
||||
while remaining > 0:
|
||||
chunk_n = min(CHUNK_SIZE, remaining)
|
||||
@@ -254,6 +272,18 @@ def _seed_documents(
|
||||
if document_type_ids and rng.random() < 0.6
|
||||
else None
|
||||
),
|
||||
storage_path_id=(
|
||||
rng.choice(storage_path_ids)
|
||||
if storage_path_ids and rng.random() < 0.8
|
||||
else None
|
||||
),
|
||||
# Document.created is a plain DateField (default: today).
|
||||
# Leaving it unset would give every seeded document the same
|
||||
# date, collapsing Document's ("-created",) ordering index
|
||||
# into a single-valued sort key -- spread it over a
|
||||
# realistic multi-year window instead.
|
||||
created=datetime.date.today()
|
||||
- datetime.timedelta(days=rng.randint(0, 365 * 3)),
|
||||
)
|
||||
owner, bucket = _assign_owner(rng, perf_target, other_users)
|
||||
doc.owner_id = owner.pk if owner else None
|
||||
@@ -272,6 +302,10 @@ def _seed_documents(
|
||||
|
||||
perm_rows = []
|
||||
for doc, bucket in zip(created, owner_buckets, strict=True):
|
||||
if bucket == "unowned":
|
||||
continue
|
||||
owned_document_pks.append(doc.pk)
|
||||
|
||||
if bucket != "other":
|
||||
continue
|
||||
if rng.random() >= SHARED_WITH_TARGET_FRACTION:
|
||||
@@ -296,13 +330,18 @@ def _seed_documents(
|
||||
if perm_rows:
|
||||
UserObjectPermission.objects.bulk_create(perm_rows, batch_size=CHUNK_SIZE)
|
||||
|
||||
documents.extend(created)
|
||||
log(f" {len(documents)}/{counts.documents} documents seeded")
|
||||
document_count += len(created)
|
||||
log(f" {document_count}/{counts.documents} documents seeded")
|
||||
|
||||
return tuple(documents)
|
||||
return document_count, owned_document_pks
|
||||
|
||||
|
||||
def _grant_general_permissions(rng: random.Random, documents, users, groups) -> None:
|
||||
def _grant_general_permissions(
|
||||
rng: random.Random,
|
||||
owned_document_pks: list[int],
|
||||
users,
|
||||
groups,
|
||||
) -> None:
|
||||
"""
|
||||
Layer realistic (issue #13276-derived) guardian permission-row ratios
|
||||
across owned documents for the general user/group pool, so `profile`
|
||||
@@ -316,8 +355,7 @@ def _grant_general_permissions(rng: random.Random, documents, users, groups) ->
|
||||
|
||||
from documents.models import Document
|
||||
|
||||
owned_documents = [d for d in documents if d.owner_id is not None]
|
||||
if not owned_documents or not users:
|
||||
if not owned_document_pks or not users:
|
||||
return
|
||||
|
||||
doc_content_type = ContentType.objects.get_for_model(Document)
|
||||
@@ -326,16 +364,16 @@ def _grant_general_permissions(rng: random.Random, documents, users, groups) ->
|
||||
content_type=doc_content_type,
|
||||
)
|
||||
|
||||
n_user_perms = round(len(owned_documents) * USER_PERM_ROWS_PER_DOC)
|
||||
n_user_perms = round(len(owned_document_pks) * USER_PERM_ROWS_PER_DOC)
|
||||
n_group_perms = (
|
||||
round(len(owned_documents) * GROUP_PERM_ROWS_PER_DOC) if groups else 0
|
||||
round(len(owned_document_pks) * GROUP_PERM_ROWS_PER_DOC) if groups else 0
|
||||
)
|
||||
|
||||
user_rows = [
|
||||
UserObjectPermission(
|
||||
permission=view_perm,
|
||||
content_type=doc_content_type,
|
||||
object_pk=str(rng.choice(owned_documents).pk),
|
||||
object_pk=str(rng.choice(owned_document_pks)),
|
||||
user=rng.choice(users),
|
||||
)
|
||||
for _ in range(n_user_perms)
|
||||
@@ -351,7 +389,7 @@ def _grant_general_permissions(rng: random.Random, documents, users, groups) ->
|
||||
GroupObjectPermission(
|
||||
permission=view_perm,
|
||||
content_type=doc_content_type,
|
||||
object_pk=str(rng.choice(owned_documents).pk),
|
||||
object_pk=str(rng.choice(owned_document_pks)),
|
||||
group=rng.choice(groups),
|
||||
)
|
||||
for _ in range(n_group_perms)
|
||||
@@ -379,28 +417,29 @@ def seed_benchmark_dataset(tier: Tier, *, seed: int = 42) -> SeededData:
|
||||
log(f"Seeding tier={tier!r}")
|
||||
perf_target, perf_admin, other_users, groups = _create_users_and_groups(counts)
|
||||
tags, correspondents, document_types, storage_paths = _create_lookup_tables(counts)
|
||||
documents = _seed_documents(
|
||||
document_count, owned_document_pks = _seed_documents(
|
||||
rng,
|
||||
counts,
|
||||
tags,
|
||||
correspondents,
|
||||
document_types,
|
||||
storage_paths,
|
||||
perf_target,
|
||||
other_users,
|
||||
)
|
||||
all_users = (perf_target, *other_users)
|
||||
_grant_general_permissions(rng, documents, all_users, groups)
|
||||
_grant_general_permissions(rng, owned_document_pks, all_users, groups)
|
||||
|
||||
log(f"Done. {len(documents)} documents seeded for tier={tier!r}.")
|
||||
log(f"Done. {document_count} documents seeded for tier={tier!r}.")
|
||||
|
||||
return SeededData(
|
||||
perf_target=perf_target,
|
||||
perf_admin=perf_admin,
|
||||
users=all_users,
|
||||
groups=groups,
|
||||
documents=documents,
|
||||
tags=tags,
|
||||
correspondents=correspondents,
|
||||
document_types=document_types,
|
||||
storage_paths=storage_paths,
|
||||
documents=document_count,
|
||||
tags=len(tags),
|
||||
correspondents=len(correspondents),
|
||||
document_types=len(document_types),
|
||||
storage_paths=len(storage_paths),
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user