Throw error when index operator is used without index #124

dqii · 2023-09-08T00:50:59Z

This implementation uses the ExecutorStart_hook to detect that the usage of the operator <-> is only in the context of an index scan.

codecov · 2023-09-08T00:53:32Z

Codecov Report

Merging #124 (e3e3e10) into main (4646afc) will decrease coverage by 0.71%.
Report is 10 commits behind head on main.
The diff coverage is 82.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
- Coverage   84.28%   83.57%   -0.71%     
==========================================
  Files          13       16       +3     
  Lines        1069     1163      +94     
  Branches      238      258      +20     
==========================================
+ Hits          901      972      +71     
- Misses         74       81       +7     
- Partials       94      110      +16

Files Changed	Coverage Δ
src/hnsw/options.c	`86.66% <66.66%> (+1.81%)`	⬆️
src/hooks/plan_tree_walker.c	`68.75% <68.75%> (ø)`
src/hooks/executor_start.c	`88.88% <88.88%> (ø)`
src/hooks/post_parse.c	`95.31% <95.31%> (ø)`
src/hooks/utils.c	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

dqii · 2023-09-09T00:28:46Z

I considered passing data from the post_parse_analyze_hook to the ExecutorStart_hook indicating that the operator is used, so that we didn't do this check in ExecutorStart_hook unnecessarily. The approaches didn't seem clean. I didn't want global variables. I didn't want a hash table. There's no built in way. I think it's better to just accept the duplication.

I considered removing the parse analyzer. This doesn't work because the planner is missing some information that the parser has.

I considered reducing the number of tree iterations by the post-parse hook by merging the operator check and the sort function. I didn't think this was worth it given that most queries probably won't use the index.

Lastly, more could be done to flesh out the plan tree walker. I opted against it for this diff to keep it simpler and because the other cases were lower priority.

Ngalstyan4

Looks good!

src/hooks/executor_start.c

Ngalstyan4 · 2023-09-09T23:07:17Z

src/hooks/executor_start.c

