mikebridge commented on code in PR #44251:
URL: https://github.com/apache/superset/pull/44251#discussion_r4039659177
##########
superset/versioning/restore.py:
##########
@@ -71,6 +73,205 @@
}
+class PrunedChildHistoryError(Exception):
+ """The target version's child history is no longer fully recoverable.
+
+ Version-history retention prunes closed child shadow rows (and their
+ ``version_transaction`` rows) once they age out; a restore that
+ proceeded anyway would persist an INCOMPLETE column/metric set for a
+ ``SqlaTable`` — a durable partial write. Restore fails closed
+ instead (sc-120012). The message is user-facing.
+ """
+
+ def __init__(self, model_name: str, detail: str) -> None:
+ super().__init__(
+ f"This {model_name} version can no longer be fully restored: "
+ f"{detail} needed by the snapshot were pruned by "
+ "version-history retention. The entity was left unchanged."
+ )
+
+
+def _verify_child_history_complete(entity: Any, target_tx: int) -> None:
+ """Refuse the restore when a needed child shadow row was pruned.
+
+ ``revert(relations=...)`` reconstructs a ``SqlaTable``'s columns and
+ metrics from the child shadow rows valid at *target_tx*. Retention
+ can have pruned exactly those rows while the parent's row at
+ *target_tx* survives; the pruner also deletes the covering
+ ``version_transaction`` rows (change records cascade with them), so
+ the only surviving evidence is the validity chain itself. Per child
+ (grouped by the child's own ``id``), the state at *target_tx* is
+ PROVABLE when a surviving row's validity interval covers it (a
+ non-DELETE covering row: present, restored; a DELETE covering row:
+ provably absent). Without a covering interval, a last same-parent
+ row at/before the target that is DELETE is accepted as absence.
+ If no row is at/before the target, an earliest surviving INSERT
+ is accepted as born-after evidence, not proof of the original birth. See
+ :func:`_child_state_provable_at` for the interval semantics.
+
+ Anything else means a pruned row MAY have covered ``target_tx`` —
+ fail closed (sc-120012).
+
+ Known limitation (ratified, sc-120012): the fail-closed guard refuses
+ every DETECTABLE pruning of needed child history, including a pruned
+ closed row whose successor survives, and protects all restores
+ targeting versions within the retention window. Missing evidence can
+ be indistinguishable from legitimate absence in three shapes:
+
+ * a column/metric deleted AFTER the target version whose entire
+ shadow chain — including its closing DELETE row — has aged out of
+ retention leaves no surviving evidence anywhere;
+ * an erased same-parent re-birth window leaves an earlier terminal
+ DELETE, indistinguishable from a purged foreign incarnation;
+ * erased birth and covering rows leave only a later re-insertion
+ INSERT, indistinguishable from a child first born after the target.
+
+ No read-side check can recover that erased evidence. The pruner
+ deletes shadow rows by create-tx OR close-tx
+ (``tasks/version_history_retention.py`` ``_delete_for_transactions``)
+ and then the ``version_transaction`` rows themselves, cascading the
+ change records (same module, transaction-delete step) — and change
+ records are per-tx DIFFS (``versioning/changes/table.py`` path /
+ from_value / to_value columns), never a full child set, so replay
+ cannot reconstruct what pruning erased. Only reachable for targets
+ older than the retention window (the closing DELETE row's own tx must
+ itself have been prunable). Dependency-closure retention is the
+ separate post-GA sc-120945 follow-up, not implemented by this guard.
+ """
+ # pylint: disable=import-outside-toplevel
+ # Local imports: the models pull in the initialised-app graph (same
+ # bootstrap-cycle rationale as the other deferred imports here).
+ from superset.connectors.sqla.models import SqlMetric, TableColumn
+
+ # SQLite drops FOR UPDATE, and pysqlite's legacy mode starts no real
+ # transaction for SELECTs — the verification reads and the reverter's
+ # re-reads could straddle a concurrent commit, and the write lock
+ # would otherwise arrive only at the deferred flush. Reserve the
+ # write lock up front (BEGIN IMMEDIATE) so no other writer can commit
+ # between the check and the restore's own commit; the read-to-write
+ # upgrade conflict cannot occur because the reservation precedes
+ # every read. No-op when the driver is already in a transaction.
+ if db.engine.dialect.name == "sqlite":
+ conn: sa.engine.Connection = db.session.connection()
+ if not conn.connection.dbapi_connection.in_transaction:
+ # Issued through the SQLAlchemy Connection (not the raw DBAPI
+ # handle) so exception translation applies: reservation
+ # contention surfaces as sqlalchemy.exc.OperationalError,
+ # which the command's @transaction wraps into its failure
+ # type and the endpoint maps to 422 — a raw sqlite3 error
+ # would escape both as a 500.
+ conn.exec_driver_sql("BEGIN IMMEDIATE")
+
+ missing: list[str] = []
+ for label, child_cls in (("column", TableColumn), ("metric", SqlMetric)):
+ shadow: sa.Table = version_class(child_cls).__table__
+ # One locked read: EVERY surviving row the verdict depends on is
+ # taken FOR UPDATE in the transaction that owns the restore —
+ # covering non-DELETE rows (the reconstruction inputs) AND
+ # covering DELETE witnesses. The witnesses are load-bearing for
+ # the WRITE as well as the verdict: Continuum's one-to-many
+ # reconstruction selects the latest surviving row at/before the
+ # target and then excludes DELETEs, so a pruner deleting a
+ # covering DELETE witness mid-restore would resurrect an older
+ # incarnation the target never had. The row locks block the
+ # pruner's DELETE until this transaction commits (its
+ # SERIALIZABLE pass waits or retries). On SQLite the BEGIN
+ # IMMEDIATE reservation above provides the equivalent.
+ rows: Sequence[sa.engine.Row[Any]] = db.session.execute(
+ sa.select(
+ shadow.c.id,
+ shadow.c.transaction_id,
+ shadow.c.end_transaction_id,
+ shadow.c.operation_type,
+ )
+ .where(shadow.c.table_id == entity.id)
+ .order_by(shadow.c.id, shadow.c.transaction_id)
+ .with_for_update()
+ ).all()
+ by_child: dict[int, list[Any]] = {}
+ for row in rows:
+ by_child.setdefault(row.id, []).append(row)
+
+ for child_id, child_rows in by_child.items():
+ if not _child_state_provable_at(child_rows, target_tx):
+ missing.append(f"{label} id={child_id}")
+ # The refusal is fail-closed by design; the chain dump is
+ # what lets an operator (or CI) see WHY this child's state
+ # at the target is unprovable from the surviving intervals.
+ logger.warning(
+ "versioning: restore refused for %s id=%s at tx=%s — "
+ "%s id=%s surviving chain=%s",
+ type(entity).__name__,
+ entity.id,
+ target_tx,
+ label,
+ child_id,
+ [
+ (
+ row.transaction_id,
+ row.end_transaction_id,
+ row.operation_type,
+ )
+ for row in child_rows
+ ],
+ )
+
+ if missing:
+ raise PrunedChildHistoryError(
+ type(entity).__name__,
+ f"{len(missing)} column/metric history row(s) ({',
'.join(missing[:10])})",
+ )
+
+
+def _child_state_provable_at(rows: list[Any], target_tx: int) -> bool:
+ """Whether *rows* (one child's surviving SAME-PARENT shadow rows,
+ tx-ordered) prove the child's state at *target_tx*.
+
+ Interval semantics: a row is valid over ``[transaction_id,
+ end_transaction_id)`` (open end = unbounded). A covering interval
+ proves the state (non-DELETE: present, restored; DELETE: provably
+ absent). With no covering interval the verdict rests on the LAST
+ same-parent row at/before the target:
+
+ * none at all → born-after: provable only when the earliest
+ surviving row is INSERT (original birth and re-insertion cannot
+ be distinguished if earlier history was erased);
+ * a DELETE → provably absent, UNCONDITIONALLY — even with a closed
+ end whose closer row no longer survives. Two invariants make this
+ sound (ratified, sc-120012): retention cannot erase the closer of
+ a surviving closed DELETE (the pruner deletes rows whose CLOSE
+ transaction is pruned, so this DELETE row would have gone with
+ it), and purge never touches the live parent's own rows — so a
+ missing closer is a purged FOREIGN incarnation of a recycled id,
+ not this parent's history;
+ * a non-DELETE whose interval expired before the target → its
+ same-parent successor is missing (that row's close-tx was pruned
+ while its own create-tx survived): the child may have existed at
+ the target — fail closed. This is the classic pruned-history
+ shape the guard exists for.
+
+ Known limitation (ratified): a same-parent re-birth whose window
+ ``[X, e)`` was retention-pruned via ``e`` while ``X`` survived is
+ observationally identical to a purged foreign closer, and is
+ accepted as absence. Only reachable for targets inside a
+ retention-pruned window (older than the cutoff); the consequence is
+ a silently omitted column that existed only in that window. Closing
+ it is sc-120945's separate post-GA dependency-closure retention work.
+ Similarly, if only a later re-insertion INSERT survives, the born-after
+ branch cannot distinguish it from the child's original birth.
+ """
+ for row in rows:
+ if row.transaction_id <= target_tx and (
+ row.end_transaction_id is None or row.end_transaction_id >
target_tx
+ ):
+ return True
+ at_or_before: list[Any] = [row for row in rows if row.transaction_id <=
target_tx]
+ if not at_or_before:
+ return rows[0].operation_type == OPERATION_INSERT
Review Comment:
Good catch — addressed in b3f7072f606493cb107f556436aec633cf9c90da using the
explicit earliest-transaction selection. The production ORDER BY stays intact.
Two unsorted-input cases have independent literal expectations: earliest INSERT
proves born-after, while a later INSERT must not conceal an earliest UPDATE.
Both failed against rows[0] (one false refusal, one false acceptance); a real
revert control reproduced both failures, and the restored final file passes 28
tests. No test/helper sorts the inputs. Full changed-file pre-commit, including
MyPy/pylint, and final-source independent review passed; new-head CI remains
pending. I did not rerun the full 180-test ORDER BY-removal experiment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
