Fix 1.37 crashes (#14081)

* Wait for pending read to complete before destroying the page

* fix page alignment crash

* Compare copy of descriptor

* prevent workers crashes by disabling cancellability on critical areas and separate sqlite3 statistics to its own worker job

* do not update sqlite3 stats when they are slow

* do not query sqlite3 statistics when they are slow

* flipped condition

* sqlite3 proper timeout calculation

Co-authored-by: Costa Tsaousis <costa@netdata.cloud>

7 files changed, 78 insertions, 21 deletions

diff --git a/database/engine/pagecache.c b/database/engine/pagecache.c
index d65cb35a57..5e5766d2f1 100644
--- a/database/engine/pagecache.c
+++ b/database/engine/pagecache.c
@@ -524,6 +524,14 @@ uint8_t pg_cache_punch_hole(struct rrdengine_instance *ctx, struct rrdeng_page_d
     }
     rrdeng_page_descr_mutex_unlock(ctx, descr);
 
+    while (unlikely(pg_cache_descr->flags & RRD_PAGE_READ_PENDING)) {
+        error_limit_static_global_var(erl, 1, 0);
+        error_limit(&erl, "%s: Found page with READ PENDING, waiting for read to complete", __func__);
+        if (unlikely(debug_flags & D_RRDENGINE))
+            print_page_cache_descr(descr, "", true);
+        pg_cache_wait_event_unsafe(descr);
+    }
+
     if (pg_cache_descr->flags & RRD_PAGE_POPULATED) {
         /* only after locking can it be safely deleted from LRU */
         pg_cache_replaceQ_delete(ctx, descr);
diff --git a/database/engine/rrdengine.c b/database/engine/rrdengine.c
index e4cd37e989..a6840f38cf 100644
--- a/database/engine/rrdengine.c
+++ b/database/engine/rrdengine.c
@@ -272,9 +272,19 @@ static void fill_page_with_nulls(void *page, uint32_t page_length, uint8_t type)
     }
 }
 
+struct rrdeng_page_descr *get_descriptor(struct pg_cache_page_index *page_index, time_t start_time_s)
+{
+    uv_rwlock_rdlock(&page_index->lock);
+    Pvoid_t *PValue = JudyLGet(page_index->JudyL_array, start_time_s, PJE0);
+    struct rrdeng_page_descr *descr = unlikely(NULL == PValue) ? NULL : *PValue;
+    uv_rwlock_rdunlock(&page_index->lock);
+    return descr;
+};
+
 static void do_extent_processing (struct rrdengine_worker_config *wc, struct extent_io_descriptor *xt_io_descr, bool read_failed)
 {
     struct rrdengine_instance *ctx = wc->ctx;
+    struct page_cache *pg_cache = &ctx->pg_cache;
     struct rrdeng_page_descr *descr;
     struct page_cache_descr *pg_cache_descr;
     int ret;
@@ -365,19 +375,30 @@ after_crc_check:
         }
     }
 
