Log empty or nil select calls #277

oleg-jukovec · 2022-04-29T10:51:22Z

Empty or nil select calls on user spaces tend to be dangerous.

A critical log entry containing the current stack traceback is created
upon empty or nil select calls — a user can explicitly request a full
scan though by passing fullscan = true to select's options table
argument in which a case a log entry will not be created.

Tarantool has a similar implementation[1].

Log empty or nil select calls tarantool#7064

Closes #276

crud/ratelimit.lua

crud/select/compat/common.lua

crud/select/compat/select.lua

test/unit/select_nil_test.lua

README.md

oleg-jukovec · 2022-05-27T05:57:05Z

I will delete a doc entry:

**Note:**

Pagination with parameter ``after`` leads to a full scan even with the condition '=' or '==' on the index fields.
...

from doc/select.md after #295 (or I'll do it there, it depends on the merging order).

DifferentialOrange

I think we should merge #295 first and then deal with this PR.

Are there any plans to add more critical log conditions or we decided to stop for now?

oleg-jukovec · 2022-05-30T10:45:36Z

Are there any plans to add more critical log conditions or we decided to stop for now?

Thank you for the review! I think that we decided to stop with current conditions.

doc/select.md

test/helper.lua

Before the patch we copy an original EQ/REQ condition to post-filter and change the original to GE/LE. The problem is that EQ/REQ can't be a stop condition. As a result EQ/REQ + after unexpectedly became a full scan. The patch removes this behavior, the query becomes more expected and in base executes as regular EQ/REQ. We had an idea to replace EQ with GE+LE. This would help to skip less tuples in the executor inside scroll_to_after_tuple. But unfortunately, due to a bug[1], this will break the current behavior. So we need to fix the bug first. In additional, the 'potentially long select'[2] warning is confused by the GE/LE in the query plan. This patch only changes the internal behavior and does not affect a client code. 1. #301 2. #277

oleg-jukovec · 2022-06-14T06:05:33Z

A note after leads to a full scan has been removed from doc/select.md. The pull request should be merged after #295 .

Before the patch we copy an original EQ/REQ condition to post-filter and change the original to GE/LE. The problem is that EQ/REQ can't be a stop condition. As a result EQ/REQ + after unexpectedly became a full scan. The patch removes this behavior, the query becomes more expected and in base executes as regular EQ/REQ. We had an idea to replace EQ with GE+LE. This would help to skip less tuples in the executor inside scroll_to_after_tuple. But unfortunately, due to a bug[1], this will break the current behavior. So we need to fix the bug first. In additional, the 'potentially long select'[2] warning is confused by the GE/LE in the query plan. This patch only changes the internal behavior and does not affect a client code. 1. #301 2. #277

Potentially long select calls on user spaces tend to be dangerous. A critical log entry containing the current stack traceback is created upon potentially long select calls — a user can explicitly request a full scan though by passing fullscan = true to select's options table argument in which a case a log entry will not be created. Tarantool has a similar implementation[1][2]. 1. tarantool/tarantool#7064 2. tarantool/tarantool#7131 Part of #276

Potentially long count calls on user spaces tend to be dangerous. A critical log entry containing the current stack traceback is created upon potentially long count calls — a user can explicitly request a full scan though by passing fullscan = true to count's options table argument in which a case a log entry will not be created. Closes #276

oleg-jukovec force-pushed the oleg-jukovec/gh-276-log-select-nil branch 5 times, most recently from a70c0c4 to 7445256 Compare April 29, 2022 13:38

oleg-jukovec marked this pull request as ready for review April 29, 2022 14:02

oleg-jukovec requested review from DifferentialOrange, AnaNek and Totktonada April 29, 2022 14:02