From 48937cd18360b52d8205d5059a5cb079ea4897a4 Mon Sep 17 00:00:00 2001 From: Matthew Oliver Date: 2025年11月25日 14:56:52 +0000 Subject: [PATCH] TombstoneReclaimer: quote string timestamp in sql query Reclaim age and sync "timestamp" args passed to DatabaseBroker.reclaim() and TombstoneReclaimer.__init__() are expected to be floats. This is adhered to in code, but many tests pass a string representation of a Timestamp instance. The string has been tolerated because it has so far always been identical to a str(float). However, if a Timestamp instance has an offset then its string representation is not equivalent to str(float), and TombstoneReclaimer.reclaim() would raise an error because the reclaim age value in the SQL query is not quoted and does not appear to be any recognised type. This patch quotes the reclaim age in the TombstoneReclaimer SQL query. This makes it tolerant of being passed string representations of Timestamps with offsets. This is not yet expected, but, nevertheless, text in SQL queries should be quoted. Change-Id: Ic70689db1e4ddc6f526c283bf0cbb5862b16fd90 Signed-off-by: Alistair Coles Co-Authored-By: Alistair Coles --- swift/common/db.py | 21 +++++++++++++++++---- swift/container/backend.py | 10 ++++++++++ test/unit/common/test_db.py | 18 +++++++++++++++++- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/swift/common/db.py b/swift/common/db.py index f8c0648379..f1ddd1f431 100644 --- a/swift/common/db.py +++ b/swift/common/db.py @@ -252,7 +252,7 @@ class TombstoneReclaimer(object): ''' % self.broker.db_contains_type self.clean_batch_query = ''' DELETE FROM %s WHERE deleted = 1 - AND name>= ? AND %s < %s + AND name>= ? AND %s < '%s' ''' % (self.broker.db_contains_type, self.broker.db_reclaim_timestamp, self.age_timestamp) @@ -1053,8 +1053,10 @@ class DatabaseBroker(object): Subclasses may reclaim other items by overriding :meth:`_reclaim`. - :param age_timestamp: max created_at timestamp of object rows to delete - :param sync_timestamp: max update_at timestamp of sync rows to delete + :param age_timestamp: (float) the max created_at timestamp of object + rows to delete + :param sync_timestamp: (float) the max update_at timestamp of sync rows + to delete """ if not self._skip_commit_puts(): with lock_parent_directory(self.pending_file, @@ -1072,11 +1074,22 @@ class DatabaseBroker(object): """ This is only called once at the end of reclaim after tombstone reclaim has been completed. + + :param conn: db connection + :param age_timestamp: (float) the max created_at timestamp of object + rows to delete + :param sync_timestamp: (float) the max update_at timestamp of sync rows + to delete """ self._reclaim_sync(conn, sync_timestamp) self._reclaim_metadata(conn, age_timestamp) def _reclaim_sync(self, conn, sync_timestamp): + """ + :param conn: db connection + :param sync_timestamp: (float) the max update_at timestamp of sync rows + to delete + """ try: conn.execute(''' DELETE FROM outgoing_sync WHERE updated_at < ? @@ -1098,7 +1111,7 @@ class DatabaseBroker(object): from other related functions. :param conn: Database connection to reclaim metadata within. - :param timestamp: Empty metadata items last updated before this + :param timestamp: (float) Empty metadata items last updated before this timestamp will be removed. :returns: True if conn.commit() should be called """ diff --git a/swift/container/backend.py b/swift/container/backend.py index da0632fb24..2971ba873a 100644 --- a/swift/container/backend.py +++ b/swift/container/backend.py @@ -1662,6 +1662,16 @@ class ContainerBroker(DatabaseBroker): ''' % SHARD_RANGE_TABLE) def _reclaim_other_stuff(self, conn, age_timestamp, sync_timestamp): + """ + This is only called once at the end of reclaim after tombstone reclaim + has been completed. + + :param conn: db connection + :param age_timestamp: (float) the max created_at timestamp of object + rows to delete + :param sync_timestamp: (float) the max update_at timestamp of sync rows + to delete + """ super(ContainerBroker, self)._reclaim_other_stuff( conn, age_timestamp, sync_timestamp) # populate instance cache, but use existing conn to avoid deadlock diff --git a/test/unit/common/test_db.py b/test/unit/common/test_db.py index dbe60308e8..ee460a723a 100644 --- a/test/unit/common/test_db.py +++ b/test/unit/common/test_db.py @@ -1676,7 +1676,7 @@ class TestTombstoneReclaimer(TestDbBase): Timestamp(top_of_the_minute - (i * 60)), True) self._make_object( broker, 'a_%3d' % (559 - i if reverse_names else i + 1), - Timestamp(top_of_the_minute - ((i + 1) * 60)), False) + Timestamp(top_of_the_minute - ((i + 1) * 60), offset=1), False) self._make_object( broker, 'b_%3d' % (560 - i if reverse_names else i), Timestamp(top_of_the_minute - ((i + 2) * 60)), True) @@ -1792,6 +1792,22 @@ class TestTombstoneReclaimer(TestDbBase): self.assertEqual(0, self._get_reclaimable(broker, reclaim_age)) self.assertEqual(140, actual) + def test_reclaim_with_timestamp_with_offset(self): + broker, totm, reclaim_age = self._setup_tombstones() + + age_timestamp = Timestamp(reclaim_age, offset=0xabcd) + reclaimer = TombstoneReclaimer(broker, age_timestamp.internal) + reclaimer.reclaim() + + self.assertEqual(0, self._get_reclaimable(broker, reclaim_age)) + tombstones = self._get_reclaimable(broker, totm + 1) + self.assertEqual(140, tombstones) + # in this scenario the reclaim phase all tombstones (140) + self.assertEqual(0, reclaimer.remaining_tombstones) + # get_tombstone_count finds the rest + actual = reclaimer.get_tombstone_count() + self.assertEqual(140, actual) + if __name__ == '__main__': unittest.main()

AltStyle によって変換されたページ (->オリジナル) /