Skip to content

2.27.0.0-b438

@myang2021 myang2021 tagged this 12 Aug 18:24
Summary:
I found a bug in `IsRenewRequired` that makes the `renew soft` strategy not
working as expected.

```
  [[nodiscard]] bool IsRenewRequired(
      CoarseTimePoint creation_time, CoarseTimePoint now,
      const PgPerformOptionsPB::CachingInfoPB& cache_info, Counter** renew_metric) const {
    if (!cache_info.has_lifetime_threshold_ms()) {
      return false;
    }

    const auto threshold = cache_info.has_lifetime_threshold_ms();
    if (!threshold) {
      *renew_metric = renew_hard_.get();
      return true;
    }

    if (creation_time < now - std::chrono::milliseconds(threshold)) {
      *renew_metric = renew_soft_.get();
      return true;
    }

    return false;
  }

```
The buggy code is
```
    const auto threshold = cache_info.has_lifetime_threshold_ms();
```
The correct code should be:
```
    const auto threshold = cache_info.lifetime_threshold_ms().value();
```

Note that the meaning of threshold is defined by
```
[[nodiscard]] std::optional<uint32_t> GetCacheLifetimeThreshold(PrefetchingCacheMode mode) {
  switch(mode) {
    case PrefetchingCacheMode::TRUST_CACHE:
      return std::nullopt;
    case PrefetchingCacheMode::RENEW_CACHE_SOFT:
      return FLAGS_pg_cache_response_renew_soft_lifetime_limit_ms;
    case PrefetchingCacheMode::RENEW_CACHE_HARD:
      return 0;
  }
  FATAL_INVALID_ENUM_VALUE(PrefetchingCacheMode, mode);
}
```

and

```
  message CachingInfoPB {
    uint32 key_group = 1;
    bytes key_value = 2;
    OptionalUint32PB lifetime_threshold_ms = 3;
  }
```

Therefore if `has_lifetime_threshold_ms()` is true then

(1) lifetime_threshold_ms > 0, it means soft renew
(2) lifetime_threshold_ms is 0, it means hard renew

A new unit test is added that would fail before the fix.
Jira: DB-17888

Test Plan: ./yb_build.sh release --cxx-test pgwrapper_pg_catalog_perf-test

Reviewers: kfranz, sanketh, mihnea

Reviewed By: sanketh

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D45930
Assets 2
Loading