On Thu, Jun 5, 2025 at 11:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jun 4, 2025 at 11:36 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Sawada-san,
> >
> > > Alternative idea is to lower the constant value when using an
> > > assertion build. That way, we don't need to rely on injection points
> > > being enabled.
> >
> > Hmm, possible but I prefer current one. Two concerns:
> >
> > 1.
> > USE_ASSERT_CHECKING has not been used to change the value yet. The main usage is
> > to call debug functions in debug build.
>
> I think we have a similar precedent such as MT_NRELS_HASH to improve
> the test coverages.
>
> > 2.
> > If we add tests which is usable only for debug build, it must be run only when it
> > is enabled. IIUC such test does not exist yet.
>
> I think we need to test cases not to check if we reach a specific code
> point but to check if we can get the correct results even if we've
> executed various code paths. As for this bug, it is better to check
> that it works properly in a variety of cases. That way, we can check
> overflow cases and non-overflow cases also in test cases added in the
> future, improving the test coverage more.
>
This makes sense, but we should see whether some existing tests cover
this code path after lowering the limit in the overflow code path. One
minor point to consider is that at the time, the value MT_NRELS_HASH
was used to cover cases in a debug build, but we didn't have the
injection_point framework.
BTW, I noticed that you removed the following comments in your suggestions:
/*
* Stores cache invalidation messages distributed by other transactions.
- *
- * It is acceptable to skip invalidations received from concurrent
- * transactions during ReorderBufferForget and ReorderBufferInvalidate,
- * because the transaction being discarded wouldn't have loaded any shared
IIUC, you only mentioned having some comments like this for ease of
understanding, and now you are suggesting to remove those.
--
With Regards,
Amit Kapila.