+    uv_rwlock_rdlock(&pg_cache->metrics_index.lock);
+    Pvoid_t *PValue = JudyHSGet(pg_cache->metrics_index.JudyHS_array, xt_io_descr->descr_array[0]->id, sizeof(uuid_t));
+    struct pg_cache_page_index *page_index = likely( NULL != PValue) ? *PValue : NULL;
+    uv_rwlock_rdunlock(&pg_cache->metrics_index.lock);
+
+
     for (i = 0, page_offset = 0; i < count; page_offset += header->descr[i++].page_length) {
         uint8_t is_prefetched_page;
         descr = NULL;
         for (j = 0 ; j < xt_io_descr->descr_count; ++j) {
-            struct rrdeng_page_descr *descrj;
+            struct rrdeng_page_descr descrj;
 
-            descrj = xt_io_descr->descr_array[j];
+            descrj = xt_io_descr->descr_read_array[j];
             /* care, we don't hold the descriptor mutex */
-            if (!uuid_compare(*(uuid_t *) header->descr[i].uuid, *descrj->id) &&
-                header->descr[i].page_length == descrj->page_length &&
-                header->descr[i].start_time_ut == descrj->start_time_ut &&
-                header->descr[i].end_time_ut == descrj->end_time_ut) {
-                descr = descrj;
+            if (!uuid_compare(*(uuid_t *) header->descr[i].uuid, *descrj.id) &&
+                header->descr[i].page_length == descrj.page_length &&
+                header->descr[i].start_time_ut == descrj.start_time_ut &&
+                header->descr[i].end_time_ut == descrj.end_time_ut) {
+                //descr = descrj;
+                descr = get_descriptor(page_index, (time_t) (descrj.start_time_ut / USEC_PER_SEC));
+                if (unlikely(!descr)) {
+                    error_limit_static_thread_var(erl, 1, 0);
+                    error_limit(&erl, "%s: Required descriptor is not in the page index anymore", __FUNCTION__);
+                }
                 break;
             }
         }
@@ -506,6 +527,7 @@ static void do_read_extent(struct rrdengine_worker_config* wc,
         pg_cache_descr->flags |= RRD_PAGE_READ_PENDING;
         rrdeng_page_descr_mutex_unlock(ctx, descr[i]);
         xt_io_descr->descr_array[i] = descr[i];
+        xt_io_descr->descr_read_array[i] = *(descr[i]);
     }
     xt_io_descr->descr_count = count;
     xt_io_descr->file = datafile->file;
diff --git a/database/engine/rrdengine.h b/database/engine/rrdengine.h
index fedadbe861..aca8184380 100644
--- a/database/engine/rrdengine.h
+++ b/database/engine/rrdengine.h
@@ -117,6 +117,7 @@ struct extent_io_descriptor {
     unsigned descr_count;
     int release_descr;
     struct rrdeng_page_descr *descr_array[MAX_PAGES_PER_EXTENT];
+    struct rrdeng_page_descr descr_read_array[MAX_PAGES_PER_EXTENT];
     Word_t descr_commit_idx_array[MAX_PAGES_PER_EXTENT];
     struct extent_io_descriptor *next; /* multiple requests to be served by the same cached extent */
 };
diff --git a/database/engine/rrdengineapi.c b/database/engine/rrdengineapi.c
index 27503baee1..6f2a32777e 100755
--- a/database/engine/rrdengineapi.c
+++ b/database/engine/rrdengineapi.c
@@ -39,20 +39,39 @@ uint8_t rrdeng_drop_metrics_under_page_cache_pressure = 1;
 // ----------------------------------------------------------------------------
 // metrics groups
 
+static inline void rrdeng_page_alignment_acquire(struct pg_alignment *pa) {
+    if(unlikely(!pa)) return;
+    __atomic_add_fetch(&pa->refcount, 1, __ATOMIC_SEQ_CST);
+}
+
+static inline bool rrdeng_page_alignment_release(struct pg_alignment *pa) {
+    if(unlikely(!pa)) return true;
+
+    if(__atomic_sub_fetch(&pa->refcount, 1, __ATOMIC_SEQ_CST) == 0) {
+        freez(pa);
+        return true;
+    }
+
+    return false;
+}
+
+// charts call this
 STORAGE_METRICS_GROUP *rrdeng_metrics_group_get(STORAGE_INSTANCE *db_instance __maybe_unused, uuid_t *uuid __maybe_unused) {
-    return callocz(1, sizeof(struct pg_alignment));
+    struct pg_alignment *pa = callocz(1, sizeof(struct pg_alignment));
+    rrdeng_page_alignment_acquire(pa);
+    return (STORAGE_METRICS_GROUP *)pa;
 }
 
+// charts call this
 void rrdeng_metrics_group_release(STORAGE_INSTANCE *db_instance, STORAGE_METRICS_GROUP *smg) {
-    if(!smg) return;
+    if(unlikely(!smg)) return;
 
     struct rrdengine_instance *ctx = (struct rrdengine_instance *)db_instance;
     struct pg_alignment *pa = (struct pg_alignment *)smg;
     struct page_cache *pg_cache = &ctx->pg_cache;
 
     uv_rwlock_rdlock(&pg_cache->metrics_index.lock);
-    if(pa->refcount == 0)
-        freez(pa);
+    rrdeng_page_alignment_release(pa);
     uv_rwlock_rdunlock(&pg_cache->metrics_index.lock);
 }
 
@@ -134,12 +153,13 @@ STORAGE_METRIC_HANDLE *rrdeng_metric_get(STORAGE_INSTANCE *db_instance, uuid_t *
         __atomic_add_fetch(&page_index->refcount, 1, __ATOMIC_SEQ_CST);
 
         if(pa) {
-            if(page_index->alignment && page_index->alignment != pa && page_index->writers > 0)
-                fatal("DBENGINE: page_index has a different alignment (page_index refcount is %u, writers is %u).",
-                        page_index->refcount, page_index->writers);
+            if(page_index->alignment != pa) {
+                if(!rrdeng_page_alignment_release(page_index->alignment)) // NULL is ok
+                    error("DBENGINE: metric switched alignment, but the previous is still used.");
 
-            page_index->alignment = pa;
-            __atomic_add_fetch(&pa->refcount, 1, __ATOMIC_SEQ_CST);
+                rrdeng_page_alignment_acquire(pa);
+                page_index->alignment = pa;
+            }
         }
     }
 
@@ -162,8 +182,7 @@ STORAGE_METRIC_HANDLE *rrdeng_metric_create(STORAGE_INSTANCE *db_instance, uuid_
     pg_cache->metrics_index.last_page_index = page_index;
     page_index->alignment = pa;
     page_index->refcount = 1;
-    if(pa)
-        pa->refcount++;
+    rrdeng_page_alignment_acquire(pa);
     uv_rwlock_wrunlock(&pg_cache->metrics_index.lock);
 
     return (STORAGE_METRIC_HANDLE *)page_index;
@@ -532,6 +551,9 @@ int rrdeng_store_metric_finalize(STORAGE_COLLECT_HANDLE *collection_handle) {
     uv_rwlock_wrlock(&page_index->lock);
     if (!--page_index->writers && !page_index->page_count) {
         can_delete_metric = 1;
+
+        rrdeng_page_alignment_release(page_index->alignment);
+        page_index->alignment = NULL;
     }
     uv_rwlock_wrunlock(&page_index->lock);
     freez(handle);
diff --git a/database/rrddim.c b/database/rrddim.c
index 1b3d9952c9..f3aeb26371 100644
--- a/database/rrddim.c
+++ b/database/rrddim.c
@@ -195,19 +195,19 @@ static void rrddim_delete_callback(const DICTIONARY_ITEM *item __maybe_unused, v
 
     debug(D_RRD_CALLS, "rrddim_free() %s.%s", rrdset_name(st), rrddim_name(rd));
 
-    size_t tiers_available = 0, tiers_said_yes = 0;
+    size_t tiers_available = 0, tiers_said_no_retention = 0;
     for(size_t tier = 0; tier < storage_tiers ;tier++) {
         if(rd->tiers[tier] && rd->tiers[tier]->db_collection_handle) {
             tiers_available++;
 
             if(rd->tiers[tier]->collect_ops->finalize(rd->tiers[tier]->db_collection_handle))
-                tiers_said_yes++;
+                tiers_said_no_retention++;
 
             rd->tiers[tier]->db_collection_handle = NULL;
         }
     }
 
-    if (tiers_available == tiers_said_yes && tiers_said_yes && rd->rrd_memory_mode == RRD_MEMORY_MODE_DBENGINE) {
+    if (tiers_available == tiers_said_no_retention && tiers_said_no_retention && rd->rrd_memory_mode == RRD_MEMORY_MODE_DBENGINE) {
         /* This metric has no data and no references */
         metaqueue_delete_dimension_uuid(&rd->metric_uuid);
     }
diff --git a/database/sqlite/sqlite_context.c b/database/sqlite/sqlite_context.c
index 9c7a61c6e0..deca84584b 100644
--- a/database/sqlite/sqlite_context.c
+++ b/database/sqlite/sqlite_context.c
@@ -449,7 +449,9 @@ skip_delete:
 int sql_context_cache_stats(int op)
 {
     int count, dummy;
+    netdata_thread_disable_cancelability();
     sqlite3_db_status(db_context_meta, op, &count, &dummy, 0);
+    netdata_thread_enable_cancelability();
     return count;
 }
 
diff --git a/database/sqlite/sqlite_functions.c b/database/sqlite/sqlite_functions.c
index eeb3c38229..ce5487fbfc 100644
--- a/database/sqlite/sqlite_functions.c
+++ b/database/sqlite/sqlite_functions.c
@@ -1263,7 +1263,9 @@ int bind_text_null(sqlite3_stmt *res, int position, const char *text, bool can_b
 int sql_metadata_cache_stats(int op)
 {
     int count, dummy;
+    netdata_thread_disable_cancelability();
     sqlite3_db_status(db_meta, op, &count, &dummy, 0);
+    netdata_thread_enable_cancelability();
     return count;
 }