sql: ignore empty statements during Prepare

[maddyblue]

Fixes #9093
Fixes #9518

---

This change is

[a-robinson]

---

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

---

sql/pgwire_test.go, line 848 at r1 (raw file):

              }
          } else {
              if rowsAffected, _ := result.RowsAffected(); rowsAffected != test.rowsAffected {

Why do we no longer want to check the error?

---

Comments from Reviewable

[tamird]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

sql/executor.go, line 349 at r1 (raw file):

      return nil, err
  }
  if len(stmts) > 1 {

switch len(stmts) {
...
}

---

sql/pgwire_test.go, line 848 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Why do we no longer want to check the error?

+1, let's still check it (string comparison, womp womp).

---

Comments from Reviewable

[maddyblue]

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

---

sql/pgwire_test.go, line 848 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

+1, let's still check it (string comparison, womp womp).

Because in this specific case it's really, really stupid. We have to add a new property that is the rowsAffectedError (can't use the existing error field because that's used for something else) and then do the thing to make sure both have the same error or neither do. It's a more lines of code for just the stupidest error. But whatever I'll add it.

---

Comments from Reviewable

[knz]

---

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

---

sql/pgwire_test.go, line 848 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Because in this specific case it's really, really stupid. We have to add a new property that is the rowsAffectedError (can't use the existing error field because that's used for something else) and then do the thing to make sure both have the same error or neither do. It's a more lines of code for just the stupidest error. But whatever I'll add it.

No, just keep it simple:

if rowsAffected, err := result.RowsAffected(); rowsAffected != test.rowsAffected {
  t.Errorf("%s: %v: expected %v, got %v (error %+v)", query, test.qargs, test.rowsAffected, rowsAffected, err)
}

No need for an additional test field.

---

Comments from Reviewable

[maddyblue]

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

---

sql/pgwire_test.go, line 848 at r1 (raw file):

Previously, knz (kena) wrote…

No, just keep it simple:

if rowsAffected, err := result.RowsAffected(); rowsAffected != test.rowsAffected {
  t.Errorf("%s: %v: expected %v, got %v (error %+v)", query, test.qargs, test.rowsAffected, rowsAffected, err)
}

No need for an additional test field.

This won't catch the case where rowsAffected returns 0 and an error if test.rowsAffected is also 0.

---

Comments from Reviewable

[tamird]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

sql/pgwire_test.go, line 855 at r2 (raw file):

          } else {
              rowsAffected, err := result.RowsAffected()
              hasRAErr := err != nil

eh, this could be stronger; in general, we should not accept just any error here; this is what testutils.IsError is for.

---

Comments from Reviewable