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

sql: the rows affected count is wrong for UPSERT #24928

Closed
knz opened this issue Apr 19, 2018 · 0 comments
Closed

sql: the rows affected count is wrong for UPSERT #24928

knz opened this issue Apr 19, 2018 · 0 comments
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@knz
Copy link
Contributor

knz commented Apr 19, 2018

Found while working on a test for the rows affected value on statements that have no result rows.

Hinted by a chat with @mberhault.

The expected behavior of UPSERT (and possibly other mutation statements) is that its rows affected count should reflect the number of rows actually modified. However, CockroachDB gets this wrong:

INSERT INTO kv VALUES(1,2);
INSERT INTO kv VALUES (1,2), (3,4) ON CONFLICT(k) DO NOTHING; -- returns 2 rows affected, even though just 1 row was upserted

This in turn means that the GRANT statement will bump the role membership table version on every invocation, even if nothing is changed (= excessive chattiness).

(So this is not a correctness bug)

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 19, 2018
@knz knz self-assigned this Apr 19, 2018
@knz knz added this to Backlog in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Apr 19, 2018
@knz knz moved this from Backlog to In progress in (DEPRECATED) SQL Front-end, Lang & Semantics Apr 19, 2018
craig bot pushed a commit that referenced this issue Apr 21, 2018
23373: sql: fix the batching behavior of mutation statements, especially with RETURNING r=knz a=knz

Fixes #22304.
Fixes #23156.
Fixes #24928.

To review: 
- @RaduBerinde for the physical property propagation. Check the order_by test, confirm that this is enables optimizations of DELETE in a WITH clause or [ ... ]
- @jordanlewis for overall design of the new `batchedPlanNode` interface.
- @andreimatei @jordanlewis  for correctness of the batching behavior (like we discussed - I think this does everything you want)


Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig craig bot closed this as completed in #23373 Apr 21, 2018
(DEPRECATED) SQL Front-end, Lang & Semantics automation moved this from In progress to Archived Apr 21, 2018
@knz knz moved this from Finished (some previous milestone) to Finished (milestone 0423) in (DEPRECATED) SQL Front-end, Lang & Semantics May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

No branches or pull requests

1 participant