Optimize NooBaa deletion paths #7262

tangledbytes · 2023-04-10T09:06:43Z

Explain the changes

This PR attempts to improve performance NooBaa delete paths by doing the following:

Change bucket_reclaimer approach to object deletion - instead of deleting objects 1 by 1, 1k objects are marked deleted at once, this significantly reduces DB calls.
Add index for objectparts table which significantly speed up the queries which DB cleaner uses to identify the object parts that needs to be deleted.
Add index for objectmultiparts table which significantly speed up the queries which DB cleaner uses to identify the object parts that needs to be deleted.
Forces use of indexes by altering selections in DB calls.

Analysis

analysis.md

Issues: Fixed #xxx / Gap #xxx

This was a customer issue where they noticed serious NooBaa degradation when a bucket with 3M objects was deleted.

Doc added/updated
Tests added

dannyzaken

@utkarsh-pro. At first look, It looks good. There are few things we need to get more info on:

How are the new queries behave with the indexes? We should run EXPLAIN on the new queries and ensure they hit the indexes correctly and do not require sorting.
Let's inspect other paths that go through the delete flows. mainly delete_multiple_objects, delete_object and lifecycle

tangledbytes · 2023-04-18T10:09:56Z

Folks, I have attached a analysis.md in the PR description which might be helpful in understanding several delete path queries performance.

nimrod-becker · 2023-04-18T14:40:41Z

@guymguym if you feel like taking a look

baum · 2023-04-18T14:58:45Z

src/server/bg_services/lifecycle.js

