On Fri, Jun 6, 2025 at 10:51 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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?
>
Your reasoning makes sense to me. The other thing is that it would be
better if we don't add more cases to rely on debug build for testing.
Going forward, it can become difficult to decide which cases are good
to test only in debug mode.
> 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().
>
+1.
> >
> > 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.
>
> I forgot to mention the reason. I thought we need either a
> comprehensive comment in a place about in which case we need to
> execute both the current transaction's inval messages and the
> distributed inval messages and in which case we need to execute only
> inval messages in the current transaction or to put comments where we
> need. The v11 added the comprehensive comment to the declaration of
> ninvalidations_distributed and invalidations_distributed in
> ReoderBufferTXN, but I'm not sure that was the right place to have
> such a comment as it's beyond the description of these fields. So in
> my suggestion, I tried to clarify that we execute only the inval
> message in the current transaction in ReorderBufferForget() and
> ReorderBufferAbort() as they seems to already have a enough comment
> for the reason why we need to execute the inval message there.
>
Fair enough. I see one more case aka the call to
ReorderBufferExecuteInvalidations() in ReorderBufferFinishPrepared().
I think we should have executed the required invalidations at the end
of prepare only, so why do we need to execute in
ReorderBufferFinishPrepared? It might be kept as a general cleanup
call and if that is the case we might want to even adjust comments
there. What do you think?
--
With Regards,
Amit Kapila.
