sql: properly log copy error on error

[otan]

This commit fixes an earlier commit by ensuring maybeLogStatement for
copy actually logs the error that comes out of it.

Release justification: bug fix
Release note: None

[cockroach-teamcity]

This change is 

[cucaroach]

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @otan)

---

pkg/sql/conn_executor.go line 2459 at r1 (raw file):

			ex.extraTxnState.txnCounter,
			cm.numInsertedRows(),
			copyError,

Man I actually looked at this retErr thing and thought about if it was correct and convinced myself it was 🤦 My bad!

[otan]

cc @cucaroach
probably worth another look, added some extra state to set to make it not panic!

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @otan, and @rafiss)

---

pkg/sql/conn_executor.go line 2424 at r3 (raw file):

	ann := tree.MakeAnnotations(0)
	ex.planner.extendedEvalCtx.Context.Annotations = &ann
	ex.planner.extendedEvalCtx.Context.Placeholders = &tree.PlaceholderInfo{}

i don't follow the reasoning for zeroing out annotations and placeholders or changing ex.planner.stmt

---

pkg/sql/conn_executor.go line 2427 at r3 (raw file):

	ex.planner.curPlan.stmt = &ex.planner.stmt
	var cm copyMachineInterface
	var err error

i thought it was more readable in the earlier revision where it was named coopyErr

---

pkg/sql/conn_executor.go line 2430 at r3 (raw file):

	defer func() {
		var numInsertedRows int
		if cm != nil {

you don't need this nil check, since you made the method able to handle nil receivers. i'd say do one nil check or the other

---

pkg/sql/conn_executor.go line 2434 at r3 (raw file):

		}
		// These fields are not available in COPY, so use the empty value.
		var stmtFingerprintID roachpb.StmtFingerprintID

can we call roachpb.ConstructStatementFingerprintID instead of using the empty value?

---

pkg/sql/event_log.go line 279 at r3 (raw file):

	// Inject the common fields into the payload provided by the caller.
	injectCommonFields := func(event logpb.EventPayload) error {
		if txn != nil {

in which cases is txn nil?

[otan]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @otan, and @rafiss)

---

pkg/sql/conn_executor.go line 2459 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Man I actually looked at this retErr thing and thought about if it was correct and convinced myself it was 🤦 My bad!

Done.

---

pkg/sql/conn_executor.go line 2424 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't follow the reasoning for zeroing out annotations and placeholders or changing ex.planner.stmt

without this it panics on

cockroach/pkg/sql/event_log.go

Line 189 in 92b550b

p.extendedEvalCtx.Context.Annotations, opt.toFlags(),

---

pkg/sql/conn_executor.go line 2427 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i thought it was more readable in the earlier revision where it was named coopyErr

i'm 50:50, this is the original error

---

pkg/sql/conn_executor.go line 2430 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

you don't need this nil check, since you made the method able to handle nil receivers. i'd say do one nil check or the other

no you do, because the interface could point to nil if cm never gets registered.

---

pkg/sql/event_log.go line 279 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

in which cases is txn nil?

when copy is the first statement. we never actually set txn up in the planner for copy.

[otan]

@rafiss mind giving this another look? having to rebase a lot so fearing the backport even more now 🙃

[rafiss]

this needs a small change to the FmtCtx, then feel free to merge

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @otan, and @rafiss)

---

pkg/sql/conn_executor.go line 2448 at r4 (raw file):

		}
		// These fields are not available in COPY, so use the empty value.
		f := tree.NewFmtCtx(tree.FmtAnonymize | tree.FmtHideConstants)

for making the fingerprint, we only want FmtHideConstants. we don't want FmtAnonymize, since COPY t1 FROM STDIN should not have the same fingerprint as COPY t2 FROM STDIN

part of the confusion here is probably that the param of ConstructStatementFingerprintID is named anonymizedStmt. I missed that when i made 1944c52

[otan]

bors r=rafiss,cucaroach

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)