On Sat, 7 Jun 2025 at 12:47, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 6 Jun 2025 at 22:51, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Jun 5, 2025 at 8:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > 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.
> >
> > True.
> >
> > After thinking about it more, perhaps my proposal would not be a good
> > idea for this case. I think that the cases where we selectively
> > invalidate caches is more complex and error-prone than the cases where
> > we invalidate a complete cache. If we invalidated all caches after
> > decoding each transaction, we wouldn't have had the original data-loss
> > issue. Having a lower MAX_DISTR_INVAL_MSG_PER_TXN value when using an
> > assertio build means that we're going to test the cases using a
> > simpler invalidation mechanism while productions systems, which has a
> > higher MAX_DISTR_INVAL_MSG_PER_TXN value, would end up executing
> > complex cases, which is not great. What do you think?
> >
> > BTW, as for a new test case, it might be worth having a case I
> > mentioned before[1]:
> >
> > 1) S1: CREATE TABLE d (data text not null);
> > 2) S1: INSERT INTO d VALUES ('d1');
> > 3) S2: BEGIN; INSERT INTO d VALUES ('d2');
> > 4) S3: BEGIN; INSERT INTO d VALUES ('d3');
> > 5) S1: ALTER PUBLICATION pb ADD TABLE d;
> > 6) S2: INSERT INTO d VALUES ('d4');
> > 7) S2: COMMIT;
> > 8) S3: COMMIT;
> > 9) S2: INSERT INTO d VALUES('d5');
> > 10) S1: INSERT INTO d VALUES ('d6');
> >
> > With this case, we can test if we need to execute the distributed
> > invalidations as well in the non-error path in
> > ReorderBufferProcessTXN().
>
> The attached v13 version patch has the change to include this test case.
Attached are the patches, including those required for the back branches.
Regards,
Vignesh
