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

Report unsupported database options under unsupported query constructs #2270

Merged
merged 17 commits into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/issue-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:
strategy:
fail-fast: false
matrix:
version: [2.23.1.0-b220, 2024.1.3.1-b8, 2024.2.0.0-b145, 2.20.8.0-b53, 2.18.9.0-b17, 2.25.0.0-b489]
version: [2.23.1.0-b220, 2024.1.3.1-b8, 2024.2.0.0-b145, 2.20.8.0-b53, 2.25.0.0-b489]
env:
YB_VERSION: ${{ matrix.version }}
YB_CONN_STR: "postgres://yugabyte:yugabyte@127.0.0.1:5433/yugabyte"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"VoyagerVersion": "IGNORED",
"TargetDBVersion": "IGNORED",
"MigrationComplexity": "MEDIUM",
"MigrationComplexityExplanation": "Found 0 Level 1 issue(s), 19 Level 2 issue(s) and 1 Level 3 issue(s), resulting in MEDIUM migration complexity",
"MigrationComplexityExplanation": "Found 0 Level 1 issue(s), 20 Level 2 issue(s) and 1 Level 3 issue(s), resulting in MEDIUM migration complexity",
"SchemaSummary": {
"Description": "Objects that will be created on the target YugabyteDB.",
"DbName": "pg_assessment_report_uqc",
Expand Down Expand Up @@ -321,6 +321,12 @@
"2.25": "2.25.0.0"
}
},
{
"ConstructTypeName": "Database options",
"Query": "CREATE DATABASE strategy_example\n WITH STRATEGY = 'wal_log'",
"DocsLink": "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#postgresql-12-and-later-features",
"MinimumVersionsFixedIn": null
},
{
"ConstructTypeName": "Jsonb Subscripting",
"Query": "SELECT ($1 :: jsonb)[$2][$3] as b",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,9 @@ WITH w AS (
SELECT * FROM w AS w1 JOIN w AS w2 ON w1.key = w2.ref
WHERE w2.key = 123;

LISTEN my_table_changes;
CREATE DATABASE strategy_example
WITH STRATEGY = 'wal_log';

DROP DATABASE strategy_example;

LISTEN my_table_changes;
2 changes: 2 additions & 0 deletions yb-voyager/src/query/queryissue/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ const (
LISTEN_NOTIFY_NAME = "Events Listen / Notify"
NON_DECIMAL_INTEGER_LITERAL = "NON_DECIMAL_INTEGER_LITERAL"
NON_DECIMAL_INTEGER_LITERAL_NAME = "Non-decimal integer literal"

DATABASE_OPTIONS = "DATABASE_OPTIONS"
)

const (
Expand Down
50 changes: 50 additions & 0 deletions yb-voyager/src/query/queryissue/detectors.go
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,56 @@ func (c *CommonTableExpressionDetector) GetIssues() []QueryIssue {
return issues
}

type DatabaseOptionsDetector struct {
query string
dbName string
pg15OptionsDetected mapset.Set[string]
pg17OptionsDetected mapset.Set[string]
}

func NewDatabaseOptionsDetector(query string) *DatabaseOptionsDetector {
return &DatabaseOptionsDetector{
query: query,
pg15OptionsDetected: mapset.NewThreadUnsafeSet[string](),
pg17OptionsDetected: mapset.NewThreadUnsafeSet[string](),
}
}

func (d *DatabaseOptionsDetector) Detect(msg protoreflect.Message) error {
if queryparser.GetMsgFullName(msg) != queryparser.PG_QUERY_CREATEDB_STMT_NODE {
return nil
}
/*
stmts:{stmt:{createdb_stmt:{dbname:"test" options:{def_elem:{defname:"oid" arg:{integer:{ival:121231}} defaction:DEFELEM_UNSPEC
location:22}}}} stmt_len:32} stmts:{stmt:{listen_stmt:{conditionname:"my_table_changes"}} stmt_location:33 stmt_len:25}
*/
d.dbName = queryparser.GetStringField(msg, "dbname")
defNames, err := queryparser.TraverseAndExtractDefNamesFromDefElem(msg)
if err != nil {
return err
}
for defName, _ := range defNames {
if unsupportedDatabaseOptionsFromPG15.ContainsOne(defName) {
d.pg15OptionsDetected.Add(defName)
}
if unsupportedDatabaseOptionsFromPG17.ContainsOne(defName) {
d.pg17OptionsDetected.Add(defName)
}
}
return nil
}

func (d *DatabaseOptionsDetector) GetIssues() []QueryIssue {
var issues []QueryIssue
if d.pg15OptionsDetected.Cardinality() > 0 {
issues = append(issues, NewDatabaseOptionsPG15Issue("DATABASE", d.dbName, d.query, d.pg15OptionsDetected.ToSlice()))
}
if d.pg17OptionsDetected.Cardinality() > 0 {
issues = append(issues, NewDatabaseOptionsPG17Issue("DATABASE", d.dbName, d.query, d.pg17OptionsDetected.ToSlice()))
}
return issues
}

type ListenNotifyIssueDetector struct {
query string
detected bool
Expand Down
8 changes: 8 additions & 0 deletions yb-voyager/src/query/queryissue/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,11 @@ var nonDecimalIntegerLiterals = []string{
//https://github.com/pganalyze/pg_query_go/blob/38c866daa3fdb0a7af78741476d6b89029c19afe/parser/src_backend_utils_adt_numutils.c#L59C30-L61C76
// the prefix "0x" could be "0X" as well so should check both
}

var unsupportedDatabaseOptionsFromPG15 = mapset.NewThreadUnsafeSet([]string{
"icu_locale", "locale_provider", "locale", "strategy", "collation_version", "oid",
}...)

var unsupportedDatabaseOptionsFromPG17 = mapset.NewThreadUnsafeSet([]string{
"builtin_locale", "icu_rules",
}...)
39 changes: 38 additions & 1 deletion yb-voyager/src/query/queryissue/issues_ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package queryissue

import (
"fmt"
"sort"
"strings"

"github.com/yugabyte/yb-voyager/yb-voyager/src/constants"
Expand Down Expand Up @@ -556,7 +557,7 @@ var deterministicOptionCollationIssue = issue.Issue{
GH: "https://github.com/yugabyte/yugabyte-db/issues/25575",
DocsLink: "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#postgresql-12-and-later-features",
MinimumVersionsFixedIn: map[string]*ybversion.YBVersion{
ybversion.SERIES_2_25: ybversion.V2_25_0_0,
ybversion.SERIES_2_25: ybversion.V2_25_0_0,
},
}

Expand Down Expand Up @@ -631,3 +632,39 @@ var uniqueNullsNotDistinctIssue = issue.Issue{
func NewUniqueNullsNotDistinctIssue(objectType string, objectName string, sqlStatement string) QueryIssue {
return newQueryIssue(uniqueNullsNotDistinctIssue, objectType, objectName, sqlStatement, map[string]interface{}{})
}

var databaseOptionsPG15Issue = issue.Issue{
Type: DATABASE_OPTIONS,
Name: "Database options",
Impact: constants.IMPACT_LEVEL_2,
Description: "Database options (%s) introduced in PostgreSQL 12 and later are not supported yet in YugabyteDB.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets define this along with other description constant vars?

Suggestion: "",
GH: "https://github.com/yugabyte/yugabyte-db/issues/25575",
DocsLink: "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#postgresql-12-and-later-features",
// MinimumVersionsFixedIn: map[string]*ybversion.YBVersion{
// ybversion.SERIES_2_25: ybversion.V2_25_0_0,
// },// Not marking it as supported for 2.25 as it seems these might be actually not supported at least the locale and collation ones https://github.com/yugabyte/yugabyte-db/issues/25541
}

func NewDatabaseOptionsPG15Issue(objectType string, objectName string, sqlStatement string, options []string) QueryIssue {
issue := databaseOptionsPG15Issue
issue.Description = fmt.Sprintf(issue.Description, strings.Join(options, ", "))
return newQueryIssue(issue, objectType, objectName, sqlStatement, map[string]interface{}{})
}

var databaseOptionsPG17Issue = issue.Issue{
Type: DATABASE_OPTIONS,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's have a different type for both these issues? Type should ideally be unique. Perhaps, name can be the same.

Name: "Database options",
Impact: constants.IMPACT_LEVEL_2,
Description: "Database options (%s) introduced in PostgreSQL 12 and later are not supported yet in YugabyteDB.",
Suggestion: "",
GH: "https://github.com/yugabyte/yugabyte-db/issues/25575",
DocsLink: "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#postgresql-12-and-later-features",
}

func NewDatabaseOptionsPG17Issue(objectType string, objectName string, sqlStatement string, options []string) QueryIssue {
sort.Strings(options)
issue := databaseOptionsPG17Issue
issue.Description = fmt.Sprintf(issue.Description, strings.Join(options, ", "))
return newQueryIssue(issue, objectType, objectName, sqlStatement, map[string]interface{}{})
}
66 changes: 54 additions & 12 deletions yb-voyager/src/query/queryissue/issues_ddl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,16 +71,6 @@ func assertErrorCorrectlyThrownForIssueForYBVersion(t *testing.T, execErr error,
}
}

func testXMLFunctionIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, "SELECT xmlconcat('<abc/>', '<bar>foo</bar>')")
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "unsupported XML feature", xmlFunctionsIssue)
}

func testStoredGeneratedFunctionsIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
Expand Down Expand Up @@ -393,6 +383,56 @@ EXECUTE FUNCTION public.check_sales_region();`)
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, `"sales_region" is a partitioned table`, beforeRowTriggerOnPartitionTableIssue)
}

func testDatabaseOptions(t *testing.T) {
sqlsforPG15 := []string{
` CREATE DATABASE locale_example
WITH LOCALE = 'en_US.UTF-8'
TEMPLATE = template0;`,
`CREATE DATABASE locale_provider_example
WITH ICU_LOCALE = 'en_US'
LOCALE_PROVIDER = 'icu'
TEMPLATE = template0;`,
`CREATE DATABASE oid_example
WITH OID = 123456;`,
`CREATE DATABASE collation_version_example
WITH COLLATION_VERSION = '153.128';`,
`CREATE DATABASE strategy_example
WITH STRATEGY = 'wal_log';`,
}
sqlsForPG17 := []string{
`CREATE DATABASE icu_rules_example
WITH ICU_RULES = '&a < b < c';`,
`CREATE DATABASE builtin_locale_example
WITH BUILTIN_LOCALE = 'C';`,
}

for _, sql := range sqlsforPG15 {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, sql)
switch {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why do we need this switch case? If it's 2.25, err will be empty, and assertErrorCorrectlyThrownForIssueForYBVersion will take care of it automatically, right?

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the switch case right, because firstly we are not marking this as fixed in 2.25 but the CREATE DATABASE works without error in the 2.25 so, we need to run this func assertErrorCorrectlyThrownForIssueForYBVersion with different expected and actual errors in different cases

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a comment here as well to indicate that it is not actually supported in 2.25, but does not throw any error as such

case testYbVersion.ReleaseType() == ybversion.V2_25_0_0.ReleaseType() && testYbVersion.GreaterThanOrEqual(ybversion.V2_25_0_0):
assert.NoError(t, err)
assertErrorCorrectlyThrownForIssueForYBVersion(t, fmt.Errorf(""), "", databaseOptionsPG15Issue)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNoError, is enough, right? do we need the assertErrorCorrectlyThrownForIssueForYBVersion as well?

Copy link
Contributor Author

@priyanshi-yb priyanshi-yb Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this function assertErrorCorrectlyThrownForIssueForYBVersion we also do the IsFixedIn check so I think it makes sense to run this as well?

default:
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, `not recognized`, databaseOptionsPG15Issue)
}
}
for _, sql := range sqlsForPG17 {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, sql)
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, `not recognized`, databaseOptionsPG17Issue)
}

}

func testNonDeterministicCollationIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
Expand Down Expand Up @@ -485,8 +525,6 @@ func TestDDLIssuesInYBVersion(t *testing.T) {

// run tests
var success bool
success = t.Run(fmt.Sprintf("%s-%s", "xml functions", ybVersion), testXMLFunctionIssue)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "stored generated functions", ybVersion), testStoredGeneratedFunctionsIssue)
assert.True(t, success)
Expand Down Expand Up @@ -535,4 +573,8 @@ func TestDDLIssuesInYBVersion(t *testing.T) {

success = t.Run(fmt.Sprintf("%s-%s", "before row triggers on partitioned table", ybVersion), testBeforeRowTriggerOnPartitionedTable)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "database options", ybVersion), testDatabaseOptions)
assert.True(t, success)

}
49 changes: 48 additions & 1 deletion yb-voyager/src/query/queryissue/issues_dml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,43 @@ import (
testutils "github.com/yugabyte/yb-voyager/yb-voyager/test/utils"
)

func testXMLFunctionIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, "SELECT xmlconcat('<abc/>', '<bar>foo</bar>')")
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "unsupported XML feature", xmlFunctionsIssue)
}

func testAdvisoryLocks(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, "SELECT pg_advisory_unlock_shared(100);")
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "advisory locks are not yet implemented", advisoryLocksIssue)
}

func testSystemColumns(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
assert.NoError(t, err)

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, `
CREATE TABLE system_col_test(id int, val text);
`)
assert.NoError(t, err)
for _, col := range unsupportedSysCols.ToSlice() {
query := fmt.Sprintf("select %s from system_col_test", col)
_, err = conn.Exec(ctx, query)
assertErrorCorrectlyThrownForIssueForYBVersion(t, err, fmt.Sprintf(`System column "%s" is not supported yet`, col), systemColumnsIssue)
}
}

func testLOFunctionsIssue(t *testing.T) {
ctx := context.Background()
conn, err := getConn()
Expand Down Expand Up @@ -440,7 +477,17 @@ func TestDMLIssuesInYBVersion(t *testing.T) {
}

// run tests
success := t.Run(fmt.Sprintf("%s-%s", "lo functions", ybVersion), testLOFunctionsIssue)

success := t.Run(fmt.Sprintf("%s-%s", "xml functions", ybVersion), testXMLFunctionIssue)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "advisory locks", ybVersion), testAdvisoryLocks)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "system columns", ybVersion), testSystemColumns)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "lo functions", ybVersion), testLOFunctionsIssue)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "regex functions", ybVersion), testRegexFunctionsIssue)
Expand Down
1 change: 1 addition & 0 deletions yb-voyager/src/query/queryissue/parser_issue_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ func (p *ParserIssueDetector) genericIssues(query string) ([]QueryIssue, error)
NewJsonPredicateExprDetector(query),
NewNonDecimalIntegerLiteralDetector(query),
NewCommonTableExpressionDetector(query),
NewDatabaseOptionsDetector(query),
NewListenNotifyIssueDetector(query),
}

Expand Down
Loading