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

Test and Update MInVersionsFixedIn for issue of ALTER PARTITIONED TABLE ADD PRIMARY KEY #2047

Merged
merged 11 commits into from
Dec 11, 2024
11 changes: 0 additions & 11 deletions migtests/tests/analyze-schema/expected_issues.json
Original file line number Diff line number Diff line change
Expand Up @@ -1319,17 +1319,6 @@
"GH": "https://github.com/yugabyte/yugabyte-db/issues/3944",
"MinimumVersionsFixedIn": null
},
{
"IssueType": "migration_caveats",
"ObjectType": "TABLE",
"ObjectName": "public.range_columns_partition_test",
"Reason": "Adding primary key to a partitioned table is not supported yet.",
"SqlStatement": "ALTER TABLE ONLY public.range_columns_partition_test\n ADD CONSTRAINT range_columns_partition_test_pkey PRIMARY KEY (a, b);",
"DocsLink": "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#adding-primary-key-to-a-partitioned-table-results-in-an-error",
"Suggestion": "",
"GH": "https://github.com/yugabyte/yugabyte-db/issues/10074",
"MinimumVersionsFixedIn": null
},
{
"IssueType": "unsupported_features",
"ObjectType": "TABLE",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1960,17 +1960,7 @@
"MigrationCaveats": [
{
"FeatureName": "Alter partitioned tables to add Primary Key",
"Objects": [
{
"ObjectName": "public.sales_region",
"SqlStatement": "ALTER TABLE ONLY public.sales_region\n ADD CONSTRAINT sales_region_pkey PRIMARY KEY (id, region);"
},
{
"ObjectName": "schema2.sales_region",
"SqlStatement": "ALTER TABLE ONLY schema2.sales_region\n ADD CONSTRAINT sales_region_pkey PRIMARY KEY (id, region);"
}
],
"DocsLink": "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#adding-primary-key-to-a-partitioned-table-results-in-an-error",
"Objects": [],
"FeatureDescription": "After export schema, the ALTER table should be merged with CREATE table for partitioned tables as alter of partitioned tables to add primary key is not supported.",
"MinimumVersionsFixedIn": null
},
Expand Down
6 changes: 6 additions & 0 deletions yb-voyager/src/query/queryissue/issues_ddl.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"strings"

"github.com/yugabyte/yb-voyager/yb-voyager/src/issue"
"github.com/yugabyte/yb-voyager/yb-voyager/src/ybversion"
)

var generatedColumnsIssue = issue.Issue{
Expand Down Expand Up @@ -241,6 +242,11 @@ var alterTableAddPKOnPartitionIssue = issue.Issue{
TypeName: "Adding primary key to a partitioned table is not supported yet.",
DocsLink: "https://docs.yugabyte.com/preview/yugabyte-voyager/known-issues/postgresql/#adding-primary-key-to-a-partitioned-table-results-in-an-error",
GH: "https://github.com/yugabyte/yugabyte-db/issues/10074",
MinimumVersionsFixedIn: map[string]*ybversion.YBVersion{
ybversion.SERIES_2024_1: ybversion.V2024_1_0_0,
ybversion.SERIES_2024_2: ybversion.V2024_2_0_0,
ybversion.SERIES_2_23: ybversion.V2_23_0_0,
},
}

func NewAlterTableAddPKOnPartiionIssue(objectType string, objectName string, SqlStatement string) QueryIssue {
Expand Down
70 changes: 57 additions & 13 deletions yb-voyager/src/query/queryissue/issues_ddl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,31 @@ import (
"context"
"fmt"
"os"
"strings"
"testing"

"github.com/jackc/pgx/v5"
"github.com/jackc/pgx/v5/pgconn"
"github.com/stretchr/testify/assert"
"github.com/testcontainers/testcontainers-go/modules/yugabytedb"
"github.com/yugabyte/yb-voyager/yb-voyager/src/issue"
"github.com/yugabyte/yb-voyager/yb-voyager/src/ybversion"
)

var (
yugabytedbContainer *yugabytedb.Container
yugabytedbConnStr string
versions = []string{}
testYugabytedbContainer *yugabytedb.Container
testYugabytedbConnStr string
testYbVersion *ybversion.YBVersion
)

func getConn() (*pgx.Conn, error) {
ctx := context.Background()
var connStr string
var err error
if yugabytedbConnStr != "" {
connStr = yugabytedbConnStr
if testYugabytedbConnStr != "" {
connStr = testYugabytedbConnStr
} else {
connStr, err = yugabytedbContainer.YSQLConnectionString(ctx, "sslmode=disable")
connStr, err = testYugabytedbContainer.YSQLConnectionString(ctx, "sslmode=disable")
if err != nil {
return nil, err
}
Expand All @@ -53,14 +56,31 @@ func getConn() (*pgx.Conn, error) {
return conn, nil
}

func fatalIfError(t *testing.T, err error) {
if err != nil {
t.Fatalf("error: %v", err)
}
}

func assertErrorCorrectlyThrownForIssueForYBVersion(t *testing.T, execErr error, expectedError string, issue issue.Issue) {
isFixed, err := issue.IsFixedIn(testYbVersion)
fatalIfError(t, err)

if isFixed {
assert.NoError(t, execErr)
} else {
assert.ErrorContains(t, execErr, expectedError)
}
}

func getConnWithNoticeHandler(noticeHandler func(*pgconn.PgConn, *pgconn.Notice)) (*pgx.Conn, error) {
ctx := context.Background()
var connStr string
var err error
if yugabytedbConnStr != "" {
connStr = yugabytedbConnStr
if testYugabytedbConnStr != "" {
connStr = testYugabytedbConnStr
} else {
connStr, err = yugabytedbContainer.YSQLConnectionString(ctx, "sslmode=disable")
connStr, err = testYugabytedbContainer.YSQLConnectionString(ctx, "sslmode=disable")
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -128,23 +148,44 @@ func testUnloggedTableIssue(t *testing.T) {

}

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

defer conn.Close(context.Background())
_, err = conn.Exec(ctx, `
CREATE TABLE orders2 (
order_id bigint NOT NULL,
order_date timestamp
) PARTITION BY RANGE (order_date);
ALTER TABLE orders2 ADD PRIMARY KEY (order_id,order_date)`)

assertErrorCorrectlyThrownForIssueForYBVersion(t, err, "changing primary key of a partitioned table is not yet implemented", alterTableAddPKOnPartitionIssue)
}

func TestDDLIssuesInYBVersion(t *testing.T) {
var err error
ybVersion := os.Getenv("YB_VERSION")
if ybVersion == "" {
panic("YB_VERSION env variable is not set. Set YB_VERSIONS=2024.1.3.0-b105 for example")
}

yugabytedbConnStr = os.Getenv("YB_CONN_STR")
if yugabytedbConnStr == "" {
ybVersionWithoutBuild := strings.Split(ybVersion, "-")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: so our --target-db-version can only accept the version without build number only?
Just asking this if we are mentioning this in flag's help msg ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, right now it does accept build number as well, but the problem is that 2024.2.0.0-b145 is considered to be > 2024.2.0.0 (which is defined in the MinVersionsFixedIn).

I'm planning to raise another PR preventing users from passing the build number in the input to prevent these kinds of issues.

testYbVersion, err = ybversion.NewYBVersion(ybVersionWithoutBuild)
fatalIfError(t, err)

testYugabytedbConnStr = os.Getenv("YB_CONN_STR")
if testYugabytedbConnStr == "" {
// spawn yugabytedb container
var err error
ctx := context.Background()
yugabytedbContainer, err = yugabytedb.Run(
testYugabytedbContainer, err = yugabytedb.Run(
ctx,
"yugabytedb/yugabyte:"+ybVersion,
)
assert.NoError(t, err)
defer yugabytedbContainer.Terminate(context.Background())
defer testYugabytedbContainer.Terminate(context.Background())
}

// run tests
Expand All @@ -158,4 +199,7 @@ func TestDDLIssuesInYBVersion(t *testing.T) {
success = t.Run(fmt.Sprintf("%s-%s", "unlogged table", ybVersion), testUnloggedTableIssue)
assert.True(t, success)

success = t.Run(fmt.Sprintf("%s-%s", "alter table add PK on partition", ybVersion), testAlterTableAddPKOnPartitionIssue)
assert.True(t, success)

}
11 changes: 11 additions & 0 deletions yb-voyager/src/ybversion/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,18 @@ const (

var LatestStable *YBVersion

var V2024_1_0_0 *YBVersion
var V2024_1_3_1 *YBVersion
var V2024_2_0_0 *YBVersion

var V2_23_0_0 *YBVersion

func init() {
var err error
V2024_1_0_0, err = NewYBVersion("2024.1.0.0")
if err != nil {
panic("could not create version 2024.1.0.0")
}
V2024_1_3_1, err = NewYBVersion("2024.1.3.1")
if err != nil {
panic("could not create version 2024.1.3.1")
Expand All @@ -41,5 +48,9 @@ func init() {
panic("could not create version 2024.2.0.0")
}

V2_23_0_0, err = NewYBVersion("2.23.0.0")
if err != nil {
panic("could not create version 2.23.0.0")
}
LatestStable = V2024_2_0_0
}