From a9e33f3e8d7af196ddb45b5945ac417ef75f88fa Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 8 Jul 2021 16:56:00 -0700 Subject: [PATCH 1/4] 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 --- pkg/sql/colexec/execgen/datadriven_test.go | 4 +++- pkg/sql/colexec/execgen/execgen.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/sql/colexec/execgen/datadriven_test.go b/pkg/sql/colexec/execgen/datadriven_test.go index 1c3764029a5f..3a3d12047caf 100644 --- 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 index 876b103d9a33..c744a0a7034b 100644 --- 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 } From 545a95412b1dddddda1433eadf19bd7a11342688 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 8 Jul 2021 17:01:52 -0700 Subject: [PATCH 2/4] 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 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 --- pkg/sql/colexec/execgen/template.go | 4 +++ pkg/sql/colexec/execgen/testdata/template | 35 +++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/pkg/sql/colexec/execgen/template.go b/pkg/sql/colexec/execgen/template.go index a62a949371ad..40528ed6d401 100644 --- 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/template b/pkg/sql/colexec/execgen/testdata/template index f671e6d72200..08033a36fe93 100644 --- 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 +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 +} +---- +---- From 91982ad3bced2707c9505ce1bee59cc88875505d Mon Sep 17 00:00:00 2001 From: Jane Xing Date: Thu, 8 Jul 2021 10:33:59 -0500 Subject: [PATCH 3/4] sql: support backslash_quote session setting This setting is based on an previous issue: #45774. This commit set a placeholder for PostgreSQL configuration setting called (backslash_quote)[https://www.postgresql.org/docs/13/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` Updates: - nil: updated url for backslash_quote, and removed extra spaces - Deleted `backslash_quote` from `unsupported_vars.go` - Added `./.idea` at `.gitignore` - 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. --- .../testdata/logic_test/information_schema | 1 + pkg/sql/logictest/testdata/logic_test/pg_catalog | 3 +++ pkg/sql/logictest/testdata/logic_test/set | 14 ++++++++++++++ pkg/sql/logictest/testdata/logic_test/show_source | 1 + pkg/sql/unsupported_vars.go | 1 - pkg/sql/vars.go | 6 ++++++ 6 files changed, 25 insertions(+), 1 deletion(-) diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema index 315b36b365d2..9177bd425460 100644 --- 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 index c59f61ce3fa4..9e7a7dcdcd69 100644 --- 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 index f9207f9ecc2a..22830479e730 100644 --- 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 index c9be403bc688..020873390419 100644 --- 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 index 489eabc30e25..2915aa415c79 100644 --- 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 index 5d3faeb3a1f3..2ee6bd373298 100644 --- 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`: { From 3ca6f2b02722cc3fe9c7f768ec8fc082e5edf686 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Tue, 6 Jul 2021 22:58:29 -0600 Subject: [PATCH 4/4] execgen: preserve comments around callsite of inlined function 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 --- pkg/sql/colexec/colexecjoin/hashjoiner.eg.go | 4 +++ pkg/sql/colexec/execgen/inline.go | 6 ++++ pkg/sql/colexec/execgen/testdata/inline | 31 ++++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go b/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go index bfe62b4a36b0..4481d46cbb8e 100644 --- a/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go +++ b/pkg/sql/colexec/colexecjoin/hashjoiner.eg.go @@ -299,6 +299,8 @@ func collectLeftAnti_true( //gcassert:bce currentID := HeadIDs[i] if currentID == 0 { + // currentID of 0 indicates that ith probing row didn't have a match, so + // we include it into the output. { var __retval_0 int { @@ -388,6 +390,8 @@ func collectLeftAnti_false( //gcassert:bce currentID := HeadIDs[i] if currentID == 0 { + // currentID of 0 indicates that ith probing row didn't have a match, so + // we include it into the output. { var __retval_0 int { diff --git a/pkg/sql/colexec/execgen/inline.go b/pkg/sql/colexec/execgen/inline.go index 478d068f6c76..2db786284b86 100644 --- 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/testdata/inline b/pkg/sql/colexec/execgen/testdata/inline index 998d9fe1a978..bcb0a7e3b8b0 100644 --- 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" +---- +----