Update comments, function name, and file name for clarity

Author: marcleblanc2

cmd/src/snapshot_upload.go

@@ -152,30 +152,30 @@ func copyDumpToBucket(ctx context.Context, src io.ReadSeeker, stat fs.FileInfo,
 	// To assert against actual file size
 	var totalWritten int64
 
-	// Do a partial copy that trims out unwanted statements
+	// Do a partial copy, that filters out incompatible statements
 	if trimExtensions {
-		written, err := pgdump.CommentOutInvalidLines(object, src, progressFn)
+		written, err := pgdump.FilterInvalidLines(object, src, progressFn)
 		if err != nil {
-			return errors.Wrap(err, "trim extensions and upload")
+			return errors.Wrap(err, "filter out incompatible statements and upload")
 		}
 		totalWritten += written
 	}
 
-	// io.Copy is the best way to copy from a reader to writer in Go, and storage.Writer
-	// has its own chunking mechanisms internally. io.Reader is stateful, so this copy
-	// will just continue from where we left off if we use copyAndTrimExtensions.
+	// io.Copy is the best way to copy from a reader to writer in Go,
+	// storage.Writer has its own chunking mechanisms internally.
+	// io.Reader is stateful, so this copy will just continue from where FilterInvalidLines left off, if used
 	written, err := io.Copy(object, src)
 	if err != nil {
 		return errors.Wrap(err, "upload")
 	}
 	totalWritten += written
 
-	// Progress is not called on completion of io.Copy, so we call it manually after to
-	// update our pretty progress bars.
+	// Progress is not called on completion of io.Copy,
+	// so we call it manually after to update our pretty progress bars.
 	progressFn(written)
 
-	// Validate we have sent all data. copyAndTrimExtensions may add some bytes, so the
-	// check is not a strict equality.
+	// Validate we have sent all data.
+	// FilterInvalidLines may add some bytes, so the check is not a strict equality.
 	size := stat.Size()
 	if totalWritten < size {
 		return errors.Newf("expected to write %d bytes, but actually wrote %d bytes (diff: %d bytes)",

internal/pgdump/extensions.go renamed to internal/pgdump/pgdump_sql_filter.go

@@ -8,21 +8,22 @@ import (
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
-// CommentOutInvalidLines will comment out lines in the customer's SQL database dump file
-// which gcloud sql import errors out on
+// FilterInvalidLines copies the initial lines of the pg_dump-created .sql files,
+// from src to dst (the GCS bucket),
+// until it hits a line prefixed with a filterEndMarker,
+// while commenting out the linesToFilter which cause `gcloud sql import` to error out.
+// It then resets src to the position of the last contents written to dst.
 //
-// It performs a partial copy of a SQL database dump from
-// src to dst while commenting out the problematic lines.
-// When it determines there are no more EXTENSIONs-related statements,
-// it will return, resetting src to the position of the last contents written to dst.
+// Filtering requires reading entire lines into memory,
+// this can be a very expensive operation, so when filtering is complete,
+// the more efficient io.Copy is used to perform the remainder of the copy in the calling funciton
 //
-// This is needed for import to Google Cloud Storage, which does not like many statements which pg_dump may insert
-// For more details, see https://cloud.google.com/sql/docs/postgres/import-export/import-export-dmp
-//
-// Filtering requires reading entire lines into memory - this can be a very expensive
-// operation, so when filtering is complete, the more efficient io.Copy should be used
-// to perform the remainder of the copy from src to dst.
-func CommentOutInvalidLines(dst io.Writer, src io.ReadSeeker, progressFn func(int64)) (int64, error) {
+// pg_dump writes these .sql files based on its own version,
+// not based on the Postgres version of either the source or destination database;
+// so self-hosted customers' diverse database environments
+// have inserted a variety of statements into the .sql files which cause the import to fail
+// For details, see https://cloud.google.com/sql/docs/postgres/import-export/import-export-dmp
+func FilterInvalidLines(dst io.Writer, src io.ReadSeeker, progressFn func(int64)) (int64, error) {
 	var (
 		reader = bufio.NewReader(src)
 
@@ -60,8 +61,15 @@ func CommentOutInvalidLines(dst io.Writer, src io.ReadSeeker, progressFn func(in
 			"SET transaction_timeout", // pg_dump v17, importing to Postgres 16
 
 			"\\connect",
+
+			// Cloud instances' databases have been upgraded to Postgres v16.10,
+			// which should include support for \restrict and \unrestrict
+			// but leaving in the list in case we need to re-add them
 			// "\\restrict",
+			// To handle the \unrestrict command,
+			// we'd have to add a search from the end of the file
 			// "\\unrestrict",
+			// Remove comments after databases are upgraded >= Postgres 17
 		}
 	)
 
@@ -71,8 +79,8 @@ func CommentOutInvalidLines(dst io.Writer, src io.ReadSeeker, progressFn func(in
 		line, err := reader.ReadBytes('\n')
 		consumed += int64(len(line))
 
-		// If this function has read through the whole file,
-		// then hand the last line
+		// If this function has read through the whole file without hitting a filterEndMarker,
+		// then handle the last line correctly
 		if err == io.EOF {
 			noMoreLinesToFilter = true
 

internal/pgdump/extensions_test.go renamed to internal/pgdump/pgdump_sql_filter_test.go

@@ -22,7 +22,7 @@ func createTestFile(t *testing.T, content string) *os.File {
 	return src
 }
 
-func TestCommentOutInvalidLines(t *testing.T) {
+func TestFilterInvalidLines(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("Test doesn't work on Windows of weirdness with t.TempDir() handling")
 	}
@@ -187,7 +187,7 @@ CREATE TABLE "public"."access_tokens" (
 			src := createTestFile(t, tt.input)
 			var dst bytes.Buffer
 
-			_, err := CommentOutInvalidLines(&dst, src, func(i int64) {})
+			_, err := FilterInvalidLines(&dst, src, func(i int64) {})
 			require.NoError(t, err)
 
 			// Copy rest of contents