Merge #67291 #67343 #67399

67291: execgen: preserve comments around callsite of inlined function r=DrewKimball a=DrewKimball This patch modifies the execgen `inline` directive to ensure that comments before and after the callsites of inlined functions are not removed. Example: ``` func a() { // This comment should not be removed. b() // This one too. } // execgen:inline func b() { foo = bar } => func a() { // This comment should not be removed. { foo = bar } // This one too. } // execgen:inline const _ = "inlined_b" ``` Release note: None 67343: sql: support backslash_quote session setting r=ZhouXing19 a=ZhouXing19 This setting is based on a previous issue: #45774. This commit set a placeholder for PostgreSQL configuration setting called [backslash_quote](https://www.postgresql.org/docs/12/runtime-config-compatible.html#GUC-BACKSLASH-QUOTE), and ensured it is compatible with ORMS. Currently, we only allow the value to be `backslash_quote`. If `client_encoding` is updated to allow encodings other than UTF8, then the default value of `backslash_quote` should be changed to `on`. When users try setting the backslash_quote session variable, CRDB will let it pass, but not making any changes. We leave the functionality implementation to future works. Details: - Added `backslash_quote` at varGen at `sql/vars.go` without implementing the true functionality - Tested it with `subtest backslash_quote_test` at `pkg/sql/logictest/testdata/logic_test/set` - Removed `backslash_quote` from `unsupported_vars.go` Passed local test: <img src="https://i.imgur.com/crZFk9w.png"> Updates: - nil: updated url for backslash_quote, and removed extra spaces - Deleted `backslash_quote` from `unsupported_vars.go` - Ran `make testlogic FILES=information_schema TESTFLAGS=-rewrite` Release note (sql change): Added session variable backslash_quote for compatibility, setting it does a no-op, and only `safe_encoding` is supported. 67399: execgen: fix template function names with argument comments r=mgartner a=mgartner #### execgen: do not silently err when producing output string Previously, execgen would silently fail if `decorator.Fprint` returned an error. This could happen if templating logic created an invalid DST. The result of the silent failure was an empty `.eg.go` file without any indication of what went wrong. Errors are now more explicit which should give us a starting place for tracking down templating bugs. Release note: None #### execgen: fix template function names with argument comments Previously, execgen would generate invalid DSTs for template function variants when call sites had comments describing an argument. For example: func a() { b(true /* t */) } // execgen:inline // execgen:template<t> func b(t bool) { // ... } This would produce a template function variant with `/* t */`, which was invalid because it contains spaces. Template argument decorations are now cleared to avoid this issue. Release note: None Co-authored-by: Drew Kimball <[email protected]> Co-authored-by: Jane Xing <[email protected]> Co-authored-by: Marcus Gartner <[email protected]>
cockroachdb · Jul 9, 2021 · 0a48b0b · 0a48b0b
4 parents c4abae9 + 3ca6f2b + 91982ad + 545a954
commit 0a48b0b
Show file tree

Hide file tree

Showing 13 changed files with 111 additions and 3 deletions.
diff --git a/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go b/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go
diff --git a/pkg/sql/colexec/execgen/datadriven_test.go b/pkg/sql/colexec/execgen/datadriven_test.go
@@ -36,7 +36,9 @@ func TestExecgen(t *testing.T) {
 				return ""
 			}
 			var sb strings.Builder
-			_ = decorator.Fprint(&sb, f)
+			if err := decorator.Fprint(&sb, f); err != nil {
+				return err.Error()
+			}
 			return sb.String()
 		})
 	})

diff --git a/pkg/sql/colexec/execgen/execgen.go b/pkg/sql/colexec/execgen/execgen.go
@@ -34,6 +34,8 @@ func Generate(inputFileContents string) (string, error) {
 
 	// Produce output string.
 	var sb strings.Builder
-	_ = decorator.Fprint(&sb, f)
+	if err := decorator.Fprint(&sb, f); err != nil {
+		panic(err)
+	}
 	return sb.String(), nil
 }
