From cd3d484e3b066e6a82b8d9cf31be4afdadfc1aa5 Mon Sep 17 00:00:00 2001 From: Nick Vigilante Date: Wed, 31 Aug 2022 14:56:15 +0000 Subject: [PATCH] docs-issue-generation: Script can now handle multiple release notes Previously, if a commit to the cockroach repo contained multiple RNs, the docs issue generation script only evaluated the last release note. Additionally, if the release justification was included below the last release note in a commit, it would be included as part of the release note. Now, each release note in a commit is evaluated, and the release justification is stripped. Release note: None Release justification: non-production code change --- .../docs_issue_generation.go | 162 ++-- .../docs_issue_generation_test.go | 705 ++++++++++++++++-- 2 files changed, 747 insertions(+), 120 deletions(-) diff --git a/pkg/cmd/docs-issue-generation/docs_issue_generation.go b/pkg/cmd/docs-issue-generation/docs_issue_generation.go index 96f38b786462..7939143802ab 100644 --- a/pkg/cmd/docs-issue-generation/docs_issue_generation.go +++ b/pkg/cmd/docs-issue-generation/docs_issue_generation.go @@ -23,28 +23,29 @@ import ( "time" ) -// commit contains details about each formatted commit -type commit struct { - sha string - title string - releaseNote string +// docsIssue contains details about each formatted commit to be committed to the docs repo +type docsIssue struct { + sourceCommitSha string + title string + body string } -// pr contains details about the pull request +// pr contains details about the cockroach pull request type pr struct { number int mergeBranch string - commits []commit + docsIssues []docsIssue } -// ghSearch contains a list of search items (in this case, PRs) from the GitHub API -type ghSearch struct { // this struct holds the search results of all PRs based on a given commit +// ghSearch contains a list of search items (in this case, cockroach PRs) based on a given commit from the GitHub API. +type ghSearch struct { Items []ghSearchItem `json:"items"` } -// ghSearchItem contains details about a specific PR, including its number, from the GitHub API +// ghSearchItem contains details about a specific cockroach PR, +// including its number, from the GitHub API. type ghSearchItem struct { - Number int `json:"number"` // PR number + PRNumber int `json:"number"` PullRequest ghSearchItemPr `json:"pull_request"` } @@ -53,7 +54,9 @@ type ghSearchItemPr struct { MergedAt *time.Time `json:"merged_at"` } -// this struct holds details about the PR from the GitHub API +// ghPull holds the base branch for a cockroach PR from the GitHub API. +// It directly mirrors the structure of the PR where the branch name +// field (Ref) is nested within the Base field. type ghPull struct { Base struct { Ref string `json:"ref"` // this is the destination branch of the PR @@ -66,7 +69,7 @@ type ghPullCommit struct { Commit ghPullCommitMsg `json:"commit"` } -// ghPullCommitMsg holds the commit message from the GitHub API +// ghPullCommitMsg holds the commit message in the cockroach PR from the GitHub API type ghPullCommitMsg struct { Message string `json:"message"` // the commit message } @@ -81,21 +84,36 @@ type parameters struct { func docsIssueGeneration(params parameters) { var search ghSearch // a search for all PRs based on a given commit SHA var prs []pr - if err := httpGet("https://api.github.com/search/issues?q=sha:"+params.Sha+"+repo:cockroachdb/cockroach+is:merged", params.Token, &search); err != nil { + err := httpGet( + "https://api.github.com/search/issues?q=sha:"+params.Sha+"+repo:cockroachdb/cockroach+is:merged", + params.Token, + &search, + ) + if err != nil { log.Fatal(err) } prs = prNums(search) // populate slice of PRs of type pr for _, pr := range prs { // for each PR in the list, get the merge branch and the list of eligible commits var pull ghPull - if err := httpGet("https://api.github.com/repos/cockroachdb/cockroach/pulls/"+strconv.Itoa(pr.number), params.Token, &pull); err != nil { + err := httpGet( + "https://api.github.com/repos/cockroachdb/cockroach/pulls/"+strconv.Itoa(pr.number), + params.Token, + &pull, + ) + if err != nil { log.Fatal(err) } pr.mergeBranch = pull.Base.Ref var commits []ghPullCommit - if err := httpGet("https://api.github.com/repos/cockroachdb/cockroach/pulls/"+strconv.Itoa(pr.number)+"/commits?per_page:250", params.Token, &commits); err != nil { + err = httpGet( + "https://api.github.com/repos/cockroachdb/cockroach/pulls/"+strconv.Itoa(pr.number)+"/commits?per_page:250", + params.Token, + &commits, + ) + if err != nil { log.Fatal(err) } - pr.commits = getCommits(commits, pr.number) + pr.docsIssues = getIssues(commits, pr.number) pr.createDocsIssues(params.Token) } } @@ -115,7 +133,8 @@ func httpGet(url string, token string, out interface{}) error { if err != nil { return err } - if err := json.Unmarshal(bs, out); err != nil { // unmarshal (convert) the byte slice into an interface + // unmarshal (convert) the byte slice into an interface + if err := json.Unmarshal(bs, out); err != nil { return err } return nil @@ -124,61 +143,110 @@ func httpGet(url string, token string, out interface{}) error { // prNums returns an array of PRS for the given GH search of PRs against a given commit. func prNums(search ghSearch) []pr { var result []pr - for _, x := range search.Items { // each PR returned from the search is iterated through, and a new pr object is created for each + // each PR returned from the search is iterated through, and a new pr object is created for each + for _, x := range search.Items { if x.PullRequest.MergedAt != nil { - result = append(result, pr{number: x.Number}) // a new object of type pr containing the PR number is appended to the result slice + // a new object of type pr containing the PR number is appended to the result slice + result = append(result, pr{number: x.PRNumber}) } } return result // return a slice of prs } -// getCommits takes a list of commits from GitHub as well as the PR associated with those commits and outputs a formatted list of commits with valid release notes on that PR -func getCommits(pullCommit []ghPullCommit, prNumber int) []commit { - var result []commit +// getIssues takes a list of commits from GitHub as well as the PR number associated with those commits and outputs a +// formatted list of docs issues with valid release notes +func getIssues(pullCommit []ghPullCommit, prNumber int) []docsIssue { + var result []docsIssue for _, c := range pullCommit { message := c.Commit.Message - rn := formatReleaseNote(message, prNumber, c.Sha) // return a slice of data that locates every instance of the phrase "release note (", case insensitive - if rn != "" { // checks to make sure a match was returned - x := commit{ // declare a new commit object - sha: c.Sha, - title: formatTitle(message), - releaseNote: rn, + // return a slice of data that locates every instance of the phrase "release note (", case-insensitive + rns := formatReleaseNotes(message, prNumber, c.Sha) + for i, rn := range rns { + { // checks to make sure a match was returned + x := docsIssue{ // declare a new commit object + sourceCommitSha: c.Sha, + title: formatTitle(message, prNumber, i+1, len(rns)), + body: rn, + } + result = append(result, x) } - result = append(result, x) } } return result } -func formatReleaseNote(message string, prNumber int, sha string) string { - re := regexp.MustCompile(`(?s)[rR]elease [nN]ote \(.*`) // this regex checks to make sure there's a release note within the commit - reNeg := regexp.MustCompile(`([rR]elease [nN]ote \(bug fix.*)|([rR]elease [nN]ote: [nN]one)`) // this regex is used to exclude bug fixes or releases without release notes - rn := re.FindString(message) // return the first instance of the phrase "release note (", case insensitive - if len(rn) > 0 && !reNeg.MatchString(message) { // checks to make sure the desired release note is not null and doesn't match the negating string. - return fmt.Sprintf( - "Related PR: https://github.com/cockroachdb/cockroach/pull/%s\nCommit: https://github.com/cockroachdb/cockroach/commit/%s\n\n---\n\n%s", +var ( + noRNRE = regexp.MustCompile(`[rR]elease [nN]ote: [nN]one`) + allRNRE = regexp.MustCompile(`[rR]elease [nN]ote \(.*`) + bugFixRNRE = regexp.MustCompile(`([rR]elease [nN]ote \(bug fix\):.*)`) + releaseJustificationRE = regexp.MustCompile(`[rR]elease [jJ]ustification:.*`) +) + +// formatReleaseNotes generates a list of docsIssue bodies for the docs repo based on a given CRDB sha +func formatReleaseNotes(message string, prNumber int, crdbSha string) []string { + rnBodySlice := []string{} + if noRNRE.MatchString(message) { + return rnBodySlice + } + splitString := strings.Split(message, "\n") + releaseNoteLines := []string{} + var rnBody string + for _, x := range splitString { + validRn := allRNRE.MatchString(x) + bugFixRn := bugFixRNRE.MatchString(x) + releaseJustification := releaseJustificationRE.MatchString(x) + if len(releaseNoteLines) > 0 && (validRn || releaseJustification) { + rnBody = fmt.Sprintf( + "Related PR: https://github.com/cockroachdb/cockroach/pull/%s\n"+ + "Commit: https://github.com/cockroachdb/cockroach/commit/%s\n"+ + "\n---\n\n%s", + strconv.Itoa(prNumber), + crdbSha, + strings.Join(releaseNoteLines, "\n"), + ) + rnBodySlice = append(rnBodySlice, strings.TrimSuffix(rnBody, "\n")) + rnBody = "" + releaseNoteLines = []string{} + } + if (validRn && !bugFixRn) || (len(releaseNoteLines) > 0 && !bugFixRn && !releaseJustification) { + releaseNoteLines = append(releaseNoteLines, x) + } + } + if len(releaseNoteLines) > 0 { // commit whatever is left in the buffer to the rnBodySlice set + rnBody = fmt.Sprintf( + "Related PR: https://github.com/cockroachdb/cockroach/pull/%s\n"+ + "Commit: https://github.com/cockroachdb/cockroach/commit/%s\n"+ + "\n---\n\n%s", strconv.Itoa(prNumber), - sha, - rn, + crdbSha, + strings.Join(releaseNoteLines, "\n"), ) + rnBodySlice = append(rnBodySlice, strings.TrimSuffix(rnBody, "\n")) } - return "" + return rnBodySlice } -func formatTitle(message string) string { - if i := strings.IndexRune(message, '\n'); i > 0 { - return message[:i] +func formatTitle(message string, prNumber int, index int, totalLength int) string { + var commitTitle string + if i := strings.IndexRune(message, '\n'); i > 0 { // + commitTitle = message[:i] + } else { + commitTitle = message + } + result := fmt.Sprintf("PR #%d - %s", prNumber, commitTitle) + if totalLength > 1 { + result += fmt.Sprintf(" (%d of %d)", index, totalLength) } - return message + return result } func (p pr) createDocsIssues(token string) { postURL := "https://api.github.com/repos/cockroachdb/docs/issues" - for _, commit := range p.commits { + for _, issue := range p.docsIssues { reqBody, err := json.Marshal(map[string]interface{}{ - "title": commit.title, + "title": issue.title, "labels": []string{"C-product-change", p.mergeBranch}, - "body": commit.releaseNote, + "body": issue.body, }) if err != nil { log.Fatal(err) diff --git a/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go b/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go index 37c73c943f22..7a966ad33cd2 100644 --- a/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go +++ b/pkg/cmd/docs-issue-generation/docs_issue_generation_test.go @@ -38,7 +38,7 @@ func TestPrNums(t *testing.T) { search: ghSearch{ Items: []ghSearchItem{ { - Number: 80158, + PRNumber: 80158, PullRequest: ghSearchItemPr{ MergedAt: timeFromGhTime("2022-04-19T17:48:55Z"), }, @@ -56,7 +56,7 @@ func TestPrNums(t *testing.T) { search: ghSearch{ Items: []ghSearchItem{ { - Number: 80157, + PRNumber: 80157, PullRequest: ghSearchItemPr{ MergedAt: nil, }, @@ -70,19 +70,19 @@ func TestPrNums(t *testing.T) { search: ghSearch{ Items: []ghSearchItem{ { - Number: 66328, + PRNumber: 66328, PullRequest: ghSearchItemPr{ MergedAt: timeFromGhTime("2021-12-01T06:00:00Z"), }, }, { - Number: 74525, + PRNumber: 74525, PullRequest: ghSearchItemPr{ MergedAt: nil, }, }, { - Number: 75077, + PRNumber: 75077, PullRequest: ghSearchItemPr{ MergedAt: nil, }, @@ -104,12 +104,12 @@ func TestPrNums(t *testing.T) { } } -func TestGetCommits(t *testing.T) { +func TestGetIssues(t *testing.T) { testCases := []struct { testName string pullCommit []ghPullCommit prNumber int - commits []commit + issues []docsIssue }{ { testName: "66328", @@ -117,63 +117,340 @@ func TestGetCommits(t *testing.T) { { Sha: "4dd8da9609adb3acce6795cea93b67ccacfc0270", Commit: ghPullCommitMsg{ - Message: "util/log: move the crdb-v1 test code to a separate file\n\nRelease note: None", + Message: `util/log: move the crdb-v1 test code to a separate file + +Release note: None`, }, }, { Sha: "cae4e525511a7d87f3f5ce74f02a6cf6edab4f3d", Commit: ghPullCommitMsg{ - Message: "util/log: use a datadriven test for the crdb-v1 format/parse test\n\nThis simplifies the test and makes it more extensible.\n\nRelease note: None", + Message: `util/log: use a datadriven test for the crdb-v1 format/parse test + +This simplifies the test and makes it more extensible. + +Release note: None`, }, }, { Sha: "3be8458eda90aa922fa2a22ad7d6531c57aa12e1", Commit: ghPullCommitMsg{ - Message: "util/log: make `logpb.Entry` slightly more able to represent crdb-v2 entries\n\nPrior to this change, we did not have a public data structure able to\nrepresent the various fields available in a `crdb-v2` log entry: the\nexisting `logpb.Entry` was not able to distinguish structured and\nnon-structured entries, and did not have the ability to delimit the\nmessage and the stack trace.\n\nThis patch extends `logpb.Entry` to make it more able to represent\n`crdb-v2` entries, at least for the purpose of extending `debug\nmerge-log` towards conversion between logging formats.\n\nAdditionally, the patch adds a *best effort* attempt at populating\nthe new fields in the `crdb-v1` entry parser. This is best effort\nbecause the crdb-v1 parser is lossy so we cannot faithfully\nparse its entries reliably.\n\nRelease note: None", + Message: "util/log: make " + "`logpb.Entry`" + ` slightly more able to represent crdb-v2 entries + +Prior to this change, we did not have a public data structure able to +represent the various fields available in a ` + "`crdb-v2`" + ` log entry: the +existing ` + "`logpb.Entry`" + ` was not able to distinguish structured and +non-structured entries, and did not have the ability to delimit the +message and the stack trace. + +This patch extends ` + "`logpb.Entry`" + ` to make it more able to represent +` + "`crdb-v2`" + " entries, at least for the purpose of extending " + "`debug\nmerge-log`" + + ` towards conversion between logging formats. + +Additionally, the patch adds a *best effort* attempt at populating +the new fields in the ` + "`crdb-v1`" + ` entry parser. This is best effort +because the crdb-v1 parser is lossy so we cannot faithfully +parse its entries reliably. + +Release note: None`, }, }, { Sha: "0f329965acccb3e771ec1657c7def9e881dc78bb", Commit: ghPullCommitMsg{ - Message: "util/log: report the logging format at the start of new files\n\nRelease note (cli change): When log entries are written to disk,\nthe first few header lines written at the start of every new file\nnow report the configured logging format.", + Message: `util/log: report the logging format at the start of new files + +Release note (cli change): When log entries are written to disk, +the first few header lines written at the start of every new file +now report the configured logging format.`, }, }, { Sha: "934c7da83035fb78108daa23fa9cb8925d7b6d10", Commit: ghPullCommitMsg{ - Message: "logconfig: fix the handling of crdb-v1 explicit config\n\nRelease note (bug fix): Previously, `--log='file-defaults: {format:\ncrdb-v1}'` was not handled properly. This has been fixed. This bug\nexisted since v21.1.0.", + Message: `logconfig: fix the handling of crdb-v1 explicit config + +Release note (bug fix): Previously, ` + "`--log='file-defaults: {format:\ncrdb-v1}'`" + + ` was not handled properly. This has been fixed. This bug +existed since v21.1.0.`, }, }, { Sha: "fb249c7140b634a53dca2967c946bc78ba927e1a", Commit: ghPullCommitMsg{ - Message: "cli: report explicit log config in logs\n\nThis increases troubleshootability.\n\nRelease note: None", + Message: `cli: report explicit log config in logs + +This increases troubleshootability. + +Release note: None`, }, }, { Sha: "e22a0ebb46806a0054115edbeef0d6205203eef5", Commit: ghPullCommitMsg{ - Message: "util/log,logspy: make the logspy mechanism better\n\nPrior to this patch, the logspy mechanism was utterly broken: any time\nit was running, it would cut off any and all log entries from going to\nfiles, stderr, network sinks etc. This was a gross violation\nof the guarantees we have constructed around structured logging,\nas well as a security vulnerability (see release note below).\n\nAdditionally, it was impossible to launch multiple logspy sessions\nsimultaneously on the same node, for example using different grep\nfilters.\n\nThis commit rectifies the situation.\n\nAt a technical level, we have two changes: one in the logging Go API\nand one in the `/debug/logspy` HTTP API.\n\n**For the logging changes**, this patch replaces the \"interceptor\" singleton\ncallback function that takes over `(*loggerT).outputLogEntry()`, by a\nnew *interceptor sink* that lives alongside the other sinks on every\nchannel.\n\nBecause the interceptor logic is now a regular sink, log entries\ncontinue to be delivered to other sinks while an interceptor is\nactive.\n\nReusing the sink abstraction, with its own sink configuration with no\nseverity filter, clarifies that the interceptor accepts all the log\nentries regardless of which filters are configured on other sinks.\n\nAdditionally, the interceptor sink now supports multiple concurrent\ninterception sessions. Each log entry is delivered to all current\ninterceptors. The `logspy` logic is adapted to use this facility,\nso that multiple `logspy` requests can run side-by-side.\n\n**For the HTTP API change**, we are changing the `/debug/logspy`\nsemantics. This is explained in the release note below.\n\nRelease note (security update): All the logging output to files\nor network sinks was previously disabled temporarily while an operator\nwas using the `/debug/logspy` HTTP API, resulting in lost entries\nand a breach of auditability guarantees. This behavior has been corrected.\n\nRelease note (bug fix): Log entries are not lost any more while the\n`/debug/logspy` HTTP API is being used. This bug had existed since\nCockroachDB v1.1.\n\nRelease note (api change): The `/debug/logspy` HTTP API has changed.\nThe endpoint now returns JSON data by default.\nThis change is motivated as follows:\n\n- the previous format, `crdb-v1`, cannot be parsed reliably.\n- using JSON entries guarantees that the text of each entry\n all fits on a single line of output (newline characters\n inside the messages are escaped). This makes filtering\n easier and more reliable.\n- using JSON enables the user to apply `jq` on the output, for\n example via `curl -s .../debug/logspy | jq ...`\n\nIf the previous format is desired, the user can pass the query\nargument `&flatten=1` to the `logspy` URL to obtain the previous flat\ntext format (`crdb-v1`) instead.\n\nCo-authored-by: Yevgeniy Miretskiy ", + Message: `util/log,logspy: make the logspy mechanism better + +Prior to this patch, the logspy mechanism was utterly broken: any time +it was running, it would cut off any and all log entries from going to +files, stderr, network sinks etc. This was a gross violation +of the guarantees we have constructed around structured logging, +as well as a security vulnerability (see release note below). + +Additionally, it was impossible to launch multiple logspy sessions +simultaneously on the same node, for example using different grep +filters. + +This commit rectifies the situation. + +At a technical level, we have two changes: one in the logging Go API +and one in the ` + "`/debug/logspy`" + ` HTTP API. + +**For the logging changes**, this patch replaces the "interceptor" singleton +callback function that takes over ` + "`(*loggerT).outputLogEntry()`" + `, by a +new *interceptor sink* that lives alongside the other sinks on every +channel. + +Because the interceptor logic is now a regular sink, log entries +continue to be delivered to other sinks while an interceptor is +active. + +Reusing the sink abstraction, with its own sink configuration with no +severity filter, clarifies that the interceptor accepts all the log +entries regardless of which filters are configured on other sinks. + +Additionally, the interceptor sink now supports multiple concurrent +interception sessions. Each log entry is delivered to all current +interceptors. The ` + "`logspy`" + ` logic is adapted to use this facility, +so that multiple ` + "`logspy`" + ` requests can run side-by-side. + +**For the HTTP API change**, we are changing the ` + "`/debug/logspy`" + ` +semantics. This is explained in the release note below. + +Release note (security update): All the logging output to files +or network sinks was previously disabled temporarily while an operator +was using the ` + "`/debug/logspy`" + ` HTTP API, resulting in lost entries +and a breach of auditability guarantees. This behavior has been corrected. + +Release note (bug fix): Log entries are not lost any more while the +` + "`/debug/logspy`" + ` HTTP API is being used. This bug had existed since +CockroachDB v1.1. + +Release note (api change): The ` + "`/debug/logspy`" + ` HTTP API has changed. +The endpoint now returns JSON data by default. +This change is motivated as follows: + +- the previous format, ` + "`crdb-v1`" + `, cannot be parsed reliably. +- using JSON entries guarantees that the text of each entry + all fits on a single line of output (newline characters + inside the messages are escaped). This makes filtering + easier and more reliable. +- using JSON enables the user to apply ` + "`jq`" + ` on the output, for + example via ` + "`curl -s .../debug/logspy | jq ...`" + ` + +If the previous format is desired, the user can pass the query +argument ` + "`&flatten=1`" + " to the " + "`logspy`" + ` URL to obtain the previous flat +text format (` + "`crdb-v1`" + `) instead. + +Co-authored-by: Yevgeniy Miretskiy `, }, }, { Sha: "44836265f924a14f8c996a714d954e0e7e35dff7", Commit: ghPullCommitMsg{ - Message: "util/log,server/debug: new API `/debug/vmodule`, change `logspy`\n\nPrior to this patch, any ongoing `/debug/logspy` query would\ntrigger maximum verbosity in the logging package - i.e.\ncause all logging API calls under `log.V` to be activated.\n\nThis was problematic, as it would cause a large amount\nof logging traffic to be pumped through the interceptor logic,\nincreasing the chance for entries to be dropped (especially\nwhen `logspy?grep=...` is not used).\n\nAdditionally, since the previous change to make the interceptor logic\na regular sink, all the entries captured by the interceptors are now\nalso duplicated to the other sinks (this is a feature / bug fix,\nas explained in the previous commit message).\n\nHowever, this change also meant that any ongoing `logspy` request\nwould cause maximum verbosity to all the sinks with threshold\nINFO (most logging calls under `log.V` have severity INFO). For\nexample, the DEV channel accepts all entries at level INFO or higher\nin the default config. This in turn could incur unacceptable disk\nspace or IOPS consumption in certain deployments.\n\nIn orde to mitigate this new problem, this patch removes the special\nconditional from the logging package. From this point forward,\nthe verbosity of the entries delivered via `/debug/logspy` are those\nconfigured via `vmodule` -- no more and no less.\n\nTo make this configurable for a running server, including one where\nthe SQL endpoint may not be available yet, this patch also introduces\na new `/debug/vmodule` HTTP API and extends `/debug/logspy` with\nthe ability to temporarily change `vmodule` *upon explicit request*.\n\nRelease note (api change): The `/debug/logspy` API does not any more\nenable maximum logging verbosity automatically. To change the\nverbosity, use the new `/debug/vmodule` endpoint or pass the\n`&vmodule=` query parameter to the `/debug/logspy` endpoint.\n\nFor example, suppose you wish to run a 20s logspy session:\n\n- Before:\n\n ```\n curl 'https://.../debug/logspy?duration=20s&...'\n ```\n\n- Now:\n\n ```\n curl 'https://.../debug/logspy?duration=20s&vmodule=...'\n ```\n\n OR\n\n ```\n curl 'https://.../debug/vmodule?duration=22s&vmodule=...'\n curl 'https://.../debug/logspy?duration=20s'\n ```\n\nAs for the regular `vmodule` command-line flag, the maximum verbosity\nacross all the source code can be selected with the pattern `*=4`.\n\nNote: at most one in-flight HTTP API request is allowed to modify the\n`vmodule` parameter. This maintain the invariant that the\nconfiguration restored at the end of each request is the same as when\nthe request started.\n\nRelease note (api change): The new `/debug/vmodule` API makes it\npossible for an operator to configure the logging verbosity in a\nsimilar way as the SQL built-in function\n`crdb_internal.set_vmodule()`, or to query the current configuration\nas in `crdb_internal.get_vmodule()`. Additionally, any configuration\nchange performed via this API can be automatically reverted after a\nconfigurable delay. The API forms are:\n\n- `/debug/vmodule` - retrieve the current configuration.\n- `/debug/vmodule?set=[vmodule config]&duration=[duration]` - change\n the configuration to `[vmodule config]` . The previous configuration\n at the time the `/debug/vmodule` request started is restored after\n `[duration]`. This duration, if not specified, defaults to twice the\n default duration of a `logspy` request (currently, the `logspy`\n default duration is 5s, so the `vmodule` default duration is 10s).\n If the duration is zero or negative, the previous configuration\n is never restored.", + Message: "util/log,server/debug: new API " + "`/debug/vmodule`" + ", change " + "`logspy`" + ` + +Prior to this patch, any ongoing ` + "`/debug/logspy`" + ` query would +trigger maximum verbosity in the logging package - i.e. +cause all logging API calls under ` + "`log.V`" + ` to be activated. + +This was problematic, as it would cause a large amount +of logging traffic to be pumped through the interceptor logic, +increasing the chance for entries to be dropped (especially +when ` + "`logspy?grep=...`" + ` is not used). + +Additionally, since the previous change to make the interceptor logic +a regular sink, all the entries captured by the interceptors are now +also duplicated to the other sinks (this is a feature / bug fix, +as explained in the previous commit message). + +However, this change also meant that any ongoing ` + "`logspy`" + ` request +would cause maximum verbosity to all the sinks with threshold +INFO (most logging calls under ` + "`log.V`" + ` have severity INFO). For +example, the DEV channel accepts all entries at level INFO or higher +in the default config. This in turn could incur unacceptable disk +space or IOPS consumption in certain deployments. + +In orde to mitigate this new problem, this patch removes the special +conditional from the logging package. From this point forward, +the verbosity of the entries delivered via ` + "`/debug/logspy`" + ` are those +configured via ` + "`vmodule`" + ` -- no more and no less. + +To make this configurable for a running server, including one where +the SQL endpoint may not be available yet, this patch also introduces +a new ` + "`/debug/vmodule`" + " HTTP API and extends " + "`/debug/logspy`" + ` with +the ability to temporarily change ` + "`vmodule`" + ` *upon explicit request*. + +Release note (api change): The ` + "`/debug/logspy`" + ` API does not any more +enable maximum logging verbosity automatically. To change the +verbosity, use the new ` + "`/debug/vmodule`" + ` endpoint or pass the +` + "`&vmodule=`" + " query parameter to the " + "`/debug/logspy`" + ` endpoint. + +For example, suppose you wish to run a 20s logspy session: + +- Before: + + ` + "```\n curl 'https://.../debug/logspy?duration=20s&...'\n ```" + ` + +- Now: + + ` + "```\n curl 'https://.../debug/logspy?duration=20s&vmodule=...'\n ```" + ` + + OR + + ` + + "```\n curl 'https://.../debug/vmodule?duration=22s&vmodule=...'\n curl 'https://.../debug/logspy?duration=20s'\n ```" + ` + +As for the regular ` + "`vmodule`" + ` command-line flag, the maximum verbosity +across all the source code can be selected with the pattern ` + "`*=4`" + `. + +Note: at most one in-flight HTTP API request is allowed to modify the +` + "`vmodule`" + ` parameter. This maintain the invariant that the +configuration restored at the end of each request is the same as when +the request started. + +Release note (api change): The new ` + "`/debug/vmodule`" + ` API makes it +possible for an operator to configure the logging verbosity in a +similar way as the SQL built-in function +` + "`crdb_internal.set_vmodule()`" + `, or to query the current configuration +as in ` + "`crdb_internal.get_vmodule()`" + `. Additionally, any configuration +change performed via this API can be automatically reverted after a +configurable delay. The API forms are: + +- ` + "`/debug/vmodule`" + ` - retrieve the current configuration. +- ` + "`/debug/vmodule?set=[vmodule config]&duration=[duration]`" + ` - change + the configuration to ` + "`[vmodule config]`" + ` . The previous configuration + at the time the ` + "`/debug/vmodule`" + ` request started is restored after + ` + "`[duration]`" + `. This duration, if not specified, defaults to twice the + default duration of a ` + "`logspy`" + " request (currently, the " + "`logspy`" + ` + default duration is 5s, so the ` + "`vmodule`" + ` default duration is 10s). + If the duration is zero or negative, the previous configuration + is never restored.`, }, }, }, prNumber: 66328, - commits: []commit{ + issues: []docsIssue{ { - sha: "0f329965acccb3e771ec1657c7def9e881dc78bb", - title: "util/log: report the logging format at the start of new files", - releaseNote: "Related PR: https://github.com/cockroachdb/cockroach/pull/66328\nCommit: https://github.com/cockroachdb/cockroach/commit/0f329965acccb3e771ec1657c7def9e881dc78bb\n\n---\n\nRelease note (cli change): When log entries are written to disk,\nthe first few header lines written at the start of every new file\nnow report the configured logging format.", + sourceCommitSha: "0f329965acccb3e771ec1657c7def9e881dc78bb", + title: "PR #66328 - util/log: report the logging format at the start of new files", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/0f329965acccb3e771ec1657c7def9e881dc78bb + +--- + +Release note (cli change): When log entries are written to disk, +the first few header lines written at the start of every new file +now report the configured logging format.`, }, { - sha: "44836265f924a14f8c996a714d954e0e7e35dff7", - title: "util/log,server/debug: new API `/debug/vmodule`, change `logspy`", - releaseNote: "Related PR: https://github.com/cockroachdb/cockroach/pull/66328\nCommit: https://github.com/cockroachdb/cockroach/commit/44836265f924a14f8c996a714d954e0e7e35dff7\n\n---\n\nRelease note (api change): The `/debug/logspy` API does not any more\nenable maximum logging verbosity automatically. To change the\nverbosity, use the new `/debug/vmodule` endpoint or pass the\n`&vmodule=` query parameter to the `/debug/logspy` endpoint.\n\nFor example, suppose you wish to run a 20s logspy session:\n\n- Before:\n\n ```\n curl 'https://.../debug/logspy?duration=20s&...'\n ```\n\n- Now:\n\n ```\n curl 'https://.../debug/logspy?duration=20s&vmodule=...'\n ```\n\n OR\n\n ```\n curl 'https://.../debug/vmodule?duration=22s&vmodule=...'\n curl 'https://.../debug/logspy?duration=20s'\n ```\n\nAs for the regular `vmodule` command-line flag, the maximum verbosity\nacross all the source code can be selected with the pattern `*=4`.\n\nNote: at most one in-flight HTTP API request is allowed to modify the\n`vmodule` parameter. This maintain the invariant that the\nconfiguration restored at the end of each request is the same as when\nthe request started.\n\nRelease note (api change): The new `/debug/vmodule` API makes it\npossible for an operator to configure the logging verbosity in a\nsimilar way as the SQL built-in function\n`crdb_internal.set_vmodule()`, or to query the current configuration\nas in `crdb_internal.get_vmodule()`. Additionally, any configuration\nchange performed via this API can be automatically reverted after a\nconfigurable delay. The API forms are:\n\n- `/debug/vmodule` - retrieve the current configuration.\n- `/debug/vmodule?set=[vmodule config]&duration=[duration]` - change\n the configuration to `[vmodule config]` . The previous configuration\n at the time the `/debug/vmodule` request started is restored after\n `[duration]`. This duration, if not specified, defaults to twice the\n default duration of a `logspy` request (currently, the `logspy`\n default duration is 5s, so the `vmodule` default duration is 10s).\n If the duration is zero or negative, the previous configuration\n is never restored.", + sourceCommitSha: "e22a0ebb46806a0054115edbeef0d6205203eef5", + title: "PR #66328 - util/log,logspy: make the logspy mechanism better (1 of 2)", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/e22a0ebb46806a0054115edbeef0d6205203eef5 + +--- + +Release note (security update): All the logging output to files +or network sinks was previously disabled temporarily while an operator +was using the ` + "`/debug/logspy`" + ` HTTP API, resulting in lost entries +and a breach of auditability guarantees. This behavior has been corrected.`, + }, + { + sourceCommitSha: "e22a0ebb46806a0054115edbeef0d6205203eef5", + title: "PR #66328 - util/log,logspy: make the logspy mechanism better (2 of 2)", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/e22a0ebb46806a0054115edbeef0d6205203eef5 + +--- + +Release note (api change): The ` + "`/debug/logspy`" + ` HTTP API has changed. +The endpoint now returns JSON data by default. +This change is motivated as follows: + +- the previous format, ` + "`crdb-v1`" + `, cannot be parsed reliably. +- using JSON entries guarantees that the text of each entry + all fits on a single line of output (newline characters + inside the messages are escaped). This makes filtering + easier and more reliable. +- using JSON enables the user to apply ` + "`jq`" + ` on the output, for + example via ` + "`curl -s .../debug/logspy | jq ...`" + ` + +If the previous format is desired, the user can pass the query +argument ` + "`&flatten=1`" + " to the " + "`logspy`" + ` URL to obtain the previous flat +text format (` + "`crdb-v1`" + `) instead. + +Co-authored-by: Yevgeniy Miretskiy `, + }, + { + sourceCommitSha: "44836265f924a14f8c996a714d954e0e7e35dff7", + title: "PR #66328 - util/log,server/debug: new API `/debug/vmodule`, change `logspy` (1 of 2)", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/44836265f924a14f8c996a714d954e0e7e35dff7 + +--- + +Release note (api change): The ` + "`/debug/logspy`" + ` API does not any more +enable maximum logging verbosity automatically. To change the +verbosity, use the new ` + "`/debug/vmodule`" + ` endpoint or pass the +` + "`&vmodule=`" + " query parameter to the " + "`/debug/logspy`" + ` endpoint. + +For example, suppose you wish to run a 20s logspy session: + +- Before: + + ` + "```\n curl 'https://.../debug/logspy?duration=20s&...'\n ```" + ` + +- Now: + + ` + "```\n curl 'https://.../debug/logspy?duration=20s&vmodule=...'\n ```" + ` + + OR + + ` + "```\n curl 'https://.../debug/vmodule?duration=22s&vmodule=...'\n curl 'https://.../debug/logspy?duration=20s'\n ```" + ` + +As for the regular ` + "`vmodule`" + ` command-line flag, the maximum verbosity +across all the source code can be selected with the pattern ` + "`*=4`" + `. + +Note: at most one in-flight HTTP API request is allowed to modify the +` + "`vmodule`" + ` parameter. This maintain the invariant that the +configuration restored at the end of each request is the same as when +the request started.`, + }, + { + sourceCommitSha: "44836265f924a14f8c996a714d954e0e7e35dff7", + title: "PR #66328 - util/log,server/debug: new API `/debug/vmodule`, change `logspy` (2 of 2)", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/44836265f924a14f8c996a714d954e0e7e35dff7 + +--- + +Release note (api change): The new ` + "`/debug/vmodule`" + ` API makes it +possible for an operator to configure the logging verbosity in a +similar way as the SQL built-in function +` + "`crdb_internal.set_vmodule()`" + `, or to query the current configuration +as in ` + "`crdb_internal.get_vmodule()`" + `. Additionally, any configuration +change performed via this API can be automatically reverted after a +configurable delay. The API forms are: + +- ` + "`/debug/vmodule`" + ` - retrieve the current configuration. +- ` + "`/debug/vmodule?set=[vmodule config]&duration=[duration]`" + ` - change + the configuration to ` + "`[vmodule config]`" + ` . The previous configuration + at the time the ` + "`/debug/vmodule`" + ` request started is restored after + ` + "`[duration]`" + `. This duration, if not specified, defaults to twice the + default duration of a ` + "`logspy`" + " request (currently, the " + "`logspy`" + ` + default duration is 5s, so the ` + "`vmodule`" + ` default duration is 10s). + If the duration is zero or negative, the previous configuration + is never restored.`, }, }, }, @@ -183,12 +460,36 @@ func TestGetCommits(t *testing.T) { { Sha: "1d7811d5d14f9c7e106c3ec92de9c66192f19604", Commit: ghPullCommitMsg{ - Message: "opt: do not cross-join input of semi-join\n\nThis commit fixes a logical correctness bug caused when\n`GenerateLookupJoins` cross-joins the input of a semi-join with a set of\nconstant values to constrain the prefix columns of the lookup index. The\ncross-join is an invalid transformation because it increases the size of\nthe join's input and can increase the size of the join's output.\n\nWe already avoid these cross-joins for left and anti-joins (see #59646).\nWhen addressing those cases, the semi-join case was incorrectly assumed\nto be safe.\n\nFixes #78681\n\nRelease note (bug fix): A bug has been fixed which caused the optimizer\nto generate invalid query plans which could result in incorrect query\nresults. The bug, which has been present since version 21.1.0, can\nappear if all of the following conditions are true: 1) the query\ncontains a semi-join, such as queries in the form:\n`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`,\n2) the inner table has an index containing the equality column, like\n`t2.a` in the example query, 3) the index contains one or more\ncolumns that prefix the equality column, and 4) the prefix columns are\n`NOT NULL` and are constrained to a set of constant values via a `CHECK`\nconstraint or an `IN` condition in the filter.", + Message: `opt: do not cross-join input of semi-join + +This commit fixes a logical correctness bug caused when +` + "`GenerateLookupJoins`" + ` cross-joins the input of a semi-join with a set of +constant values to constrain the prefix columns of the lookup index. The +cross-join is an invalid transformation because it increases the size of +the join's input and can increase the size of the join's output. + +We already avoid these cross-joins for left and anti-joins (see #59646). +When addressing those cases, the semi-join case was incorrectly assumed +to be safe. + +Fixes #78681 + +Release note (bug fix): A bug has been fixed which caused the optimizer +to generate invalid query plans which could result in incorrect query +results. The bug, which has been present since version 21.1.0, can +appear if all of the following conditions are true: 1) the query +contains a semi-join, such as queries in the form: +` + "`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`" + `, +2) the inner table has an index containing the equality column, like +` + "`t2.a`" + ` in the example query, 3) the index contains one or more +columns that prefix the equality column, and 4) the prefix columns are +` + "`NOT NULL`" + " and are constrained to a set of constant values via a " + "`CHECK`" + ` +constraint or an ` + "`IN`" + " condition in the filter.", }, }, }, prNumber: 78685, - commits: nil, + issues: nil, }, { testName: "79069", @@ -196,16 +497,40 @@ func TestGetCommits(t *testing.T) { { Sha: "5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9", Commit: ghPullCommitMsg{ - Message: "sql: ignore non-existent columns when injecting stats\n\nPreviously, an `ALTER TABLE ... INJECT STATS` command would return an\nerror if the given stats JSON included any columns that were not present\nin the table descriptor. Statistics in statement bundles often include\ndropped columns, so reproducing issues with a bundle required tediously\nremoving stats for these columns. This commit changes the stats\ninjection behavior so that a notice is issued for stats with\nnon-existent columns rather than an error. Any stats for existing\ncolumns will be injected successfully.\n\nInforms #68184\n\nRelease note (sql change): `ALTER TABLE ... INJECT STATISTICS ...` will\nnow issue notices when the given statistics JSON includes non-existent\ncolumns, rather than resulting in an error. Any statistics in the JSON\nfor existing columns will be injected successfully.", + Message: `sql: ignore non-existent columns when injecting stats + +Previously, an ` + "`ALTER TABLE ... INJECT STATS`" + ` command would return an +error if the given stats JSON included any columns that were not present +in the table descriptor. Statistics in statement bundles often include +dropped columns, so reproducing docsIssues with a bundle required tediously +removing stats for these columns. This commit changes the stats +injection behavior so that a notice is issued for stats with +non-existent columns rather than an error. Any stats for existing +columns will be injected successfully. + +Informs #68184 + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`, }, }, }, prNumber: 79069, - commits: []commit{ + issues: []docsIssue{ { - sha: "5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9", - title: "sql: ignore non-existent columns when injecting stats", - releaseNote: "Related PR: https://github.com/cockroachdb/cockroach/pull/79069\nCommit: https://github.com/cockroachdb/cockroach/commit/5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9\n\n---\n\nRelease note (sql change): `ALTER TABLE ... INJECT STATISTICS ...` will\nnow issue notices when the given statistics JSON includes non-existent\ncolumns, rather than resulting in an error. Any statistics in the JSON\nfor existing columns will be injected successfully.", + sourceCommitSha: "5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9", + title: "PR #79069 - sql: ignore non-existent columns when injecting stats", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/79069 +Commit: https://github.com/cockroachdb/cockroach/commit/5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9 + +--- + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`, }, }, }, @@ -215,106 +540,340 @@ func TestGetCommits(t *testing.T) { { Sha: "88be04bd64283b1d77000a3f88588e603465e81b", Commit: ghPullCommitMsg{ - Message: "changefeedccl: remove the default values from SHOW\nCHANGEFEED JOB output\n\nCurrently, when a user alters a changefeed, we\ninclude the default options in the SHOW CHANGEFEED\nJOB output. In this PR we prevent the default values\nfrom being displayed.\n\nRelease note (enterprise change): Remove the default\nvalues from the SHOW CHANGEFEED JOB output", + Message: `changefeedccl: remove the default values from SHOW +CHANGEFEED JOB output + +Currently, when a user alters a changefeed, we +include the default options in the SHOW CHANGEFEED +JOB output. In this PR we prevent the default values +from being displayed. + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`, }, }, }, prNumber: 79361, - commits: []commit{ + issues: []docsIssue{ { - sha: "88be04bd64283b1d77000a3f88588e603465e81b", - title: "changefeedccl: remove the default values from SHOW", - releaseNote: "Related PR: https://github.com/cockroachdb/cockroach/pull/79361\nCommit: https://github.com/cockroachdb/cockroach/commit/88be04bd64283b1d77000a3f88588e603465e81b\n\n---\n\nRelease note (enterprise change): Remove the default\nvalues from the SHOW CHANGEFEED JOB output", + sourceCommitSha: "88be04bd64283b1d77000a3f88588e603465e81b", + title: "PR #79361 - changefeedccl: remove the default values from SHOW", + body: `Related PR: https://github.com/cockroachdb/cockroach/pull/79361 +Commit: https://github.com/cockroachdb/cockroach/commit/88be04bd64283b1d77000a3f88588e603465e81b + +--- + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`, }, }, }, } for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - result := getCommits(tc.pullCommit, tc.prNumber) - assert.Equal(t, tc.commits, result) + result := getIssues(tc.pullCommit, tc.prNumber) + assert.Equal(t, tc.issues, result) }) } } -func TestFormatReleaseNote(t *testing.T) { +func TestFormatReleaseNotes(t *testing.T) { testCases := []struct { prNum string sha string message string - rn string + rns []string }{ { - prNum: "79069", - sha: "5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9", - message: "sql: ignore non-existent columns when injecting stats\n\nPreviously, an `ALTER TABLE ... INJECT STATS` command would return an\nerror if the given stats JSON included any columns that were not present\nin the table descriptor. Statistics in statement bundles often include\ndropped columns, so reproducing issues with a bundle required tediously\nremoving stats for these columns. This commit changes the stats\ninjection behavior so that a notice is issued for stats with\nnon-existent columns rather than an error. Any stats for existing\ncolumns will be injected successfully.\n\nInforms #68184\n\nRelease note (sql change): `ALTER TABLE ... INJECT STATISTICS ...` will\nnow issue notices when the given statistics JSON includes non-existent\ncolumns, rather than resulting in an error. Any statistics in the JSON\nfor existing columns will be injected successfully.", - rn: "Related PR: https://github.com/cockroachdb/cockroach/pull/79069\nCommit: https://github.com/cockroachdb/cockroach/commit/5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9\n\n---\n\nRelease note (sql change): `ALTER TABLE ... INJECT STATISTICS ...` will\nnow issue notices when the given statistics JSON includes non-existent\ncolumns, rather than resulting in an error. Any statistics in the JSON\nfor existing columns will be injected successfully.", + prNum: "79069", + sha: "5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9", + message: `sql: ignore non-existent columns when injecting stats + +Previously, an ` + "`ALTER TABLE ... INJECT STATS`" + ` command would return an +error if the given stats JSON included any columns that were not present +in the table descriptor. Statistics in statement bundles often include +dropped columns, so reproducing docsIssues with a bundle required tediously +removing stats for these columns. This commit changes the stats +injection behavior so that a notice is issued for stats with +non-existent columns rather than an error. Any stats for existing +columns will be injected successfully. + +Informs #68184 + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`, + rns: []string{`Related PR: https://github.com/cockroachdb/cockroach/pull/79069 +Commit: https://github.com/cockroachdb/cockroach/commit/5ec9343b0e0a00bfd4603e55ca6533e2b77db2f9 + +--- + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`}, }, { - prNum: "79361", - sha: "88be04bd64283b1d77000a3f88588e603465e81b", - message: "changefeedccl: remove the default values from SHOW\nCHANGEFEED JOB output\n\nCurrently, when a user alters a changefeed, we\ninclude the default options in the SHOW CHANGEFEED\nJOB output. In this PR we prevent the default values\nfrom being displayed.\n\nRelease note (enterprise change): Remove the default\nvalues from the SHOW CHANGEFEED JOB output", - rn: "Related PR: https://github.com/cockroachdb/cockroach/pull/79361\nCommit: https://github.com/cockroachdb/cockroach/commit/88be04bd64283b1d77000a3f88588e603465e81b\n\n---\n\nRelease note (enterprise change): Remove the default\nvalues from the SHOW CHANGEFEED JOB output", + prNum: "79361", + sha: "88be04bd64283b1d77000a3f88588e603465e81b", + message: `changefeedccl: remove the default values from SHOW +CHANGEFEED JOB output + +Currently, when a user alters a changefeed, we +include the default options in the SHOW CHANGEFEED +JOB output. In this PR we prevent the default values +from being displayed. + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`, + rns: []string{`Related PR: https://github.com/cockroachdb/cockroach/pull/79361 +Commit: https://github.com/cockroachdb/cockroach/commit/88be04bd64283b1d77000a3f88588e603465e81b + +--- + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`}, }, { - prNum: "78685", - sha: "1d7811d5d14f9c7e106c3ec92de9c66192f19604", - message: "opt: do not cross-join input of semi-join\n\nThis commit fixes a logical correctness bug caused when\n`GenerateLookupJoins` cross-joins the input of a semi-join with a set of\nconstant values to constrain the prefix columns of the lookup index. The\ncross-join is an invalid transformation because it increases the size of\nthe join's input and can increase the size of the join's output.\n\nWe already avoid these cross-joins for left and anti-joins (see #59646).\nWhen addressing those cases, the semi-join case was incorrectly assumed\nto be safe.\n\nFixes #78681\n\nRelease note (bug fix): A bug has been fixed which caused the optimizer\nto generate invalid query plans which could result in incorrect query\nresults. The bug, which has been present since version 21.1.0, can\nappear if all of the following conditions are true: 1) the query\ncontains a semi-join, such as queries in the form:\n`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`,\n2) the inner table has an index containing the equality column, like\n`t2.a` in the example query, 3) the index contains one or more\ncolumns that prefix the equality column, and 4) the prefix columns are\n`NOT NULL` and are constrained to a set of constant values via a `CHECK`\nconstraint or an `IN` condition in the filter.", - rn: "", + prNum: "78685", + sha: "1d7811d5d14f9c7e106c3ec92de9c66192f19604", + message: `opt: do not cross-join input of semi-join + +This commit fixes a logical correctness bug caused when +` + "`GenerateLookupJoins`" + ` cross-joins the input of a semi-join with a set of +constant values to constrain the prefix columns of the lookup index. The +cross-join is an invalid transformation because it increases the size of +the join's input and can increase the size of the join's output. + +We already avoid these cross-joins for left and anti-joins (see #59646). +When addressing those cases, the semi-join case was incorrectly assumed +to be safe. + +Fixes #78681 + +Release note (bug fix): A bug has been fixed which caused the optimizer +to generate invalid query plans which could result in incorrect query +results. The bug, which has been present since version 21.1.0, can +appear if all of the following conditions are true: 1) the query +contains a semi-join, such as queries in the form: +` + "`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`" + `, +2) the inner table has an index containing the equality column, like +` + "`t2.a`" + ` in the example query, 3) the index contains one or more +columns that prefix the equality column, and 4) the prefix columns are +` + "`NOT NULL`" + " and are constrained to a set of constant values via a " + "`CHECK`" + ` +constraint or an ` + "`IN`" + " condition in the filter.", + rns: []string{}, }, { - prNum: "66328", - sha: "0f329965acccb3e771ec1657c7def9e881dc78bb", - message: "util/log: report the logging format at the start of new files\n\nRelease note (cli change): When log entries are written to disk,\nthe first few header lines written at the start of every new file\nnow report the configured logging format.", - rn: "Related PR: https://github.com/cockroachdb/cockroach/pull/66328\nCommit: https://github.com/cockroachdb/cockroach/commit/0f329965acccb3e771ec1657c7def9e881dc78bb\n\n---\n\nRelease note (cli change): When log entries are written to disk,\nthe first few header lines written at the start of every new file\nnow report the configured logging format.", + prNum: "66328", + sha: "0f329965acccb3e771ec1657c7def9e881dc78bb", + message: `util/log: report the logging format at the start of new files + +Release note (cli change): When log entries are written to disk, +the first few header lines written at the start of every new file +now report the configured logging format.`, + rns: []string{`Related PR: https://github.com/cockroachdb/cockroach/pull/66328 +Commit: https://github.com/cockroachdb/cockroach/commit/0f329965acccb3e771ec1657c7def9e881dc78bb + +--- + +Release note (cli change): When log entries are written to disk, +the first few header lines written at the start of every new file +now report the configured logging format.`}, }, { - prNum: "66328", - sha: "fb249c7140b634a53dca2967c946bc78ba927e1a", - message: "cli: report explicit log config in logs\n\nThis increases troubleshootability.\n\nRelease note: None", - rn: "", + prNum: "66328", + sha: "fb249c7140b634a53dca2967c946bc78ba927e1a", + message: `cli: report explicit log config in logs + +This increases troubleshootability. + +Release note: None`, + rns: []string{}, }, } for _, tc := range testCases { t.Run(tc.prNum, func(t *testing.T) { prNumInt, _ := strconv.Atoi(tc.prNum) - result := formatReleaseNote(tc.message, prNumInt, tc.sha) - assert.Equal(t, tc.rn, result) + result := formatReleaseNotes(tc.message, prNumInt, tc.sha) + assert.Equal(t, tc.rns, result) }) } } func TestFormatTitle(t *testing.T) { testCases := []struct { - testName string - message string - title string + testName string + message string + prNumber int + index int + totalLength int + title string }{ - { testName: "Format Title 1", - message: "sql: ignore non-existent columns when injecting stats\n\nPreviously, an `ALTER TABLE ... INJECT STATS` command would return an\nerror if the given stats JSON included any columns that were not present\nin the table descriptor. Statistics in statement bundles often include\ndropped columns, so reproducing issues with a bundle required tediously\nremoving stats for these columns. This commit changes the stats\ninjection behavior so that a notice is issued for stats with\nnon-existent columns rather than an error. Any stats for existing\ncolumns will be injected successfully.\n\nInforms #68184\n\nRelease note (sql change): `ALTER TABLE ... INJECT STATISTICS ...` will\nnow issue notices when the given statistics JSON includes non-existent\ncolumns, rather than resulting in an error. Any statistics in the JSON\nfor existing columns will be injected successfully.", - title: "sql: ignore non-existent columns when injecting stats", + message: `sql: ignore non-existent columns when injecting stats + +Previously, an ` + "`ALTER TABLE ... INJECT STATS`" + ` command would return an +error if the given stats JSON included any columns that were not present +in the table descriptor. Statistics in statement bundles often include +dropped columns, so reproducing docsIssues with a bundle required tediously +removing stats for these columns. This commit changes the stats +injection behavior so that a notice is issued for stats with +non-existent columns rather than an error. Any stats for existing +columns will be injected successfully. + +Informs #68184 + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`, + prNumber: 12345, + index: 1, + totalLength: 1, + title: "PR #12345 - sql: ignore non-existent columns when injecting stats", }, { testName: "Format Title 2", - message: "changefeedccl: remove the default values from SHOW\nCHANGEFEED JOB output\n\nCurrently, when a user alters a changefeed, we\ninclude the default options in the SHOW CHANGEFEED\nJOB output. In this PR we prevent the default values\nfrom being displayed.\n\nRelease note (enterprise change): Remove the default\nvalues from the SHOW CHANGEFEED JOB output", - title: "changefeedccl: remove the default values from SHOW", + message: `changefeedccl: remove the default values from SHOW +CHANGEFEED JOB output + +Currently, when a user alters a changefeed, we +include the default options in the SHOW CHANGEFEED +JOB output. In this PR we prevent the default values +from being displayed. + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`, + prNumber: 54321, + index: 1, + totalLength: 1, + title: "PR #54321 - changefeedccl: remove the default values from SHOW", }, { testName: "Format Title 3", - message: "opt: do not cross-join input of semi-join\n\nThis commit fixes a logical correctness bug caused when\n`GenerateLookupJoins` cross-joins the input of a semi-join with a set of\nconstant values to constrain the prefix columns of the lookup index. The\ncross-join is an invalid transformation because it increases the size of\nthe join's input and can increase the size of the join's output.\n\nWe already avoid these cross-joins for left and anti-joins (see #59646).\nWhen addressing those cases, the semi-join case was incorrectly assumed\nto be safe.\n\nFixes #78681\n\nRelease note (bug fix): A bug has been fixed which caused the optimizer\nto generate invalid query plans which could result in incorrect query\nresults. The bug, which has been present since version 21.1.0, can\nappear if all of the following conditions are true: 1) the query\ncontains a semi-join, such as queries in the form:\n`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`,\n2) the inner table has an index containing the equality column, like\n`t2.a` in the example query, 3) the index contains one or more\ncolumns that prefix the equality column, and 4) the prefix columns are\n`NOT NULL` and are constrained to a set of constant values via a `CHECK`\nconstraint or an `IN` condition in the filter.", - title: "opt: do not cross-join input of semi-join", + message: `opt: do not cross-join input of semi-join + +This commit fixes a logical correctness bug caused when +` + "`GenerateLookupJoins`" + ` cross-joins the input of a semi-join with a set of +constant values to constrain the prefix columns of the lookup index. The +cross-join is an invalid transformation because it increases the size of +the join's input and can increase the size of the join's output. + +We already avoid these cross-joins for left and anti-joins (see #59646). +When addressing those cases, the semi-join case was incorrectly assumed +to be safe. + +Fixes #78681 + +Release note (bug fix): A bug has been fixed which caused the optimizer +to generate invalid query plans which could result in incorrect query +results. The bug, which has been present since version 21.1.0, can +appear if all of the following conditions are true: 1) the query +contains a semi-join, such as queries in the form: +` + "`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`" + `, +2) the inner table has an index containing the equality column, like +` + "`t2.a`" + ` in the example query, 3) the index contains one or more +columns that prefix the equality column, and 4) the prefix columns are +` + "`NOT NULL`" + " and are constrained to a set of constant values via a " + "`CHECK`" + ` +constraint or an ` + "`IN`" + " condition in the filter.", + prNumber: 65432, + index: 1, + totalLength: 1, + title: "PR #65432 - opt: do not cross-join input of semi-join", }, { testName: "Format Title 4", - message: "util/log: report the logging format at the start of new files\n\nRelease note (cli change): When log entries are written to disk,\nthe first few header lines written at the start of every new file\nnow report the configured logging format.", - title: "util/log: report the logging format at the start of new files", + message: `util/log: report the logging format at the start of new files + +Release note (cli change): When log entries are written to disk, +the first few header lines written at the start of every new file +now report the configured logging format.`, + prNumber: 23456, + index: 1, + totalLength: 1, + title: "PR #23456 - util/log: report the logging format at the start of new files", + }, + { + testName: "Format Title 5", + message: `sql: ignore non-existent columns when injecting stats + +Previously, an ` + "`ALTER TABLE ... INJECT STATS`" + ` command would return an +error if the given stats JSON included any columns that were not present +in the table descriptor. Statistics in statement bundles often include +dropped columns, so reproducing docsIssues with a bundle required tediously +removing stats for these columns. This commit changes the stats +injection behavior so that a notice is issued for stats with +non-existent columns rather than an error. Any stats for existing +columns will be injected successfully. + +Informs #68184 + +Release note (sql change): ` + "`ALTER TABLE ... INJECT STATISTICS ...`" + ` will +now docsIssue notices when the given statistics JSON includes non-existent +columns, rather than resulting in an error. Any statistics in the JSON +for existing columns will be injected successfully.`, + prNumber: 34567, + index: 2, + totalLength: 3, + title: "PR #34567 - sql: ignore non-existent columns when injecting stats (2 of 3)", + }, + { + testName: "Format Title 6", + message: `changefeedccl: remove the default values from SHOW +CHANGEFEED JOB output + +Currently, when a user alters a changefeed, we +include the default options in the SHOW CHANGEFEED +JOB output. In this PR we prevent the default values +from being displayed. + +Release note (enterprise change): Remove the default +values from the SHOW CHANGEFEED JOB output`, + prNumber: 76543, + index: 1, + totalLength: 6, + title: "PR #76543 - changefeedccl: remove the default values from SHOW (1 of 6)", + }, + { + testName: "Format Title 7", + message: `opt: do not cross-join input of semi-join + +This commit fixes a logical correctness bug caused when +` + "`GenerateLookupJoins`" + ` cross-joins the input of a semi-join with a set of +constant values to constrain the prefix columns of the lookup index. The +cross-join is an invalid transformation because it increases the size of +the join's input and can increase the size of the join's output. + +We already avoid these cross-joins for left and anti-joins (see #59646). +When addressing those cases, the semi-join case was incorrectly assumed +to be safe. + +Fixes #78681 + +Release note (bug fix): A bug has been fixed which caused the optimizer +to generate invalid query plans which could result in incorrect query +results. The bug, which has been present since version 21.1.0, can +appear if all of the following conditions are true: 1) the query +contains a semi-join, such as queries in the form: +` + + "`SELECT * FROM t1 WHERE EXISTS (SELECT * FROM t2 WHERE t1.a = t2.a);`" + `, +2) the inner table has an index containing the equality column, like +` + "`t2.a`" + ` in the example query, 3) the index contains one or more +columns that prefix the equality column, and 4) the prefix columns are +` + "`NOT NULL`" + " and are constrained to a set of constant values via a " + "`CHECK`" + ` +constraint or an ` + "`IN`" + " condition in the filter.", + prNumber: 45678, + index: 5, + totalLength: 7, + title: "PR #45678 - opt: do not cross-join input of semi-join (5 of 7)", }, } for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - result := formatTitle(tc.message) + result := formatTitle(tc.message, tc.prNumber, tc.index, tc.totalLength) assert.Equal(t, tc.title, result) }) }