Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification about READONLY errors inside a transaction #7014

Closed
luin opened this issue Mar 21, 2020 · 4 comments
Closed

Clarification about READONLY errors inside a transaction #7014

luin opened this issue Mar 21, 2020 · 4 comments

Comments

@luin
Copy link

luin commented Mar 21, 2020

Hi, I'm collaborating on a pull request in ioredis repo for handling READONLY errors inside a transaction. I think the behavior is inconsistent with the documentation so I'd just want to make sure here if it's a bug or something intentional.

Current behavior

Given two Redis servers (v5.0.6) A & B, A is the master and B is a read only replica (set up with replicaof A.host A.port), run the following commands in redis-cli against B:

B> multi
OK
B> set foo bar
(error) READONLY You can't write against a read only replica.
B> echo hi
QUEUED
B> exec
1) "hi"

EXEC command gives a partial result for the two command, which isn't expected.

Expected behavior

I think the whole transaction should be discarded.

In our documentation about transactions:

Before Redis 2.6.5 the behavior was to execute the transaction with just the subset of commands queued successfully in case the client called EXEC regardless of previous errors. The new behavior makes it much more simple to mix transactions with pipelining, so that the whole transaction can be sent at once, reading all the replies later at once.

It seems current behavior (that executing the transaction with the subset of commands queued successfully) is only normal for Redis < 2.6.5.

I checked the code and it seems READONLY is the only case that would fail without flagging the transaction. Is there any reasons behind this? If that's the case I think we should update the documentation accordingly.

https://github.com/antirez/redis/blob/1e16b9384d6aa3680ec5629fc0842ab53bf1655e/src/server.c#L3502-L3510

@WiFeng
Copy link

WiFeng commented Mar 22, 2020

Before Redis 2.6.5 the behavior was to execute the transaction with just the subset of commands queued successfully in case the client called EXEC regardless of previous errors. The new behavior makes it much more simple to mix transactions with pipelining, so that the whole transaction can be sent at once, reading all the replies later at once.

I think that It is important to mix transaction with pipelining. It is a server side capability, so you also should use pipelining to send your all commands once.

The redis-cli is a simple redis client that does not use pipelining.

@alavers
Copy link

alavers commented Mar 22, 2020

Hello @WiFeng! The same behavior is present with or without pipelining. I think what @luin is getting at is: why isn't flagTransaction being called in the linked code block.

@itamarhaber
Copy link
Member

Hi @luin - this looks like an inconsistency indeed, @antirez @yossigo @soloestoy?

@antirez
Copy link
Contributor

antirez commented Mar 22, 2020

Hi, that's a bug. Fixing.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants