fix more corner cases for check_parts_columns: true, fix #747

ChangeLog.md

@@ -4,6 +4,7 @@ IMPROVEMENTS
 
 BUG FIXES
 - fix restore for object disk frozen_metadata.txt fix [752](https://github.com/Altinity/clickhouse-backup/issues/752)
+- fix more corner cases for `check_parts_columns: true`, fix [747](https://github.com/Altinity/clickhouse-backup/issues/747)
 
 # v2.4.1
 IMPROVEMENTS

pkg/clickhouse/clickhouse.go

@@ -1152,24 +1152,11 @@ func (ch *ClickHouse) CheckReplicationInProgress(table metadata.TableMetadata) (
 	return true, nil
 }
 
+var isDateWithTzRE = regexp.MustCompile(`^Date[^(]+\(.+\)`)
+
 // CheckSystemPartsColumns check data parts types consistency https://github.com/Altinity/clickhouse-backup/issues/529#issuecomment-1554460504
 func (ch *ClickHouse) CheckSystemPartsColumns(ctx context.Context, table *Table) error {
-	if ch.isPartsColumnPresent == -1 {
-		return nil
-	}
-	if ch.isPartsColumnPresent == 0 {
-		isPartsColumn := make([]struct {
-			Present uint64 `ch:"is_parts_column_present"`
-		}, 0)
-		if err := ch.SelectContext(ctx, &isPartsColumn, "SELECT count() is_parts_column_present FROM system.tables WHERE database='system' AND name='parts_columns'"); err != nil {
-			return err
-		}
-		if len(isPartsColumn) != 1 || isPartsColumn[0].Present != 1 {
-			ch.isPartsColumnPresent = -1
-			return nil
-		}
-	}
-	ch.isPartsColumnPresent = 1
+	var err error
 	partColumnsDataTypes := make([]struct {
 		Column string   `ch:"column"`
 		Types  []string `ch:"uniq_types"`
@@ -1178,22 +1165,51 @@ func (ch *ClickHouse) CheckSystemPartsColumns(ctx context.Context, table *Table)
 		"FROM system.parts_columns " +
 		"WHERE active AND database=? AND table=? AND type NOT LIKE 'Enum%' AND type NOT LIKE 'Tuple(%' " +
 		"GROUP BY column HAVING length(uniq_types) > 1"
-	if err := ch.SelectContext(ctx, &partColumnsDataTypes, partsColumnsSQL, table.Database, table.Name); err != nil {
+	if err = ch.SelectContext(ctx, &partColumnsDataTypes, partsColumnsSQL, table.Database, table.Name); err != nil {
 		return err
 	}
 	isPartColumnsInconsistentDataTypes := false
 	if len(partColumnsDataTypes) > 0 {
 		for i := range partColumnsDataTypes {
 			isNullablePresent := false
 			isNotNullablePresent := false
+			isDatePresent := false
+			isDateTZPresent := false
+			var baseDataType string
 			for _, dataType := range partColumnsDataTypes[i].Types {
+				currentDataType := dataType
+				for _, compatiblePrefix := range []string{"Nullable(", "LowCardinality("} {
+					if strings.HasPrefix(currentDataType, compatiblePrefix) {
+						currentDataType = strings.TrimPrefix(currentDataType, compatiblePrefix)
+						currentDataType = strings.TrimSuffix(currentDataType, ")")
+					}
+				}
+				if strings.Contains(currentDataType, "(") {
+					currentDataType = currentDataType[:strings.Index(currentDataType, "(")]
+				}
+				if baseDataType == "" {
+					baseDataType = currentDataType
+				} else if baseDataType != currentDataType {
+					ch.Log.Errorf("`%s`.`%s` have incompatible data types %#v for \"%s\" column", baseDataType, currentDataType, table.Database, table.Name, partColumnsDataTypes[i].Types, partColumnsDataTypes[i].Column)
+					isPartColumnsInconsistentDataTypes = true
+					break
+				}
 				if strings.Contains(dataType, "Nullable") {
 					isNullablePresent = true
 				} else {
 					isNotNullablePresent = true
 				}
+				if strings.HasPrefix(dataType, "Date") {
+					isDatePresent = true
+					if !isDateTZPresent {
+						isDateTZPresent = isDateWithTzRE.MatchString(dataType)
+					}
+				}
 			}
 			if !isNullablePresent && isNotNullablePresent {
+				if isDatePresent && isDateTZPresent {
+					continue
+				}
 				ch.Log.Errorf("`%s`.`%s` have inconsistent data types %#v for \"%s\" column", table.Database, table.Name, partColumnsDataTypes[i].Types, partColumnsDataTypes[i].Column)
 				isPartColumnsInconsistentDataTypes = true
 			}

pkg/config/config.go

@@ -331,12 +331,10 @@ func LoadConfig(configLocation string) (*Config, error) {
 			return cfg, err
 		}
 		if err = gionice.SetIDPri(0, nicePriority, 7, gionice.IOPRIO_WHO_PGRP); err != nil {
-			log.Fatalf("SUKA1 %v", err)
 			return cfg, err
 		}
 	}
 	if err = gionice.SetNicePri(0, gionice.PRIO_PROCESS, cfg.General.CPUNicePriority); err != nil {
-		log.Fatalf("SUKA2 cfg.General.CPUNicePriority=%d, err=%v", cfg.General.CPUNicePriority, err)
 		return cfg, err
 	}
 

test/integration/integration_test.go

@@ -1168,6 +1168,51 @@ func TestProjections(t *testing.T) {
 
 }
 
+func TestCheckSystemPartsColumns(t *testing.T) {
+	var err error
+	if compareVersion(os.Getenv("CLICKHOUSE_VERSION"), "23.3") == -1 {
+		t.Skipf("Test skipped, system.parts_columns have inconsistency only in 23.3+, current version %s", os.Getenv("CLICKHOUSE_VERSION"))
+	}
+	//t.Parallel()
+	ch := &TestClickHouse{}
+	r := require.New(t)
+	ch.connectWithWait(r, 0*time.Second, 1*time.Second)
+	defer ch.chbackend.Close()
+	r.NoError(dockerCP("config-s3.yml", "clickhouse-backup:/etc/clickhouse-backup/config.yml"))
+	version, err := ch.chbackend.GetVersion(context.Background())
+	r.NoError(err)
+	ch.queryWithNoError(r, "CREATE DATABASE IF NOT EXISTS "+t.Name())
+
+	// test compatible data types
+	createSQL := "CREATE TABLE " + t.Name() + ".test_system_parts_columns(dt DateTime, v UInt64) ENGINE=MergeTree() ORDER BY tuple()"
+	ch.queryWithNoError(r, createSQL)
+	ch.queryWithNoError(r, "INSERT INTO "+t.Name()+".test_system_parts_columns SELECT today() - INTERVAL number DAY, number FROM numbers(10)")
+
+	ch.queryWithNoError(r, "ALTER TABLE "+t.Name()+".test_system_parts_columns MODIFY COLUMN dt Nullable(DateTime('Europe/Moscow')), MODIFY COLUMN v Nullable(UInt64)", t.Name())
+	ch.queryWithNoError(r, "INSERT INTO "+t.Name()+".test_system_parts_columns SELECT today() - INTERVAL number DAY, number FROM numbers(10)")
+	r.NoError(dockerExec("clickhouse-backup", "clickhouse-backup", "create", "test_system_parts_columns"))
+	r.NoError(dockerExec("clickhouse-backup", "clickhouse-backup", "delete", "local", "test_system_parts_columns"))
+	r.NoError(ch.chbackend.DropTable(clickhouse.Table{Database: t.Name(), Name: "test_system_parts_columns"}, createSQL, "", false, version, ""))
+
+	// test incompatible data types
+	ch.queryWithNoError(r, "CREATE TABLE "+t.Name()+".test_system_parts_columns(dt Date, v String) ENGINE=MergeTree() PARTITION BY dt ORDER BY tuple()")
+	ch.queryWithNoError(r, "INSERT INTO "+t.Name()+".test_system_parts_columns SELECT today() - INTERVAL number DAY, if(number>0,'a',toString(number)) FROM numbers(2)")
+
+	mutationSQL := "ALTER TABLE " + t.Name() + ".test_system_parts_columns MODIFY COLUMN v UInt64"
+	err = ch.chbackend.QueryContext(context.Background(), mutationSQL)
+	if err != nil {
+		errStr := strings.ToLower(err.Error())
+		r.True(strings.Contains(errStr, "code: 341") || strings.Contains(errStr, "code: 517") || strings.Contains(errStr, "code: 524") || strings.Contains(errStr, "timeout"), "UNKNOWN ERROR: %s", err.Error())
+		t.Logf("%s RETURN EXPECTED ERROR=%#v", mutationSQL, err)
+	}
+	ch.queryWithNoError(r, "INSERT INTO "+t.Name()+".test_system_parts_columns SELECT today() - INTERVAL number DAY, number FROM numbers(10)")
+	r.Error(dockerExec("clickhouse-backup", "clickhouse-backup", "create", "test_system_parts_columns"))
+	r.Error(dockerExec("clickhouse-backup", "clickhouse-backup", "delete", "local", "test_system_parts_columns"))
+
+	r.NoError(ch.chbackend.DropTable(clickhouse.Table{Database: t.Name(), Name: "test_system_parts_columns"}, createSQL, "", false, version, ""))
+	r.NoError(ch.dropDatabase(t.Name()))
+
+}
 func TestKeepBackupRemoteAndDiffFromRemote(t *testing.T) {
 	if isTestShouldSkip("RUN_ADVANCED_TESTS") {
 		t.Skip("Skipping Advanced integration tests...")
@@ -1218,8 +1263,7 @@ func TestSyncReplicaTimeout(t *testing.T) {
 	ch.connectWithWait(r, 0*time.Millisecond, 2*time.Second)
 	defer ch.chbackend.Close()
 
-	createDbSQL := "CREATE DATABASE IF NOT EXISTS " + t.Name()
-	ch.queryWithNoError(r, createDbSQL)
+	ch.queryWithNoError(r, "CREATE DATABASE IF NOT EXISTS "+t.Name())
 	dropReplTables := func() {
 		for _, table := range []string{"repl1", "repl2"} {
 			query := "DROP TABLE IF EXISTS " + t.Name() + "." + table