Reporting CTE Materialized clause and Sql body in Function #2165

priyanshi-yb · 2025-01-08T19:20:44Z

Describe the changes in this pull request

Reports the CTE statements with Materialized clause as not supported. https://yugabyte.atlassian.net/browse/DB-14227
Reports the presence of SQL body in Functions - PG15 docs
https://yugabyte.atlassian.net/browse/DB-14234

Describe if there are any user-facing changes

Added new feature SQL body in Function to report functions and Modifying the Materialized option with CTE in assessment.

How was this pull request tested?

unit tests and end-to-end-tests

Does your PR have changes that can cause upgrade issues?

Component	Breaking changes?
MetaDB	No
Name registry json	No
Data File Descriptor Json	No
Export Snapshot Status Json	o
Import Data State	No
Export Status Json	No
Data .sql files of tables	No
Export and import data queue	No
Schema Dump	No
AssessmentDB	No
Sizing DB	No
Migration Assessment Report Json	No
Callhome Json	No
YugabyteD Tables	No
TargetDB Metadata Tables	No

sanyamsinghal · 2025-01-17T08:40:37Z

yb-voyager/src/query/queryissue/issues_ddl.go

@@ -566,6 +566,19 @@ func NewForeignKeyReferencesPartitionedTableIssue(objectType string, objectName
 	return newQueryIssue(foreignKeyReferencesPartitionedTableIssue, objectType, objectName, SqlStatement, details)
 }

+var sqlBodyInFunctionIssue = issue.Issue{


please define a Description for each issue moving forward.

sanyamsinghal · 2025-01-17T08:40:50Z

yb-voyager/src/query/queryissue/issues_dml.go

+	Type:        CTE_WITH_MATERIALIZED_CLAUSE,
+	Name:        CTE_WITH_MATERIALIZED_CLAUSE_NAME,
+	Impact:      constants.IMPACT_LEVEL_2,
+	Description: "",


similarly here.

makalaaneesh · 2025-01-20T04:36:37Z

yb-voyager/src/query/queryissue/detectors.go

+	if err != nil {
+		return err
+	}
+	if cteNode.Ctematerialized != queryparser.CTE_MATERIALIZED_DEFAULT {


Can you add a comment here explaining that both MATERIALIZED, NOT MATERIALIZED flags were not supported..

makalaaneesh · 2025-01-20T04:41:38Z

yb-voyager/src/query/queryissue/detectors_ddl.go

+	}
+	var issues []QueryIssue
+
+	if function.HasSqlBody {


Can you use just BEGIN to start a function block?

If so, are both BEGIN and BEGIN ATOMIC unsupported?

sql_body could also just be a "return expression" from docs. Would that also be unsupported? https://www.postgresql.org/docs/current/sql-createfunction.html#:~:text=a%20new%20session.-,sql_body,is%20more%20compatible%20with%20the%20SQL%20standard%20and%20other%20SQL%20implementations.,-Overloading I see in your test that that is also unsupported.

Can you use just BEGIN to start a function block?

yes BEGIN can also start a function block

If so, are both BEGIN and BEGIN ATOMIC unsupported?

No only BEGIN ATOMIC is unsupported as its PG 15 feature

Right, but will function.HasSqlBody be true even if the function has just BEGIN ? That would be a problem, right?

No, this HasSqlBody is true only if the parser has populated the sql_body field for function stmt, which is a new PG 15 feature for the BEGIN ATOMIC/RETURN expr type of function block. So, in normal BEGIN cases, this can't be true.

makalaaneesh · 2025-01-20T04:44:50Z

yb-voyager/src/query/queryissue/issues_dml_test.go

+	)
+	SELECT * FROM w AS w1 JOIN w AS w2 ON w1.key = w2.ref
+	WHERE w2.key = 123;`: `syntax error at or near "NOT"`,
+		`WITH moved_rows AS MATERIALIZED (


add a sql for NOT MATERIALIZED as well?

yes its there as first stmt

makalaaneesh · 2025-01-20T04:46:02Z

yb-voyager/src/query/queryparser/helpers_protomsg.go

@@ -420,28 +412,44 @@ func ProtoAsSelectStmt(msg protoreflect.Message) (*pg_query.SelectStmt, error) {
 	return selectStmtNode, nil
 }

+func ProtoAsCTENode(msg protoreflect.Message) (*pg_query.CommonTableExpr, error) {
+	protoMsg, ok := msg.Interface().(proto.Message)


I found out recently you don't need to do two steps.
You can convert it in one step directly.

msg.Interface().(*pg_query.xxx)

makalaaneesh · 2025-01-20T04:46:45Z

yb-voyager/cmd/assessMigrationCommand.go

@@ -1070,7 +1070,7 @@ func fetchUnsupportedPGFeaturesFromSchemaReport(schemaAnalysisReport utils.Schem
 	unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.JSONB_SUBSCRIPTING_NAME, "", queryissue.JSONB_SUBSCRIPTING, schemaAnalysisReport, false, ""))
 	unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE_NAME, "", queryissue.FOREIGN_KEY_REFERENCES_PARTITIONED_TABLE, schemaAnalysisReport, false, ""))
 	unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.JSON_TYPE_PREDICATE_NAME, "", queryissue.JSON_TYPE_PREDICATE, schemaAnalysisReport, false, ""))
