rangefeed: Pushtxn in rangefeed returned abort, but txn may have been committed

[pand5a]

I don't know if it's appropriate to send it here, but I'm really troubled.

The RangeFeed relies on the return result of PushTxns (task. go: pushOldTxns) ABORTED to remove the tracked txn in UnresolvedIntentQueue, which depends on the correctness of PushTxn.

cockroach/pkg/kv/kvserver/rangefeed/task.go

Lines 324 to 336 in 9c2f650

	case roachpb.ABORTED:
	// The transaction is aborted, so it doesn't need to be tracked
	// anymore nor does it need to prevent the resolved timestamp from
	// advancing. Inform the Processor that it can remove the txn from
	// its unresolvedIntentQueue.
	//
	// NOTE: the unresolvedIntentQueue will ignore MVCCAbortTxn operations
	// before it has been initialized. This is not a concern here though
	// because we never launch txnPushAttempt tasks before the queue has
	// been initialized.
	ops[i].SetValue(&enginepb.MVCCAbortTxnOp{
	TxnID: txn.ID,
	})

But in cmd_push_txn.go The “case txnID” case of [PushTxn ->SynthesizeTxnFromMeta ->CanCreateTxnRecord(replica_tscache.go)] may return a transaction that has already been COMMITTED but has status=Abort, which may cause rts in the RangeFeed to advance incorrectly. Do I understand correctly?

cockroach/pkg/kv/kvserver/replica_tscache.go

Lines 529 to 543 in 9c2f650

	if txnMinTS.LessEq(tombstoneTimestamp) {
	switch tombstoneTxnID {
	case txnID:
	// If we find our own transaction ID then a transaction record has already
	// been written. We might be a replay (e.g. a DistSender retry), or we
	// raced with an asynchronous abort. Either way, return an error.
	//
	// TODO(andrei): We could keep a bit more info in the tscache to return a
	// different error for COMMITTED transactions. If the EndTxn(commit) was
	// the only request in the batch, this this would be sufficient for the
	// client to swallow the error and declare the transaction as committed.
	// If there were other requests in the EndTxn batch, then the client would
	// still have trouble reconstructing the result, but at least it could
	// provide a non-ambiguous error to the application.
	return false, kvpb.ABORT_REASON_RECORD_ALREADY_WRITTEN_POSSIBLE_REPLAY

Jira issue: CRDB-28452

Epic CRDB-27235

[blathers-crl]

Hello, I am Blathers. I am here to help you get the issue triaged.

It looks like you have not filled out the issue in the format of any of our templates. To best assist you, we advise you to use one of these templates.

I was unable to automatically find someone to ping.

If we have not gotten back to your issue within a few business days, you can try the following:

• Join our community slack channel and ask on #cockroachdb.
• Try find someone from here if you know they worked closely on the area and CC them.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[blathers-crl]

cc @cockroachdb/cdc

[blathers-crl]

cc @cockroachdb/replication

[aliher1911]

I'm looking into this. Seems like the question would be can we have pending intent in the past when we see a transaction tombstone for committed transaction that we interpreted as abort.

[pand5a]

Is it possible that this is the case:

It is possible that the rangefeed is built on a non majority replica, while the pushtxn in the rangefeed is sent at a fixed frequency, that is:

1. Leasholder and Majority commited it at ts1.

2. The rangefeed is built on a minority replica, and a pushtxn is issued on ts2 and a txnabort is returned. However, as the replica is asynchronous, so the commit is received on ts3 on this replica

[aliher1911]

It looks like we just need a range feed running on a follower and that follower should be lagging behind on raft application. In that case it could try to push already committed transactions because it tries to compare txn timestamps from the intents it observes with wall clock (now() - threshold). Then it will drop the transaction from the list and advance its resolved timestamp.

We have a protection in rangefeed

cockroach/pkg/kv/kvserver/rangefeed/resolved_timestamp.go

Lines 265 to 273 in 9c2f650

	// assertOpAboveTimestamp asserts that this operation is at a larger timestamp
	// than the current resolved timestamp. A violation of this assertion would
	// indicate a failure of the closed timestamp mechanism.
	func (rts *resolvedTimestamp) assertOpAboveRTS(op enginepb.MVCCLogicalOp, opTS hlc.Timestamp) {
	if opTS.LessEq(rts.resolvedTS) {
	panic(fmt.Sprintf("resolved timestamp %s equal to or above timestamp of operation %v",
	rts.resolvedTS, op))
	}
	}

which would cause node to panic if timestamp regression is detected to prevent any potential data inconsistency/loss on the client side.
And I can't find any evidence of panics generated by regressions like that which means it happens sufficiently infrequently.

To address it we may need to distinguish between abort types when resolving intents and in this particular case keep transaction and wait for intents to get resolved.

[pand5a]

"We have a protection in rangefeed"
-->

Yes, but due to MVCCAbortTxnOp, txn was removed from the list, causing rts to advance, and due to 'enginepb'. MVCCCommitteIntentOp "is not protected by assertOpAboveRTS, so this data has been discarded in the following code.

cockroach/pkg/ccl/changefeedccl/event_processing.go

Lines 401 to 411 in 9c2f650

	// Ensure that r updates are strictly newer than the least resolved timestamp
	// being tracked by the local span frontier. The poller should not be forwarding
	// r updates that have timestamps less than or equal to any resolved timestamp
	// it's forwarded before.
	// TODO(dan): This should be an assertion once we're confident this can never
	// happen under any circumstance.
	if schemaTS.LessEq(c.frontier.Frontier()) && !schemaTS.Equal(c.cursor) {
	log.Errorf(ctx, "cdc ux violation: detected timestamp %s that is less than "+
	"or equal to the local frontier %s.", schemaTS, c.frontier.Frontier())
	return nil
	}

"To address it we may need to distinguish between abort types when resolving intents and in this particular case keep transaction and wait for intents to get resolved."
-->
Yes. But can it be done in ts_cache adding the state of txn？Is this feasible?

[aliher1911]

I see what you mean. Didn't realize we skip intents when checking timestamp assertion. Even if we did, intents could arrive after checkpoint is out and even if we panic, client will not reread that data because of published checkpoint timestamp.

As for pts_cache, I don't think we need any extra info there. The problem seem to be that we know this special cause where transaction tombstone is present which should be a enough rare case, but we don't have a mechanism to surface this error to range feed processor easily without changing protocol. pts_cache can reside on any other node where transaction record was created.

Did you see this error actually happening?

[pand5a]

by code review, I want to use one slow node and two fast nodes, which may trigger this issue. Because from a code perspective, this issue should exist. I will try it out some time.

[blathers-crl]

cc @cockroachdb/replication