Support exists_subquery #53

thoshiai · 2023-02-09T01:49:49Z

Simple EXISTS clause is supported by this patch. A query is not supported when WHERE clause has OR-condition or JOIN-condition is not contained by targetlist.

yugo-n · 2023-03-20T08:51:43Z

createas.c

@@ -261,7 +263,7 @@ ExecCreateImmv(ParseState *pstate, CreateTableAsStmt *stmt,
 				CreateIndexOnIMMV(viewQuery, matviewRel, true);

 				/* Create triggers on incremental maintainable materialized view */
-				CreateIvmTriggersOnBaseTables(viewQuery, matviewOid, true);
+				CreateIvmTriggersOnBaseTables(query, matviewOid, true);


Could you please add a comment for explaining why we have to use query instad of viewQuery?

Sorry. Don't need that I fixed it. revert it.

I wonder your reverting code would be wrong. The first commit changed the line 67, but the line 264 was changed in the second commit.

Sorry, I made mistake.
This code is not reverted.
I will add a comment for explaining.

createas.c

yugo-n · 2023-03-20T09:06:26Z

createas.c

-						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						 errmsg("unsupported subquery on incrementally maintainable materialized view"),
-						 errhint("Only simple subquery in FROM clause is supported.")));
+				/* Now, EXISTS clause is supported only */


Maybe, "Currently, only EXISTS clause is allowed here" or something like this?

Thanks, that's a right.
I will add its comments.

I'm sorry I didn't make it clear enough, but I just suggested that "Now, EXISTS clause is ..." at line 1031 should be replaced with "Currently, EXISTS clause is ...", instead of adding the comment.

I understood it. i will fix it

matview.c

yugo-n · 2023-03-20T09:41:38Z

I made some review comments. Especially, I think it would be nice if we have more comments in the code because we do somewhat complicated things for handling EXITS.

Also, could you please update README?

And add some comments and documents.

thoshiai · 2023-06-24T20:41:11Z

Thank you for your review.
I fixed and add README messages

yugo-n · 2023-07-13T05:51:51Z

createas.c

@@ -261,7 +263,7 @@ ExecCreateImmv(ParseState *pstate, CreateTableAsStmt *stmt,
 				CreateIndexOnIMMV(viewQuery, matviewRel, true);

 				/* Create triggers on incremental maintainable materialized view */
-				CreateIvmTriggersOnBaseTables(viewQuery, matviewOid, true);
+				CreateIvmTriggersOnBaseTables(query, matviewOid, true);


I wonder your reverting code would be wrong. The first commit changed the line 67, but the line 264 was changed in the second commit.

yugo-n · 2023-07-13T05:57:36Z

createas.c

+				 * additional restriction checks for exists subquery
+				 *
+				 * When contain EXISTS clauses, and it has a column refernces
+				 * a table outside, its column must be included by target list.


Do you mean "If the query has any EXISTS clauses and any columns in them refer to columns in tables in the outer query, those columns must be included in the target list." ?

How about adding comments about "why such check is needed" ?

Thanks! I change a comment.

yugo-n · 2023-07-14T08:25:00Z

createas.c

-						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-						 errmsg("unsupported subquery on incrementally maintainable materialized view"),
-						 errhint("Only simple subquery in FROM clause is supported.")));
+				/* Now, EXISTS clause is supported only */


I'm sorry I didn't make it clear enough, but I just suggested that "Now, EXISTS clause is ..." at line 1031 should be replaced with "Currently, EXISTS clause is ...", instead of adding the comment.

yugo-n · 2023-07-14T08:34:14Z

createas.c

+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("this query is not allowed on incrementally maintainable materialized view"),
+							 errhint("subquery in WHERE clause only supports subquery with EXISTS clause")));


What do you think this error message?

yugo-n · 2023-07-14T08:37:54Z

createas.c

+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("nested subquery is not supported on incrementally maintainable materialized view")));
+


I mean some nested subqueries are actually supported as above, so the error message does not seem correct, so should improve the message. What do you think about it?

yugo-n · 2023-07-14T08:40:38Z

I found that the following view definition caused a segmentation fault.

select create_immv('xx2', 'select exists(select 1 from t) from t2 ');