diff --git a/pkg/sql/colexec/execgen/inline.go b/pkg/sql/colexec/execgen/inline.go
@@ -122,6 +122,9 @@ func inlineFunc(inlineFuncMap map[string]funcInfo, n dst.Node) dst.Node {
 			// }
 			inlinedStatements := &dst.BlockStmt{
 				List: []dst.Stmt{retValDeclStmt},
+				Decs: dst.BlockStmtDecorations{
+					NodeDecs: n.Decs.NodeDecs,
+				},
 			}
 			body := dst.Clone(decl.Body).(*dst.BlockStmt)
 
@@ -179,6 +182,9 @@ func inlineFunc(inlineFuncMap map[string]funcInfo, n dst.Node) dst.Node {
 			// the mangled returns after the inlined function.
 			funcBlock := &dst.BlockStmt{
 				List: []dst.Stmt{reassignments},
+				Decs: dst.BlockStmtDecorations{
+					NodeDecs: n.Decs.NodeDecs,
+				},
 			}
 			body := dst.Clone(decl.Body).(*dst.BlockStmt)
 

diff --git a/pkg/sql/colexec/execgen/template.go b/pkg/sql/colexec/execgen/template.go
@@ -46,6 +46,10 @@ func replaceTemplateVars(
 	// Collect template arguments.
 	for i, param := range info.templateParams {
 		templateArgs[i] = dst.Clone(call.Args[param.fieldOrdinal]).(dst.Expr)
+		// Clear the decorations so that argument comments are not used in
+		// template function names.
+		templateArgs[i].Decorations().Start.Clear()
+		templateArgs[i].Decorations().End.Clear()
 	}
 	// Remove template vars from callsite.
 	newArgs := make([]dst.Expr, 0, len(call.Args)-len(info.templateParams))

diff --git a/pkg/sql/colexec/execgen/testdata/inline b/pkg/sql/colexec/execgen/testdata/inline
@@ -247,3 +247,34 @@ const _ = "inlined_b"
 const _ = "inlined_c"
 ----
 ----
+
+# Test that comments at call-site are preserved.
+inline
+package main
+
+func a() {
+	// This comment should not be removed.
+  b()
+  // This one too.
+}
+
+// execgen:inline
+func b() {
+  foo = bar
+}
+----
+----
+package main
+
+func a() {
+	// This comment should not be removed.
+	{
+		foo = bar
+	}
+	// This one too.
+}
+
+// execgen:inline
+const _ = "inlined_b"
+----
+----
diff --git a/pkg/sql/colexec/execgen/testdata/template b/pkg/sql/colexec/execgen/testdata/template
@@ -442,3 +442,38 @@ func b_false() int {
 }
 ----
 ----
+
+# Do not include comment decoration in template function names.
+template
+package main
+
+func a() {
+	b(true /* t */)
+}
+
+// execgen:inline
+// execgen:template<t>
+func b(t bool) int {
+	if t {
+	    return 0
+	} else {
+	    return 1
+	}
+}
+----
+----
+package main
+
+func a() {
+	b_true()
+}
+
+// execgen:inline
+const _ = "template_b"
+
+// execgen:inline
+func b_true() int {
+	return 0
+}
+----
+----
diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema
@@ -4067,6 +4067,7 @@ SELECT * FROM information_schema.session_variables where variable not in ('crdb_
 variable                                              value
 allow_prepare_as_opt_plan                             off
 application_name                                      ·
+backslash_quote                                       safe_encoding
 bytea_output                                          hex
 client_encoding                                       UTF8
 client_min_messages                                   notice

diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog
@@ -3394,6 +3394,7 @@ WHERE
 ----
 name                                                  setting             category  short_desc  extra_desc  vartype
 application_name                                      ·                   NULL      NULL        NULL        string
+backslash_quote                                       safe_encoding       NULL      NULL        NULL        string
 bytea_output                                          hex                 NULL      NULL        NULL        string
 client_encoding                                       UTF8                NULL      NULL        NULL        string
 client_min_messages                                   notice              NULL      NULL        NULL        string
@@ -3476,6 +3477,7 @@ WHERE
 ----
 name                                                  setting             unit  context  enumvals  boot_val            reset_val
 application_name                                      ·                   NULL  user     NULL      ·                   ·
+backslash_quote                                       safe_encoding       NULL  user     NULL      safe_encoding       safe_encoding
 bytea_output                                          hex                 NULL  user     NULL      hex                 hex
 client_encoding                                       UTF8                NULL  user     NULL      UTF8                UTF8
 client_min_messages                                   notice              NULL  user     NULL      notice              notice
@@ -3553,6 +3555,7 @@ SELECT name, source, min_val, max_val, sourcefile, sourceline FROM pg_catalog.pg
 ----
 name                                                  source  min_val  max_val  sourcefile  sourceline
 application_name                                      NULL    NULL     NULL     NULL        NULL
+backslash_quote                                       NULL    NULL     NULL     NULL        NULL
 bytea_output                                          NULL    NULL     NULL     NULL        NULL
 client_encoding                                       NULL    NULL     NULL     NULL        NULL
 client_min_messages                                   NULL    NULL     NULL     NULL        NULL

diff --git a/pkg/sql/logictest/testdata/logic_test/set b/pkg/sql/logictest/testdata/logic_test/set
@@ -476,3 +476,17 @@ SET standard_conforming_strings='true'
 
 statement ok
 SET standard_conforming_strings='on'
+
+subtest backslash_quote_test
+
+statement ok
+SET backslash_quote = 'safe_encoding';
+
+statement error invalid value for parameter "backslash_quote"
+SET backslash_quote = 'on';
+
+statement error invalid value for parameter "backslash_quote"
+SET backslash_quote = 'off';
+
+statement error invalid value for parameter "backslash_quote"
+SET backslash_quote = '123';
diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source
@@ -27,6 +27,7 @@ WHERE variable != 'optimizer' AND variable != 'crdb_version' AND variable != 'se
 ----
 variable                                              value
 application_name                                      ·
+backslash_quote                                       safe_encoding
 bytea_output                                          hex
 client_encoding                                       UTF8
 client_min_messages                                   notice

diff --git a/pkg/sql/unsupported_vars.go b/pkg/sql/unsupported_vars.go
@@ -64,7 +64,6 @@ var UnsupportedVars = func(ss ...string) map[string]struct{} {
 	// "application_name",
 	"array_nulls",
 	"backend_flush_after",
-	"backslash_quote",
 	// "bytea_output",
 	"check_function_bodies",
 	// "client_encoding",

diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go
@@ -831,6 +831,12 @@ var varGen = map[string]sessionVar{
 	// See https://www.postgresql.org/docs/10/static/runtime-config-client.html#GUC-LOC-TIMEOUT
 	`lock_timeout`: makeCompatIntVar(`lock_timeout`, 0),
 
+	// See https://www.postgresql.org/docs/13/runtime-config-compatible.html
+	// CockroachDB only supports safe_encoding for now. If `client_encoding` is updated to
+	// allow encodings other than UTF8, then the default value of `backslash_quote` should
+	// be changed to `on`.
+	`backslash_quote`: makeCompatStringVar(`backslash_quote`, `safe_encoding`),
+
 	// Supported for PG compatibility only.
 	// See https://www.postgresql.org/docs/10/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS
 	`max_identifier_length`: {