Hi,
I think there's a bug in how get_transaction_apply_action() interacts
with handle_streamed_transaction() to decide whether the transaction is
streamed or not. Originally, the code was simply:
/* not in streaming mode */
if (!in_streamed_transaction)
return false;
But now this decision was moved to get_transaction_apply_action(), which
does this:
if (am_parallel_apply_worker())
{
return TRANS_PARALLEL_APPLY;
}
else if (in_remote_transaction)
{
return TRANS_LEADER_APPLY;
}
and handle_streamed_transaction() then uses the result like this:
/* not in streaming mode */
if (apply_action == TRANS_LEADER_APPLY)
return false;
Notice this is not equal to the original behavior, because the two flags
(in_remote_transaction and in_streamed_transaction) are not inverse.
That is,
in_remote_transaction=false
does not imply we're processing streamed transaction. It's allowed both
flags are false, i.e. a change may be "non-transactional" and not
streamed, though the only example of such thing in the protocol are
logical messages. Which are however ignored in the apply worker, so I'm
not surprised no existing test failed on this.
So I think get_transaction_apply_action() should do this:
if (am_parallel_apply_worker())
{
return TRANS_PARALLEL_APPLY;
}
else if (!in_streamed_transaction)
{
return TRANS_LEADER_APPLY;
}
FWIW I've noticed this after rebasing the sequence decoding patch, which
adds another type of protocol message with the transactional vs.
non-transactional behavior, similar to "logical messages" except that in
this case the worker does not ignore that.
Also, I think get_transaction_apply_action() would deserve better
comments explaining how/why it makes the decisions.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