Could you fix the query check to prevent this?

yugo-n · 2023-07-14T09:30:05Z

Thank you for adding the description that EXISTS subqueries are supported to README!

If I understand correctly, I wonder that it would better to add the following information:

EXISTS subquery is supported only in WHERE but not in the target list
(Maybe, it is mandatory because it already explains Subqueries in targetlist are not supported, but adding the information would be helpful for clarification.)
Restriction that if EXISTS contains columns that refer to columns in tables in the outer query, such columns must be included in the target list.

sublink in targetlist is not supported, so add this restrict and testcase.

thoshiai

Fix your review comments.

thoshiai · 2023-07-26T05:44:32Z

createas.c

+			
F438
		ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("this query is not allowed on incrementally maintainable materialized view"),
+							 errhint("subquery in WHERE clause only supports subquery with EXISTS clause")));


This error can also occur other than subquery, so error message will be changed.

thoshiai · 2023-07-26T05:53:50Z

createas.c

+					ereport(ERROR,
+							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+							 errmsg("nested subquery is not supported on incrementally maintainable materialized view")));
+


This error can also occur other than subquery. the nested sublink is actually correct. So error message will be changed.

thoshiai · 2023-07-26T05:55:38Z

matview.c

 			TupleDesc		tupdesc_old;
 			TupleDesc		tupdesc_new;
 			bool	use_count = false;
 			char   *count_colname = NULL;

-			count_colname = pstrdup("__ivm_count__");
+			/* check if the modified table is in EXISTS clause. */
+			for (i = 0; i< list_length(rte_path); i++)


Sorry, this is fixed

thoshiai · 2023-07-26T05:57:28Z

matview.c

+			for (i = 0; i< list_length(rte_path); i++)
+			{
+				int index =  lfirst_int(list_nth_cell(rte_path, i));
+				rte = (RangeTblEntry *)lfirst(list_nth_cell(querytree->rtable, index - 1));


Sorry, it was fixed

thoshiai · 2023-08-06T16:33:14Z

matview.c

+	/* Add count(*) used for EXISTS clause */
+	foreach(tbl_lc, query->rtable)
+	{
+		RangeTblEntry *rte = (RangeTblEntry *)lfirst(tbl_lc);


Sorry, it was fixed

thoshiai · 2023-08-06T16:35:04Z

matview.c

+ * rewrite_query_for_exists_subquery
+ *
+ * Rewrite EXISTS sublink in WHERE to LATERAL subquery
+ */


I add an example at rewrite_querry_for_exists_subquery().

thoshiai · 2023-08-06T16:39:31Z

matview.c

+
+				/* get subquery in WHERE clause */
+				fromexpr = (FromExpr *)query->jointree;
+				query = rewrite_exists_subquery_walker(query, fromexpr->quals, count);


Sorry, it was fixed.
And I think that same mistakes are everywhere. so I will make a other patch with fixing spaces.

add restriction of EXISTS sublink

thoshiai · 2023-08-08T09:25:14Z

fix segmentation fault and add README

Support exists_subquery

f29aa80

Simple EXISTS clause is supported by this patch. A query is not supported when WHERE clause has OR-condition or JOIN-condition is not contained by targetlist.

thoshiai requested a review from yugo-n February 9, 2023 01:50

yugo-n requested changes Mar 20, 2023

View reviewed changes

Fix and cleanup source code

c0f8a22

And add some comments and documents.

thoshiai requested a review from yugo-n June 24, 2023 20:40

yugo-n requested changes 8000 Jul 14, 2023

View reviewed changes

hoshiai and others added 3 commits July 26, 2023 06:57

Fix some comments and error messages

01bc011

Fix segmentation fault and add some spaces

60e6a35

Add restriction of SubLink in pg_ivm

d5bfa72

sublink in targetlist is not supported, so add this restrict and testcase.

thoshiai commented Aug 8, 2023

View reviewed changes

Add README.md

0ad579f

add restriction of EXISTS sublink

Summarize the process of sublink restriction

3a35f22

yugo-n merged commit c355f40 into sraoss:main Aug 31, 2023

Support exists_subquery #53

Support exists_subquery #53

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!