Skip to content

Commit 7e743f9

Browse files
Slachaider-chat-bot
Slach
and
aider-chat-bot
committed
refactor: add mutex to Config and protect macro application in NewBackupDestination
Co-authored-by: aider (anthropic/claude-opus-4-5-20251101) <aider@aider.chat>
1 parent e7d99ff commit 7e743f9

File tree

4 files changed

+54
-80
lines changed

4 files changed

+54
-80
lines changed

‎pkg/backup/backuper.go‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -253,10 +253,6 @@ func (b *Backuper) getEmbeddedBackupLocation(ctx context.Context, backupName str
253253
if b.cfg.ClickHouse.EmbeddedBackupDisk != "" {
254254
return fmt.Sprintf("Disk('%s','%s')", b.cfg.ClickHouse.EmbeddedBackupDisk, backupName), nil
255255
}
256-
257-
if err := b.applyMacrosToObjectDiskPath(ctx); err != nil {
258-
return "", err
259-
}
260256
if b.cfg.General.RemoteStorage == "s3" {
261257
s3Endpoint, err := b.ch.ApplyMacros(ctx, b.buildEmbeddedLocationS3())
262258
if err != nil {
@@ -296,23 +292,6 @@ func (b *Backuper) getEmbeddedBackupLocation(ctx context.Context, backupName str
296292
return "", fmt.Errorf("empty clickhouse->embedded_backup_disk and invalid general->remote_storage: %s", b.cfg.General.RemoteStorage)
297293
}
298294

299-
func (b *Backuper) applyMacrosToObjectDiskPath(ctx context.Context) error {
300-
var err error
301-
if b.cfg.General.RemoteStorage == "s3" {
302-
b.cfg.S3.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.S3.ObjectDiskPath)
303-
} else if b.cfg.General.RemoteStorage == "gcs" {
304-
b.cfg.GCS.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.GCS.ObjectDiskPath)
305-
} else if b.cfg.General.RemoteStorage == "azblob" {
306-
b.cfg.AzureBlob.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.AzureBlob.ObjectDiskPath)
307-
} else if b.cfg.General.RemoteStorage == "ftp" {
308-
b.cfg.FTP.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.FTP.ObjectDiskPath)
309-
} else if b.cfg.General.RemoteStorage == "sftp" {
310-
b.cfg.SFTP.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.SFTP.ObjectDiskPath)
311-
} else if b.cfg.General.RemoteStorage == "cos" {
312-
b.cfg.COS.ObjectDiskPath, err = b.ch.ApplyMacros(ctx, b.cfg.COS.ObjectDiskPath)
313-
}
314-
return err
315-
}
316295

