-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from 11 commits
de1689e
28be12a
acf0ee1
2471cd4
e9a0232
c0372f2
807a1db
2f04509
69a8b58
5e23800
a9cae42
214babe
548343f
7b5da43
28eee46
9d58b8c
23d1fd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ package queryissue | |
|
||
import ( | ||
"fmt" | ||
"sort" | ||
"strings" | ||
|
||
"github.com/yugabyte/yb-voyager/yb-voyager/src/constants" | ||
|
@@ -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, | ||
}, | ||
} | ||
|
||
|
@@ -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.", | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{}{}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertNoError, is enough, right? do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this function |
||
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() | ||
|
@@ -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) | ||
|
@@ -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) | ||
|
||
} |
There was a problem hiding this comment.
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?