Merge #143069 #143691

143069: cloud/amazon: add option to skip checksums r=dt a=dt

Our files (SSTs) often have internal checksums anyway, and we have ecplicit checksum files for those that don't that are cloud agnostic, so we do not generally need the extra checksums in s3-specific metadata. Computing them has non-zero cost as well, so users may wish to disable them. Indeed, we almost did so by default a couple years ago before realizing that they are required by the S3 API if certain other features, like object locking, are enabled. So while some users may not be able to or want to disable them, e.g. if they're using object locking, we can provide the option to do so to those who would want it. This may also be useful to users of certain s3-like storage services that have different compatibility with different checksums.

Release note: none.
Epic: none.

143691: roachtest/pg_regress: update diffs r=yuzefovich a=yuzefovich

Fixes: #143683.

Release note: None

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: 3 people

pkg/cloud/amazon/s3_storage.go

@@ -57,6 +57,8 @@ const (
 	AWSEndpointParam = "AWS_ENDPOINT"
 	// AWSEndpointParam is the query parameter for UsePathStyle in S3 options.
 	AWSUsePathStyle = "AWS_USE_PATH_STYLE"
+	// AWSSkipChecksumParam is the query parameter for SkipChecksum in S3 options.
+	AWSSkipChecksumParam = "AWS_SKIP_CHECKSUM"
 
 	// AWSServerSideEncryptionMode is the query parameter in an AWS URI, for the
 	// mode to be used for server side encryption. It can either be AES256 or
@@ -200,6 +202,7 @@ type s3ClientConfig struct {
 	assumeRoleProvider                                           roleProvider
 	delegateRoleProviders                                        []roleProvider
 
+	skipChecksum bool
 	// log.V(2) decides session init params so include it in key.
 	verbose bool
 }
@@ -232,6 +235,7 @@ func clientConfig(conf *cloudpb.ExternalStorage_S3) s3ClientConfig {
 	return s3ClientConfig{
 		endpoint:              conf.Endpoint,
 		usePathStyle:          conf.UsePathStyle,
+		skipChecksum:          conf.SkipChecksum,
 		region:                conf.Region,
 		bucket:                conf.Bucket,
 		accessKey:             conf.AccessKey,
@@ -280,6 +284,9 @@ func S3URI(bucket, path string, conf *cloudpb.ExternalStorage_S3) string {
 	if conf.UsePathStyle {
 		q.Set(AWSUsePathStyle, "true")
 	}
+	if conf.SkipChecksum {
+		q.Set(AWSSkipChecksumParam, "true")
+	}
 	if conf.AssumeRoleProvider.Role != "" {
 		roleProviderStrings := make([]string, 0, len(conf.DelegateRoleProviders)+1)
 		for _, p := range conf.DelegateRoleProviders {
@@ -325,6 +332,15 @@ func parseS3URL(uri *url.URL) (cloudpb.ExternalStorage, error) {
 			return cloudpb.ExternalStorage{}, errors.Wrapf(err, "cannot parse %s as bool", AWSUsePathStyle)
 		}
 	}
+	skipChecksumStr := s3URL.ConsumeParam(AWSSkipChecksumParam)
+	skipChecksumBool := false
+	if skipChecksumStr != "" {
+		var err error
+		skipChecksumBool, err = strconv.ParseBool(skipChecksumStr)
+		if err != nil {
+			return cloudpb.ExternalStorage{}, errors.Wrapf(err, "cannot parse %s as bool", AWSSkipChecksumParam)
+		}
+	}
 
 	conf.S3Config = &cloudpb.ExternalStorage_S3{
 		Bucket:                s3URL.Host,
@@ -334,6 +350,7 @@ func parseS3URL(uri *url.URL) (cloudpb.ExternalStorage, error) {
 		TempToken:             s3URL.ConsumeParam(AWSTempTokenParam),
 		Endpoint:              s3URL.ConsumeParam(AWSEndpointParam),
 		UsePathStyle:          pathStyleBool,
+		SkipChecksum:          skipChecksumBool,
 		Region:                s3URL.ConsumeParam(S3RegionParam),
 		Auth:                  s3URL.ConsumeParam(cloud.AuthParam),
 		ServerEncMode:         s3URL.ConsumeParam(AWSServerSideEncryptionMode),
@@ -590,6 +607,11 @@ func (s *s3Storage) newClient(ctx context.Context) (s3Client, string, error) {
 		return s3Client{}, "", errors.Wrap(err, "could not initialize an aws config")
 	}
 
+	if s.opts.skipChecksum {
+		cfg.ResponseChecksumValidation = aws.ResponseChecksumValidationWhenRequired
+		cfg.RequestChecksumCalculation = aws.RequestChecksumCalculationWhenRequired
+	}
+
 	var endpointURI string
 	if s.opts.endpoint != "" {
 		var err error
@@ -728,7 +750,7 @@ func (s *s3Storage) putUploader(ctx context.Context, basename string) (io.WriteC
 
 	buf := bytes.NewBuffer(make([]byte, 0, 4<<20))
 
-	return &putUploader{
+	uploader := &putUploader{
 		b: buf,
 		input: &s3.PutObjectInput{
 			Bucket:               s.bucket,
@@ -739,7 +761,11 @@ func (s *s3Storage) putUploader(ctx context.Context, basename string) (io.WriteC
 			ChecksumAlgorithm:    checksumAlgorithm,
 		},
 		client: client,
-	}, nil
+	}
+	if s.conf.SkipChecksum {
+		uploader.input.ChecksumAlgorithm = ""
+	}
+	return uploader, nil
 }
 
 func (s *s3Storage) Writer(ctx context.Context, basename string) (io.WriteCloser, error) {
@@ -757,16 +783,21 @@ func (s *s3Storage) Writer(ctx context.Context, basename string) (io.WriteCloser
 	return cloud.BackgroundPipe(ctx, func(ctx context.Context, r io.Reader) error {
 		defer sp.Finish()
 		// Upload the file to S3.
-		// TODO(dt): test and tune the uploader parameters.
-		_, err := uploader.Upload(ctx, &s3.PutObjectInput{
+		input := &s3.PutObjectInput{
 			Bucket:               s.bucket,
 			Key:                  aws.String(path.Join(s.prefix, basename)),
 			Body:                 r,
 			ServerSideEncryption: types.ServerSideEncryption(s.conf.ServerEncMode),
 			SSEKMSKeyId:          nilIfEmpty(s.conf.ServerKMSID),
 			StorageClass:         types.StorageClass(s.conf.StorageClass),
 			ChecksumAlgorithm:    checksumAlgorithm,
-		})
+		}
+
+		if s.conf.SkipChecksum {
+			input.ChecksumAlgorithm = ""
+		}
+
+		_, err := uploader.Upload(ctx, input)
 		err = interpretAWSError(err)
 		err = errors.Wrap(err, "upload failed")
 		// Mark with ctx's error for upstream code to not interpret this as
@@ -787,7 +818,9 @@ func (s *s3Storage) openStreamAt(
 	if err != nil {
 		return nil, err
 	}
-	req := &s3.GetObjectInput{Bucket: s.bucket, Key: aws.String(path.Join(s.prefix, basename))}
+	req := &s3.GetObjectInput{
+		Bucket: s.bucket, Key: aws.String(path.Join(s.prefix, basename)),
+	}
 	if endPos != 0 {
 		if pos >= endPos {
 			return nil, io.EOF

pkg/cloud/amazon/s3_storage_test.go

@@ -167,6 +167,22 @@ func TestPutS3(t *testing.T) {
 				cloudtestutils.CheckExportStore(t, info)
 			})
 
+			t.Run("skip-checksums", func(t *testing.T) {
+				if locked {
+					skip.IgnoreLint(t, "object-locked buckets do not support skipping checksums")
+				}
+				skipIfNoDefaultConfig(t, ctx)
+				info := cloudtestutils.StoreInfo{
+					URI: fmt.Sprintf(
+						"s3://%s/%s-%d?%s=%s&%s=true",
+						bucket, "backup-test-skip-checksums", testID,
+						cloud.AuthParam, cloud.AuthParamImplicit, AWSSkipChecksumParam,
+					),
+					User: user,
+				}
+				cloudtestutils.CheckExportStore(t, info)
+			})
+
 			t.Run("server-side-encryption-invalid-params", func(t *testing.T) {
 				skipIfNoDefaultConfig(t, ctx)
 				// Unsupported server side encryption option.
@@ -357,6 +373,7 @@ func TestPutS3Endpoint(t *testing.T) {
 		}
 		cloudtestutils.CheckExportStore(t, info)
 	})
+
 	t.Run("use-path-style", func(t *testing.T) {
 		// EngFlow machines have no internet access, and queries even to localhost will time out.
 		// So this test is skipped above.

pkg/cloud/cloudpb/external_storage.proto

@@ -63,6 +63,7 @@ message ExternalStorage {
     string temp_token = 5;
     string endpoint = 6;
     bool use_path_style=16;
+    bool skip_checksum=17;
     string region = 7;
     string auth = 8;
     string server_enc_mode  = 9;
@@ -91,7 +92,10 @@ message ExternalStorage {
     // role chain. These roles will be assumed in the order they appear in the
     // list so that the role specified in AssumeRoleProvider can be assumed.
     repeated AssumeRoleProvider delegate_role_providers = 15 [(gogoproto.nullable) = false];
+
+    // Next ID: 18;
   }
+  
   message GCS {
     string bucket = 1;
     string prefix = 2;

pkg/cmd/roachtest/testdata/pg_regress/alter_table.diffs

@@ -1630,7 +1630,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/alter_table.out -
 -ERROR:  column "test" is in a primary key
 +ERROR:  column "test" is in a primary index
  alter table atacc1 drop constraint "atacc1_pkey";
-+ERROR:  relation "atacc1" (1956): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction
++ERROR:  relation "atacc1" (1984): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction
 +HINT:  You have attempted to use a feature that is not yet implemented.
 +See: https://go.crdb.dev/issue-v/48026/_version_
  alter table atacc1 alter column test drop not null;
@@ -5216,13 +5216,13 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/alter_table.out -
 +ERROR:  index with name "tt9_c_key" already exists
  ALTER TABLE tt9 ADD CONSTRAINT foo UNIQUE(c);  -- fail, dup name
 -ERROR:  constraint "foo" for relation "tt9" already exists
-+ERROR:  error executing StatementPhase stage 1 of 1 with 7 MutationType ops: relation "tt9" (2051): duplicate constraint name: "foo"
++ERROR:  error executing StatementPhase stage 1 of 1 with 7 MutationType ops: relation "tt9" (2079): duplicate constraint name: "foo"
  ALTER TABLE tt9 ADD CONSTRAINT tt9_c_key CHECK(c > 5);  -- fail, dup name
 -ERROR:  constraint "tt9_c_key" for relation "tt9" already exists
-+ERROR:  error executing StatementPhase stage 1 of 1 with 2 MutationType ops: relation "tt9" (2051): duplicate constraint name: "tt9_c_key"
++ERROR:  error executing StatementPhase stage 1 of 1 with 2 MutationType ops: relation "tt9" (2079): duplicate constraint name: "tt9_c_key"
  ALTER TABLE tt9 ADD CONSTRAINT tt9_c_key2 CHECK(c > 6);
  ALTER TABLE tt9 ADD UNIQUE(c);  -- picks nonconflicting name
-+ERROR:  error executing StatementPhase stage 1 of 1 with 7 MutationType ops: relation "tt9" (2051): duplicate constraint name: "tt9_c_key2"
++ERROR:  error executing StatementPhase stage 1 of 1 with 7 MutationType ops: relation "tt9" (2079): duplicate constraint name: "tt9_c_key2"
  \d tt9
 -                Table "public.tt9"
 - Column |  Type   | Collation | Nullable | Default 
@@ -5404,7 +5404,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/alter_table.out -
  DELETE FROM old_system_table WHERE othercol = 'somedata';
  TRUNCATE old_system_table;
  ALTER TABLE old_system_table DROP CONSTRAINT new_system_table_pkey;
-+ERROR:  relation "old_system_table" (2054): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction
++ERROR:  relation "old_system_table" (2082): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction
 +HINT:  You have attempted to use a feature that is not yet implemented.
 +See: https://go.crdb.dev/issue-v/48026/_version_
  ALTER TABLE old_system_table DROP COLUMN othercol;

pkg/cmd/roachtest/testdata/pg_regress/create_table_like.diffs

@@ -54,7 +54,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/create_table_like
 
  CREATE TABLE inhf (LIKE inhx, LIKE inhx); /* Throw error */
 -ERROR:  column "xx" specified more than once
-+ERROR:  relation "inhf" (1332): duplicate column name: "xx"
++ERROR:  relation "inhf" (1360): duplicate column name: "xx"
  CREATE TABLE inhf (LIKE inhx INCLUDING DEFAULTS INCLUDING CONSTRAINTS);
  INSERT INTO inhf DEFAULT VALUES;
  SELECT * FROM inhf; /* Single entry with value 'text' */

pkg/cmd/roachtest/testdata/pg_regress/float8.diffs

@@ -191,7 +191,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT power(float8 '-inf', float8 'inf');
    power   
  ----------
-@@ -631,29 +592,60 @@
+@@ -631,9 +592,25 @@
     SET f1 = FLOAT8_TBL.f1 * '-1'
     WHERE FLOAT8_TBL.f1 > '0.0';
  SELECT f.f1 * '1e200' from FLOAT8_TBL f;
@@ -207,20 +207,21 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
 +
  SELECT f.f1 ^ '1e200' from FLOAT8_TBL f;
 -ERROR:  value out of range: overflow
--SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
-  ?column? 
- ----------
--        2
--(1 row)
++ ?column? 
++----------
 +        0
 + Infinity
 + Infinity
 + Infinity
 +        0
 +(5 rows)
++
+ SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
+  ?column? 
+ ----------
+@@ -641,19 +618,38 @@
+ (1 row)
 
-+SELECT 0 ^ 0 + 0 ^ 1 + 0 ^ 0.0 + 0 ^ 0.5;
-+ERROR:  integer out of range
  SELECT ln(f.f1) from FLOAT8_TBL f where f.f1 = '0.0' ;
 -ERROR:  cannot take logarithm of zero
 +    ln     
@@ -261,7 +262,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
   -1.2345678901234e+200
   -1.2345678901234e-200
  (5 rows)
-@@ -773,7 +765,11 @@
+@@ -773,7 +769,11 @@
  -- acosh(Inf) should be Inf, but some mingw versions produce NaN, so skip test
  -- SELECT acosh(float8 'infinity');
  SELECT acosh(float8 '-infinity');
@@ -274,7 +275,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT acosh(float8 'nan');
   acosh 
  -------
-@@ -781,9 +777,17 @@
+@@ -781,9 +781,17 @@
  (1 row)
 
  SELECT atanh(float8 'infinity');
@@ -294,7 +295,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT atanh(float8 'nan');
   atanh 
  -------
-@@ -803,50 +807,15 @@
+@@ -803,50 +811,15 @@
        (1.2e-17), (2.3e-13), (1.2e-9),
        (0.45), (1.1), (2.1), (3.4), (6), (28),
        (float8 'infinity'), (float8 'nan')) AS t(x);
@@ -348,7 +349,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  DROP TABLE FLOAT8_TBL;
  -- Check the float8 values exported for use by other tests
  SELECT * FROM FLOAT8_TBL;
-@@ -867,7 +836,7 @@
+@@ -867,7 +840,7 @@
  (1 row)
 
  SELECT '32767.6'::float8::int2;
@@ -357,7 +358,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT '-32768.4'::float8::int2;
    int2  
  --------
-@@ -875,7 +844,7 @@
+@@ -875,7 +848,7 @@
  (1 row)
 
  SELECT '-32768.6'::float8::int2;
@@ -366,7 +367,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT '2147483647.4'::float8::int4;
      int4    
  ------------
-@@ -883,7 +852,7 @@
+@@ -883,7 +856,7 @@
  (1 row)
 
  SELECT '2147483647.6'::float8::int4;
@@ -375,7 +376,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT '-2147483648.4'::float8::int4;
      int4     
  -------------
-@@ -891,7 +860,7 @@
+@@ -891,7 +864,7 @@
  (1 row)
 
  SELECT '-2147483648.6'::float8::int4;
@@ -384,7 +385,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT '9223372036854773760'::float8::int8;
          int8         
  ---------------------
-@@ -899,52 +868,26 @@
+@@ -899,52 +872,26 @@
  (1 row)
 
  SELECT '9223372036854775807'::float8::int8;
@@ -444,7 +445,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  SELECT x,
         tand(x),
         tand(x) IN ('-Infinity'::float8,-1,0,
-@@ -954,79 +897,117 @@
+@@ -954,79 +901,117 @@
                     1,'Infinity'::float8) AS cotd_exact
  FROM (VALUES (0), (45), (90), (135), (180),
        (225), (270), (315), (360)) AS t(x);
@@ -604,7 +605,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  -- float8: seeeeeee eeeeeeee eeeeeeee mmmmmmmm mmmmmmmm(x4)
  -- we don't care to assume the platform's strtod() handles subnormals
  -- correctly; those are "use at your own risk". However we do test
-@@ -1051,28 +1032,7 @@
+@@ -1051,28 +1036,7 @@
    from (select bits::bigint::xfloat8::float8 as flt
            from testdata
  	offset 0) s;
@@ -634,7 +635,7 @@ diff -U3 --label=/mnt/data1/postgres/src/test/regress/expected/float8.out --labe
  -- round-trip tests
  with testdata(bits) as (values
    (x'0000000000000000'),
-@@ -1220,225 +1180,9 @@
+@@ -1220,225 +1184,9 @@
    from (select bits::bigint::xfloat8::float8 as flt
            from testdata
  	offset 0) s;