-
+	unsupportedFeatures = append(unsupportedFeatures, getUnsupportedFeaturesFromSchemaAnalysisReport(queryissue.SQL_BODY_IN_FUNCTION_NAME, "", queryissue.SQL_BODY_IN_FUNCTION, schemaAnalysisReport, false, ""))


What about MATERIALIZED CTE issue? Can they not show up in DDLs?

yeah CTE can be in CREATE VIEW, adding it here as well

makalaaneesh · 2025-01-20T04:48:08Z

yb-voyager/cmd/analyzeSchema.go

@@ -918,6 +918,9 @@ sqlParsingLoop:
 			} else if matches := dollarQuoteRegex.FindStringSubmatch(currLine); matches != nil {
 				dollarQuoteFlag = 1 //denotes start of the code/body part
 				codeBlockDelimiter = matches[0]
+			} else if strings.Contains(currLine, "BEGIN ATOMIC") {


This is also fixed in https://github.com/yugabyte/yb-voyager/pull/2201/files ?
Is that a prerequisite for this PR?

yes otherwise analyze-schema/assessment won't be able to report the case as our parser will not work

sanyamsinghal · 2025-01-20T19:20:08Z

yb-voyager/src/query/queryparser/ddl_processor.go

@@ -1021,6 +1021,7 @@ func (mv *FunctionProcessor) Process(parseTree *pg_query.ParseResult) (DDLObject
 		SchemaName: funcSchemaName,
 		FuncName:   funcName,
 		ReturnType: GetReturnTypeOfFunc(parseTree),
+		HasSqlBody: funcNode.CreateFunctionStmt.GetSqlBody() != nil,


what if funcNode.CreateFunctionStmt is nil? Is that possible here?

No, that is not possible if its function node, create function stmt can't be nil.

sanyamsinghal · 2025-01-20T19:23:56Z

yb-voyager/src/query/queryparser/helpers_struct.go

+	node, isPlPgSQLObject := getCreateFuncStmtNode(parseTree)
+	return isPlPgSQLObject && (node.CreateFunctionStmt.SqlBody == nil) //TODO fix proper https://github.com/pganalyze/pg_query_go/issues/129


how/where are we handling the case when node.CreateFunctionStmt.SqlBody != nil in a plpgsql object.

There is no need to handle this case because sqlBody is only possible in language sql, so language plpgsql can't have sql_body in parser.
docs- https://www.postgresql.org/docs/15/sql-createfunction.html#:~:text=The%20body%20of%20a%20LANGUAGE%20SQL%20function.%20This%20can%20either%20be%20a%20single%20statement

makalaaneesh · 2025-01-21T07:59:19Z

yb-voyager/src/query/queryparser/helpers_struct.go

 func IsPLPGSQLObject(parseTree *pg_query.ParseResult) bool {
 	// CREATE FUNCTION is same parser NODE for FUNCTION/PROCEDURE
-	_, isPlPgSQLObject := getCreateFuncStmtNode(parseTree)
-	return isPlPgSQLObject
+	node, isPlPgSQLObject := getCreateFuncStmtNode(parseTree)


call it node, ok now because isPlPgSqlObject has logic depending on the node.

makalaaneesh

LGTM with a nit. Let's wait for Sanyam's approval as well before merging.

priyanshi-yb added 9 commits January 8, 2025 22:56

added CTE CASE and function sql body

15b4e98

reporting SQL body and added unit tests

a9ab725

commit a file

06108b6

fix

3c0fee2

format

c3f291d

merge fixes

3058bf7

go test failure

304b991

end-to-end-tests

e89e77a

regex parser logic for PLPGSQL block to handle SQL body as well

9a358df

priyanshi-yb mentioned this pull request Jan 16, 2025

Fix regex parser for parsing functions having SQL body with language sql (PG 15 feature) #2201

Open

priyanshi-yb added 3 commits January 16, 2025 18:49

fix name

012cdb9

fix test and name of cte issue

72b73d8

use constan

fc1b227

priyanshi-yb force-pushed the priyanshi/cte-sql-body-fn branch from 047fb53 to fc1b227 Compare January 16, 2025 13:30

priyanshi-yb requested review from makalaaneesh and sanyamsinghal January 16, 2025 14:15

priyanshi-yb marked this pull request as ready for review January 16, 2025 14:15

sanyamsinghal reviewed Jan 17, 2025

View reviewed changes

makalaaneesh reviewed Jan 20, 2025

View reviewed changes

review comments

1eb486e

priyanshi-yb requested review from makalaaneesh and sanyamsinghal January 20, 2025 11:06

sanyamsinghal reviewed Jan 20, 2025

View reviewed changes

makalaaneesh reviewed Jan 21, 2025

View reviewed changes

makalaaneesh approved these changes Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reporting CTE Materialized clause and Sql body in Function #2165

Reporting CTE Materialized clause and Sql body in Function #2165

priyanshi-yb commented Jan 8, 2025 •

edited

Loading

sanyamsinghal Jan 17, 2025

sanyamsinghal Jan 17, 2025

makalaaneesh Jan 20, 2025

makalaaneesh Jan 20, 2025

priyanshi-yb Jan 20, 2025

makalaaneesh Jan 20, 2025

priyanshi-yb Jan 20, 2025

makalaaneesh Jan 20, 2025

priyanshi-yb Jan 20, 2025

makalaaneesh Jan 20, 2025

makalaaneesh Jan 20, 2025

priyanshi-yb Jan 20, 2025

makalaaneesh Jan 20, 2025

priyanshi-yb Jan 20, 2025 •

edited

Loading

sanyamsinghal Jan 20, 2025

priyanshi-yb Jan 21, 2025

sanyamsinghal Jan 20, 2025

priyanshi-yb Jan 21, 2025

makalaaneesh Jan 21, 2025

makalaaneesh left a comment

		node, isPlPgSQLObject := getCreateFuncStmtNode(parseTree)
		return isPlPgSQLObject && (node.CreateFunctionStmt.SqlBody == nil) //TODO fix proper https://github.com/pganalyze/pg_query_go/issues/129

Reporting CTE Materialized clause and Sql body in Function #2165

Are you sure you want to change the base?

Reporting CTE Materialized clause and Sql body in Function #2165

Conversation

priyanshi-yb commented Jan 8, 2025 • edited Loading

Describe the changes in this pull request

Describe if there are any user-facing changes

How was this pull request tested?

Does your PR have changes that can cause upgrade issues?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyanshi-yb Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makalaaneesh left a comment

Choose a reason for hiding this comment

priyanshi-yb commented Jan 8, 2025 •

edited

Loading

priyanshi-yb Jan 20, 2025 •

edited

Loading