sql: Record user-provided value to event log when a setting is changed

[a-robinson]

Rather than using the encoded version, which for state-machine settings
(like the cluster version) can be a protocol buffer.

To make this work, I needed parser.StrVal.Format to respect the provided
format strings. To my untrained eye this looked like a bug, but there
may be a reason for it that I'm not aware of.

Fixes #17854

I hope one of you can correct me on the StrVal.Format change if there really is a reason for it not to respect formatting flags.

[cockroach-teamcity]

This change is 

[dt]

---

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/parser/constant.go, line 356 at r1 (raw file):

		encodeSQLBytes(buf, expr.s)
	} else {
		encodeSQLStringWithFlags(buf, expr.s, f)

does this suggest we want a test that hits this?

---

Comments from Reviewable

[a-robinson]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/parser/constant.go, line 356 at r1 (raw file):

Previously, dt (David Taylor) wrote…

does this suggest we want a test that hits this?

If it's actually the intended behavior, yeah. Done. Although it'd be nice to hear from one other person (@knz? @nvanbenschoten?) that this wasn't intentionally done this way.

---

Comments from Reviewable

[knz]

---

Reviewed 2 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

pkg/sql/parser/constant.go, line 356 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

If it's actually the intended behavior, yeah. Done. Although it'd be nice to hear from one other person (@knz? @nvanbenschoten?) that this wasn't intentionally done this way.

I don't remember actually. What other things were changed that were using this code path when this was introduced the first time?

---

Comments from Reviewable

[knz]

also nit: we usually do not capitalize the first line of commit messages

[a-robinson]

Wow, I've been doing that for a year and never noticed. I'm not alone in doing it, but we're definitely outnumbered. I'll try to switch over in the future.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/constant.go, line 356 at r1 (raw file):

Previously, knz (kena) wrote…

I don't remember actually. What other things were changed that were using this code path when this was introduced the first time?

It traces all the way back to when StrVal was introduced, so it was likely an oversight:
92a6bbb#diff-9498abd5bbdfcd73f8064a4f6e918ee1R239

---

Comments from Reviewable

[nvanbenschoten]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/constant.go, line 356 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

It traces all the way back to when StrVal was introduced, so it was likely an oversight:
92a6bbb#diff-9498abd5bbdfcd73f8064a4f6e918ee1R239

Yeah, that must have just been missed when the Format method was added. Good catch.

---

Comments from Reviewable

[nvanbenschoten]

While staying as far away from a bikeshed as possible, can I ask why we usually don't capitalize the first line of commit messages? Is it an arbitrary convention for the sake of standardization (which I'm all for)?

[knz]

So my overall approach is like this:

• if the commit title starts immediately, then capitalize;
• if the title follows a prefix that names the area where the change occurs (e.g. "sql: ") then don't capitalize, like requested by English grammar: after a colon a capital letter doesn't follow.

Also at a high level since we also have this habit to not end commit titles with a period (I actually disagree with this, but we've been doing that reliably) I never saw them as sentences really and thus didn't see the point of an initial capital.

[nvanbenschoten]

Ok, thanks for the explanation.