317296
func (b *Backuper) buildEmbeddedLocationS3() string {
318297
s3backupURL := url.URL{}

‎pkg/backup/restore.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1867,10 +1867,6 @@ func (b *Backuper) restoreDataRegular(ctx context.Context, backupName string, ba
18671867
tablePattern = b.changeTablePatternFromRestoreMapping(tablePattern, "table")
18681868
}
18691869

1870-
if err := b.applyMacrosToObjectDiskPath(ctx); err != nil {
1871-
return err
1872-
}
1873-
18741870
chTables, err := b.ch.GetTables(ctx, tablePattern)
18751871
if err != nil {
18761872
return err

‎pkg/config/config.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"regexp"
99
"runtime"
1010
"strings"
11+
"sync"
1112
"time"
1213

1314
"github.com/Altinity/clickhouse-backup/v2/pkg/log_helper"
@@ -36,6 +37,18 @@ type Config struct {
3637
SFTP SFTPConfig `yaml:"sftp" envconfig:"_"`
3738
AzureBlob AzureBlobConfig `yaml:"azblob" envconfig:"_"`
3839
Custom CustomConfig `yaml:"custom" envconfig:"_"`
40+
// Mutex to protect concurrent access when applying macros
41+
mu sync.Mutex `yaml:"-"`
42+
}
43+
44+
// Lock acquires the config mutex
45+
func (cfg *Config) Lock() {
46+
cfg.mu.Lock()
47+
}
48+
49+
// Unlock releases the config mutex
50+
func (cfg *Config) Unlock() {
51+
cfg.mu.Unlock()
3952
}
4053

4154
// GeneralConfig - general setting section

‎pkg/storage/general.go‎

Lines changed: 41 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -541,128 +541,114 @@ func (bd *BackupDestination) throttleSpeed(startTime time.Time, size int64, maxS
541541

542542
func NewBackupDestination(ctx context.Context, cfg *config.Config, ch *clickhouse.ClickHouse, backupName string) (*BackupDestination, error) {
543543
var err error
544+
// Lock config to safely apply macros and modify config values
545+
cfg.Lock()
546+
defer cfg.Unlock()
547+
544548
switch cfg.General.RemoteStorage {
545549
case "azblob":
546-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
547-
azblobConfig := cfg.AzureBlob
548-
azblobStorage := &AzureBlob{
549-
Config: &azblobConfig,
550-
}
551-
if azblobStorage.Config.Path, err = ch.ApplyMacros(ctx, azblobStorage.Config.Path); err != nil {
550+
if cfg.AzureBlob.Path, err = ch.ApplyMacros(ctx, cfg.AzureBlob.Path); err != nil {
552551
return nil, err
553552
}
554-
if azblobStorage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, azblobStorage.Config.ObjectDiskPath); err != nil {
553+
if cfg.AzureBlob.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.AzureBlob.ObjectDiskPath); err != nil {
555554
return nil, err
556555
}
557-
556+
azblobStorage := &AzureBlob{
557+
Config: &cfg.AzureBlob,
558+
}
558559
return &BackupDestination{
559560
azblobStorage,
560561
cfg.AzureBlob.CompressionFormat,
561562
cfg.AzureBlob.CompressionLevel,
562563
}, nil
563564
case "s3":
564-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
565-
s3Config := cfg.S3
566-
s3Storage := &S3{
567-
Config: &s3Config,
568-
Concurrency: cfg.S3.Concurrency,
569-
BufferSize: 64 * 1024,
570-
}
571-
if s3Storage.Config.Path, err = ch.ApplyMacros(ctx, s3Storage.Config.Path); err != nil {
565+
if cfg.S3.Path, err = ch.ApplyMacros(ctx, cfg.S3.Path); err != nil {
572566
return nil, err
573567
}
574-
if s3Storage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, s3Storage.Config.ObjectDiskPath); err != nil {
568+
if cfg.S3.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.S3.ObjectDiskPath); err != nil {
575569
return nil, err
576570
}
577571
// https://github.com/Altinity/clickhouse-backup/issues/588
578-
if len(s3Storage.Config.ObjectLabels) > 0 && backupName != "" {
579-
objectLabels := s3Storage.Config.ObjectLabels
580-
objectLabels, err = ch.ApplyMacrosToObjectLabels(ctx, objectLabels, backupName)
572+
if len(cfg.S3.ObjectLabels) > 0 && backupName != "" {
573+
cfg.S3.ObjectLabels, err = ch.ApplyMacrosToObjectLabels(ctx, cfg.S3.ObjectLabels, backupName)
581574
if err != nil {
582575
return nil, err
583576
}
584-
s3Storage.Config.ObjectLabels = objectLabels
577+
}
578+
s3Storage := &S3{
579+
Config: &cfg.S3,
580+
Concurrency: cfg.S3.Concurrency,
581+
BufferSize: 64 * 1024,
585582
}
586583
return &BackupDestination{
587584
s3Storage,
588585
cfg.S3.CompressionFormat,
589586
cfg.S3.CompressionLevel,
590587
}, nil
591588
case "gcs":
592-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
593-
gcsConfig := cfg.GCS
594-
googleCloudStorage := &GCS{Config: &gcsConfig}
595-
if googleCloudStorage.Config.Path, err = ch.ApplyMacros(ctx, googleCloudStorage.Config.Path); err != nil {
589+
if cfg.GCS.Path, err = ch.ApplyMacros(ctx, cfg.GCS.Path); err != nil {
596590
return nil, err
597591
}
598-
if googleCloudStorage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, googleCloudStorage.Config.ObjectDiskPath); err != nil {
592+
if cfg.GCS.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.GCS.ObjectDiskPath); err != nil {
599593
return nil, err
600594
}
601595
// https://github.com/Altinity/clickhouse-backup/issues/588
602-
if len(googleCloudStorage.Config.ObjectLabels) > 0 && backupName != "" {
603-
objectLabels := googleCloudStorage.Config.ObjectLabels
604-
objectLabels, err = ch.ApplyMacrosToObjectLabels(ctx, objectLabels, backupName)
596+
if len(cfg.GCS.ObjectLabels) > 0 && backupName != "" {
597+
cfg.GCS.ObjectLabels, err = ch.ApplyMacrosToObjectLabels(ctx, cfg.GCS.ObjectLabels, backupName)
605598
if err != nil {
606599
return nil, err
607600
}
608-
googleCloudStorage.Config.ObjectLabels = objectLabels
609601
}
602+
googleCloudStorage := &GCS{Config: &cfg.GCS}
610603
return &BackupDestination{
611604
googleCloudStorage,
612605
cfg.GCS.CompressionFormat,
613606
cfg.GCS.CompressionLevel,
614607
}, nil
615608
case "cos":
616-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
617-
cosConfig := cfg.COS
618-
tencentStorage := &COS{
619-
Config: &cosConfig,
620-
BufferSize: 64 * 1024,
621-
}
622-
if tencentStorage.Config.Path, err = ch.ApplyMacros(ctx, tencentStorage.Config.Path); err != nil {
609+
if cfg.COS.Path, err = ch.ApplyMacros(ctx, cfg.COS.Path); err != nil {
623610
return nil, err
624611
}
625-
if tencentStorage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, tencentStorage.Config.ObjectDiskPath); err != nil {
612+
if cfg.COS.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.COS.ObjectDiskPath); err != nil {
626613
return nil, err
627614
}
615+
tencentStorage := &COS{
616+
Config: &cfg.COS,
617+
BufferSize: 64 * 1024,
618+
}
628619
return &BackupDestination{
629620
tencentStorage,
630621
cfg.COS.CompressionFormat,
631622
cfg.COS.CompressionLevel,
632623
}, nil
633624
case "ftp":
634-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
635-
ftpConfig := cfg.FTP
636-
if ftpConfig.Concurrency < cfg.General.ObjectDiskServerSideCopyConcurrency/4 {
637-
ftpConfig.Concurrency = cfg.General.ObjectDiskServerSideCopyConcurrency
638-
}
639-
ftpStorage := &FTP{
640-
Config: &ftpConfig,
625+
if cfg.FTP.Concurrency < cfg.General.ObjectDiskServerSideCopyConcurrency/4 {
626+
cfg.FTP.Concurrency = cfg.General.ObjectDiskServerSideCopyConcurrency
641627
}
642-
if ftpStorage.Config.Path, err = ch.ApplyMacros(ctx, ftpStorage.Config.Path); err != nil {
628+
if cfg.FTP.Path, err = ch.ApplyMacros(ctx, cfg.FTP.Path); err != nil {
643629
return nil, err
644630
}
645-
if ftpStorage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, ftpStorage.Config.ObjectDiskPath); err != nil {
631+
if cfg.FTP.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.FTP.ObjectDiskPath); err != nil {
646632
return nil, err
647633
}
634+
ftpStorage := &FTP{
635+
Config: &cfg.FTP,
636+
}
648637
return &BackupDestination{
649638
ftpStorage,
650639
cfg.FTP.CompressionFormat,
651640
cfg.FTP.CompressionLevel,
652641
}, nil
653642
case "sftp":
654-
// Copy config to avoid data race when multiple goroutines call NewBackupDestination
655-
sftpConfig := cfg.SFTP
656-
sftpStorage := &SFTP{
657-
Config: &sftpConfig,
658-
}
659-
if sftpStorage.Config.Path, err = ch.ApplyMacros(ctx, sftpStorage.Config.Path); err != nil {
643+
if cfg.SFTP.Path, err = ch.ApplyMacros(ctx, cfg.SFTP.Path); err != nil {
660644
return nil, err
661645
}
662-
if sftpStorage.Config.ObjectDiskPath, err = ch.ApplyMacros(ctx, sftpStorage.Config.ObjectDiskPath); err != nil {
646+
if cfg.SFTP.ObjectDiskPath, err = ch.ApplyMacros(ctx, cfg.SFTP.ObjectDiskPath); err != nil {
663647
return nil, err
664648
}
665-
649+
sftpStorage := &SFTP{
650+
Config: &cfg.SFTP,
651+
}
666652
return &BackupDestination{
667653
sftpStorage,
668654
cfg.SFTP.CompressionFormat,

0 commit comments

Comments
 (0)