@@ -71,7 +71,7 @@ async function background_worker() {
        dbg.log0('LIFECYCLE READ BUCKETS configuration buckets:', system_store.data.buckets.map(e => e.name));
        for (const bucket of system_store.data.buckets) {
            dbg.log0('LIFECYCLE READ BUCKETS configuration bucket name:', bucket.name, "rules", bucket.lifecycle_configuration_rules);
-            if (!bucket.lifecycle_configuration_rules || bucket.deleting) return;
+            if (!bucket.lifecycle_configuration_rules || bucket.deleting) continue;


Looks like a bug fix, great catch 😀

baum

lgtm

dannyzaken · 2023-04-19T09:22:58Z

src/server/object_services/schemas/object_part_indexes.js

+        // This index is used for queries where we want to find all the chunks of a specific object
+        // which are already marked deleted.
+        fields: {
+            obj: 1,


@utkarsh-pro IIUC, this can also be achieved by removing the filter on deleted, Is that right? I wonder which approach is better, since adding indexes can affect writes performance. @guymguym any thoughts?

@dannyzaken, I think partial indexes are faster for both reads and writes.

One major reason for using a partial index is to avoid indexing common values. Since a query searching for a common value (one that accounts for more than a few percent of all the table rows) will not use the index anyway, there is no point in keeping those rows in the index at all. This reduces the size of the index, which will speed up those queries that do use the index. It will also speed up many table update operations because the index does not need to be updated in all cases. Example 11.1 shows a possible application of this idea.

Taken from: https://www.postgresql.org/docs/current/indexes-partial.html

Or did I... misunderstood the comment?

We discussed these new indexes on parts/multiparts and while there is an alternative to define the index only on the deleted field like in objectmds aggregate_by_delete_dates, this would mean that db_cleaner will cleanup parts and multiparts without specific relation to a deleted object, and this could be fine, but we don't have to do it now. The benefit is that the index will allow an index condition on the delete date, but the downside is that we will delete some parts and multiparts of objects and not all parts of the object, which might look a bit weird for other scanners (perhaps). In any case, this looks fine as we have to have some index for deletion, and this index will be limited by the amount of pending rows to deleted (the db_cleaner will keep it small).

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> improve DB Cleanup index hits Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> optimize multipart deletions Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>

guymguym · 2023-04-24T07:04:42Z

src/server/object_services/object_server.js

+    // find_objects will ensure that it does not return any object
+    // which is already marked for deletion and that's all we care
+    // about here.
+    const { objects } = await MDStore.instance().find_objects({


better add a comment/hint that this uses the objectmd index with just bucket field and deleted condition -

noobaa-core/src/server/object_services/schemas/object_md_indexes.js

Lines 135 to 147 in d3091b6

{

// TODO index ??? count_objects_of_bucket()

// TODO index ??? count_objects_per_bucket()

fields: {

bucket: 1,

},

options: {

unique: false,

partialFilterExpression: {

deleted: null,

}

}

},

guymguym · 2023-04-24T07:05:29Z

src/server/object_services/object_server.js

+    // delete the objects
+    await MDStore.instance().remove_objects_and_unset_latest(objects);
+
+    const bucket_has_objects = await MDStore.instance().has_any_objects_for_bucket(bucket_id);


We can remove this and just check the objects.length.
Both here and also in the previous function delete_multiple_objects_by_filter().

guymguym · 2023-04-24T07:08:29Z

src/server/object_services/md_store.js

    count_total_objects() {
        return this._objects.countDocuments({}); // maybe estimatedDocumentCount()
    }
+
+    estimated_total_objects() {
+        return this._objects.estimatedDocumentCount();
+    }


Maybe best remove the unused count_total_objects().
Also the comment is now definitely redundant (// maybe estimatedDocumentCount())

guymguym · 2023-04-24T07:12:10Z

src/server/bg_services/db_cleaner.js

@@ -51,7 +51,7 @@ async function background_worker() {
 }

 async function clean_md_store(last_date_to_remove) {
-    const total_objects_count = await MDStore.instance().count_total_objects();
+    const total_objects_count = await MDStore.instance().estimated_total_objects();


Perhaps we can check the estimate of deleted objects here instead of the total?
We have this index but we need to see if the database has an estimate per index -

noobaa-core/src/server/object_services/schemas/object_md_indexes.js

Lines 115 to 127 in d3091b6

{

// aggregate_objects_by_delete_dates()

fields: {

deleted: 1,

},

options: {

unique: false,

name: "aggregate_by_delete_dates",

partialFilterExpression: {

deleted: { $exists: true }

}

}

},

guymguym · 2023-04-24T07:31:03Z

src/server/object_services/md_store.js

    async find_deleted_objects(max_delete_time, limit) {
        const objects = await this._objects.find({
-            deleted: { $lt: new Date(max_delete_time) },
+            deleted: {
+                $lt: new Date(max_delete_time),
+                $exists: true // This forces the index to be used
+            },


This doesn't use an index condition on the deleted date, and will continue scanning throught the index and filtering by it. This seems to be related to how this index is defined to use a to_ts() function. Here are the two different explains:

currently:

nbcore=> EXPLAIN SELECT * from objectmds WHERE data->'deleted'<'"2023-04-14T21:52:00.909Z"'::jsonb AND data ? 'deleted' LIMIT 1000; QUERY PLAN ------------------------------------------------------------------------------------------------------------------ Limit (cost=0.00..3.47 rows=1 width=430) -> Seq Scan on objectmds (cost=0.00..3.47 rows=1 width=430) Filter: ((data ? 'deleted'::text) AND ((data -> 'deleted'::text) < '"2023-04-14T21:52:00.909Z"'::jsonb)) (3 rows)

with to_ts():

nbcore=> EXPLAIN SELECT * from objectmds WHERE to_ts(data->>'deleted'::text) < to_ts('2023-04-14T21:52:00.909Z') AND data ? 'deleted' LIMIT 1000; QUERY PLAN ----------------------------------------------------------------------------------------------------------------------- Limit (cost=0.14..8.15 rows=1 width=430) -> Index Scan using idx_btree_objectmds_aggregate_by_delete_dates on objectmds (cost=0.14..8.15 rows=1 width=430) Index Cond: (to_ts((data ->> 'deleted'::text)) < '2023-04-14 21:52:00.909'::timestamp without time zone) (3 rows)

guymguym · 2023-04-24T07:47:41Z

src/server/object_services/schemas/object_part_indexes.js

+        // This index is used for queries where we want to find all the chunks of a specific object
+        // which are already marked deleted.
+        fields: {
+            obj: 1,


We discussed these new indexes on parts/multiparts and while there is an alternative to define the index only on the deleted field like in objectmds aggregate_by_delete_dates, this would mean that db_cleaner will cleanup parts and multiparts without specific relation to a deleted object, and this could be fine, but we don't have to do it now. The benefit is that the index will allow an index condition on the delete date, but the downside is that we will delete some parts and multiparts of objects and not all parts of the object, which might look a bit weird for other scanners (perhaps). In any case, this looks fine as we have to have some index for deletion, and this index will be limited by the amount of pending rows to deleted (the db_cleaner will keep it small).

pull-request-size bot added the size/L label Apr 10, 2023

tangledbytes requested a review from dannyzaken April 10, 2023 09:43

tangledbytes marked this pull request as ready for review April 10, 2023 12:23

tangledbytes requested review from a team and aspandey and removed request for a team April 10, 2023 12:23

dannyzaken reviewed Apr 10, 2023

View reviewed changes

tangledbytes force-pushed the utkarsh-pro/improve/bucket_reclaimer branch from 9852ede to f0ed7ed Compare April 17, 2023 09:22

tangledbytes changed the title ~~improve bucket_recalimer performance and DB Cleanup performance~~ Optimize NooBaa deletion paths Apr 17, 2023

dannyzaken requested a review from baum April 18, 2023 07:10

tangledbytes force-pushed the utkarsh-pro/improve/bucket_reclaimer branch 2 times, most recently from d66eeb9 to 7390ca9 Compare April 18, 2023 14:20

nimrod-becker requested a review from guymguym April 18, 2023 14:40

baum reviewed Apr 18, 2023

View reviewed changes

baum approved these changes Apr 18, 2023

View reviewed changes

dannyzaken reviewed Apr 19, 2023

View reviewed changes

dannyzaken approved these changes Apr 20, 2023

View reviewed changes

tangledbytes force-pushed the utkarsh-pro/improve/bucket_reclaimer branch from 7390ca9 to 8688995 Compare April 20, 2023 12:06

tangledbytes added 2 commits April 20, 2023 20:20

fix lifecycle BG skipping buckets bug

75de2be

Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>

tangledbytes force-pushed the utkarsh-pro/improve/bucket_reclaimer branch from 8688995 to 75de2be Compare April 20, 2023 14:50

tangledbytes merged commit 1614d64 into noobaa:master Apr 20, 2023
6 checks passed

dannyzaken mentioned this pull request Apr 23, 2023

[Backport to 5.11_special_fix] - Optimize NooBaa deletion paths #7278

Merged

2 tasks

guymguym reviewed Apr 24, 2023

View reviewed changes

Optimize NooBaa deletion paths #7262

Optimize NooBaa deletion paths #7262

tangledbytes commented Apr 10, 2023 •

edited

dannyzaken left a comment

tangledbytes commented Apr 18, 2023

nimrod-becker commented Apr 18, 2023

baum Apr 18, 2023

baum left a comment

dannyzaken Apr 19, 2023

tangledbytes Apr 20, 2023

tangledbytes Apr 20, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

guymguym Apr 24, 2023

	{
	// TODO index ??? count_objects_of_bucket()
	// TODO index ??? count_objects_per_bucket()
	fields: {
	bucket: 1,
	},
	options: {
	unique: false,
	partialFilterExpression: {
	deleted: null,
	}
	}
	},

	{
	// aggregate_objects_by_delete_dates()
	fields: {
	deleted: 1,
	},
	options: {
	unique: false,
	name: "aggregate_by_delete_dates",
	partialFilterExpression: {
	deleted: { $exists: true }
	}
	}
	},

Optimize NooBaa deletion paths #7262

Optimize NooBaa deletion paths #7262

Conversation

tangledbytes commented Apr 10, 2023 • edited

Explain the changes

Analysis

Issues: Fixed #xxx / Gap #xxx

dannyzaken left a comment

Choose a reason for hiding this comment

tangledbytes commented Apr 18, 2023

nimrod-becker commented Apr 18, 2023

Choose a reason for hiding this comment

baum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangledbytes commented Apr 10, 2023 •

edited