+{
+    if(node == NULL) return false;
+    if(IsA(node, IndexScan)) {
+        context->isIndexScan = true;


Can you explain what's happening here?
why do we set 'isIndexScan' to true, and then back to false on line 24

It's traversing a tree. When it hits an IndexScan node, it's going to "go inside it". I'm noting that we're inside an index scan now. If we hit the Lantern operator inside an index scan, that's an acceptable way to use the operator. We return false. If we hit the Lantern operator and we are not inside an index scan, that is not an acceptable way to use the operator. The if(list_member_oid(context->oidList, opExpr->opno) && !context->isIndexScan) { will catch this, and return true.

Once we have finished traversing an IndexScan, we note that we are no longer inside an IndexScan.

src/hooks/executor_start.c

src/hooks/utils.c

Ngalstyan4 · 2023-09-09T23:14:55Z

src/hooks/post_parse.c

+#endif
+    }
+
+    List *oidList = get_operator_oids();


oidList can be NULL here right?
If we account for that in function calls below, maybe leave a comment here to make it clear we did not forget it

Discussed offline. Will add ldb assert after that diff is pushed.

Ngalstyan4 · 2023-09-09T23:24:02Z

test/sql/utils/common.sql

+    plan_row RECORD;
+    found boolean := false;
+BEGIN
+    FOR plan_row IN EXECUTE 'EXPLAIN ' || query_text LOOP


I think the function would be clearer if EXPLAIN was passed with the query at callsite.
Then, it would be clear this function just inspects the output of the EXPLAIN query.

Ngalstyan4 · 2023-09-09T23:25:26Z

test/sql/hnsw_select.sql

@@ -36,11 +46,28 @@ WITH neighbors AS (
 RESET client_min_messages;

 -- Verify where condition works properly and still uses index
-SELECT * FROM small_world WHERE b IS TRUE ORDER BY v <-> '{0,0,0}';
-EXPLAIN (COSTS FALSE) SELECT * FROM small_world WHERE b IS TRUE ORDER BY v <-> '{0,0,0}';
+SELECT has_index_scan('SELECT * FROM small_world WHERE b IS TRUE ORDER BY v <-> ''{0,0,0}''');


Are multiple single quotes in ''{0,0,0}'' intentional?

Yes. Otherwise, it would match with the single quote that starts the string.

src/hooks/utils.c

Ngalstyan4 · 2023-09-09T23:31:06Z

src/hooks/plan_tree_walker.c

+
+bool plan_tree_walker(Plan *plan, bool (*walker_func)(Plan *plan, void *context), void *context)
+{
+    check_stack_depth();


what does this do?

This is in Postgres's expression_tree_walker function, so I copied it. It checks that we aren't going into too deep a recursion. So would be triggered for an unreasonable complex query, or if there's somehow an infinite loop.

src/hooks/plan_tree_walker.c

github-actions · 2023-09-10T15:44:25Z

Benchmarks

metric	old	new	pct change
recall (after create)	-	0.740	-
recall (after insert)	-	0.784	-
select tps	-	3292.904	-
select latency (ms)	-	1.666	-
create latency (ms)	-	1731.984	-
insert latency (ms)	-	2.014	-
insert tps	-	496.256	-
disk usage (bytes)	-	6348800.000	-

dqii added 11 commits September 5, 2023 18:45

Handle when there is a complex opExpr

86c02cc

Rename functions

019dac7

Remove unused functions in header file

ad3dafb

Use expression_tree_walker to simplify parser ops, add is_operator_used

5255260

Add list_free

7d1197e

Create hooks folder

d39a553

WIP

e775865

Revert

bebc9f0

Placeholder

071b0d9

Progress

6e81f2c

Fix and add tests

524bcbb

dqii added 12 commits September 8, 2023 01:24

Remove unnecessary intermediate function

2a8f962

Handle postgres 14

521a472

Fix pgvector installation (conflicting hnsw access method definition)

abcfadd

Add tests

89e36f5

Remove header file

c5bdf40

Tests

c0f7085

Merge branch 'main' into @di/ast-2

34bac4c

Fix missing import

3c822cd

Verify group by works

a570509

Verify distinct works

9dcab0b

Add join lateral

c41e0f7

Handle plan difference for pg11 vs. pg12-15

3f35203

dqii force-pushed the @di/ast-2 branch from 897fcdb to 3f35203 Compare September 8, 2023 17:37

dqii added 5 commits September 8, 2023 17:52

Add and use has_index_scan util

00b40c7

No need for else if due exhaustive conditions

2f45dd6

Null case handled in executor start

39b3127

remove unnecessary check

554de7c

Remove unused code

3ca1512

dqii added 2 commits September 8, 2023 18:44

Remove nil check

cc3f2e9

Add union test

58ba8c3

dqii force-pushed the @di/ast-2 branch from de7589a to 58ba8c3 Compare September 8, 2023 18:46

dqii added 3 commits September 8, 2023 18:57

Remove unnecessary tests, add new test

131e2e6

Add agg

1950846

Add tests

8a3702d

dqii marked this pull request as ready for review September 9, 2023 00:25

Ngalstyan4 reviewed Sep 9, 2023

View reviewed changes

dqii added 3 commits September 10, 2023 15:41

Rename get_operator_oids to clarify not native function

22c98da

Move global variable definitions

6d2a0a2

Remove unnecessary variable

ee25e22

dqii added 5 commits September 10, 2023 16:21

Rename functions for understandability

22ba311

Name function for clarity

090b356

Remove install warnings

f6bb59b

Improve comment

ea116fe

Change has_index_scan definition

e3e3e10

dqii merged commit fe37327 into main Sep 10, 2023

dqii deleted the @di/ast-2 branch September 10, 2023 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw error when index operator is used without index #124

Throw error when index operator is used without index #124

dqii commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

dqii commented Sep 9, 2023

Ngalstyan4 left a comment

Ngalstyan4 Sep 9, 2023

dqii Sep 10, 2023 •

edited

Loading

Ngalstyan4 Sep 9, 2023

dqii Sep 10, 2023

Ngalstyan4 Sep 9, 2023

Ngalstyan4 Sep 9, 2023

dqii Sep 10, 2023

Ngalstyan4 Sep 9, 2023

dqii Sep 10, 2023 •

edited

Loading

github-actions bot commented Sep 10, 2023 •

edited

Loading

Throw error when index operator is used without index #124

Throw error when index operator is used without index #124

Conversation

dqii commented Sep 8, 2023 • edited Loading

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

dqii commented Sep 9, 2023

Ngalstyan4 left a comment

Choose a reason for hiding this comment

Ngalstyan4 Sep 9, 2023

Choose a reason for hiding this comment

dqii Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Ngalstyan4 Sep 9, 2023

Choose a reason for hiding this comment

dqii Sep 10, 2023

Choose a reason for hiding this comment

Ngalstyan4 Sep 9, 2023

Choose a reason for hiding this comment

Ngalstyan4 Sep 9, 2023

Choose a reason for hiding this comment

dqii Sep 10, 2023

Choose a reason for hiding this comment

Ngalstyan4 Sep 9, 2023

Choose a reason for hiding this comment

dqii Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 10, 2023 • edited Loading

Benchmarks

dqii commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

dqii Sep 10, 2023 •

edited

Loading

dqii Sep 10, 2023 •

edited

Loading

github-actions bot commented Sep 10, 2023 •

edited

Loading