LGTM

Looks good overall, but some comments.

Reviewed 13 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/logictest/testdata/logic_test/tuple, line 712 at r1 (raw file):

----
r
("(""(1,""""2"""",3)"",""(4,""""5"""")"",""(6)"")","(7,8)","(""9"")")

This is so ugly. :(

pkg/sql/sem/tree/datum.go, line 413 at r1 (raw file):

func (d *DBool) Format(ctx *FmtCtx) {
	if ctx.HasFlags(pgwireFormat) {
		v := byte('f')

Why store this value?

if bool(*d) {
  ctx.WriteByte('t')
} else {
  ctx.WriteByte('f')
}
return

pkg/sql/sem/tree/datum.go, line 2734 at r1 (raw file):

	ctx.WriteByte('(')
	comma := ""
	for _, v := range d.D {

Not sure if this is better, but it reads a bit nicer I think. Also this way you don't set the comma variable each time.

for i, v := range d.D {
  if i > 1 {
    ctx.WriteString(", ")
  }
  ctx.FormatNode(v)
}

I really want a bool for first and last and I'm kinda surprised that they were not included in golang's range operator.

pkg/sql/sem/tree/datum.go, line 3026 at r1 (raw file):

	}

	ctx.WriteString("ARRAY[")

Here too,

pkg/sql/sem/tree/pgwire_encode.go, line 37 at r1 (raw file):

	// string printer called pgwireFormatStringInTuple().
	ctx.WriteByte('(')
	comma := ""

See my other comments, I'd rather a check on the iteration count than setting comma on each pass. For all of the commas in this file.

pkg/sql/sem/tree/pgwire_encode.go, line 57 at r1 (raw file):

func pgwireFormatStringInTuple(buf *bytes.Buffer, in string) {
	// TODO(knz): to be fully pg-compliant, this function should avoid

For all of these, this seems pretty easy to do. Is there a reason to not just use a different buffer then copy from one to the other with or without the double quotes?

pkg/sql/sem/tree/pgwire_encode.go, line 111 at r1 (raw file):

func pgwireFormatStringInArray(buf *bytes.Buffer, in string) {
	// TODO(knz): to be fully pg-compliant, this function should avoid

same here

pkg/util/stringencoding/string_encoding.go, line 86 at r1 (raw file):

// EncodeChar is used internally to write out a character from
// a larger string to a buffer.
func EncodeChar(buf *bytes.Buffer, entireString string, currentRune rune, currentIdx int) {

This function looks like it could use a unit test.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/logictest/testdata/logic_test/tuple, line 712 at r1 (raw file):

It is ugly but it is also correct. See my other comment below.

pkg/sql/pgwire/types.go, line 165 at r1 (raw file):

This way is actually faster / more efficient. Vivek taught me this trick. I'm using it as a general-purpose pattern now.

pkg/sql/sem/tree/datum.go, line 413 at r1 (raw file):

I don't care either way. Changed.

pkg/sql/sem/tree/datum.go, line 2734 at r1 (raw file):

This way is actually faster / more efficient. Vivek taught me this trick. I'm using it as a general-purpose pattern now.

pkg/sql/sem/tree/datum.go, line 3026 at r1 (raw file):

This way is actually faster / more efficient. Vivek taught me this trick. I'm using it as a general-purpose pattern now.

pkg/sql/sem/tree/format.go, line 87 at r1 (raw file):

This combination still allows up to combine other flags (e.g. anonymize).

pkg/sql/sem/tree/pgwire_encode.go, line 37 at r1 (raw file):

ditto my other comment

pkg/sql/sem/tree/pgwire_encode.go, line 57 at r1 (raw file):

Because performance.

@justinj has decided to do it this way originally and reasoned as follows: it is correct to add the double quotes when they are not required. Removing them is only "niceness" to the human user. I'm not revisiting this here -- we can make a separate PR for that.

pkg/sql/sem/tree/pgwire_encode.go, line 111 at r1 (raw file):

ditto

pkg/util/stringencoding/string_encoding.go, line 86 at r1 (raw file):

None of these functions are tested directly in this package. Their unit tests live in format_test.go in pkg/sql/sem/tree and pkg/sql/lex.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/logictest/testdata/logic_test/srfs, line 543 at r2 (raw file):

SELECT ((i.keys).*, 123) FROM (SELECT information_schema._pg_expandarray(ARRAY[3,2,1]) AS keys) AS i
----
((3, 1),123)

Jordan just noticed there is a bug here, but unrelated to this PR. Filed as #28203