sql/pgwire: send CommandComplete for DDLs

also explicitly send EmptyQuery than execute empty query strings.

Fixes cockroachdb#3849

Starts to address cockroachdb#3852 but, as noted in TODO and on the issue,
correctly handling more complex empty queries (e.g. "; ; ;") is
better left to the actual parser, which will require plumbing that
through the driver too.

sql/pgwire/v3.go

@@ -22,6 +22,7 @@ import (
 	"fmt"
 	"net"
 	"strconv"
+	"strings"
 
 	"github.com/lib/pq/oid"
 
@@ -544,6 +545,13 @@ func (c *v3Conn) handleExecute(buf *readBuffer) error {
 func (c *v3Conn) executeStatements(stmts string, params []driver.Datum, formatCodes []formatCode, sendDescription bool) error {
 	c.session.Database = c.opts.database
 
+	// TODO(dt): this is a clumsy check better left to the actual parser. #3852
+	if len(strings.TrimSpace(stmts)) == 0 {
+		// Skip executor and just send EmptyQueryResponse.
+		c.writeBuf.initMsg(serverMsgEmptyQuery)
+		return c.writeBuf.finishMsg(c.wr)
+	}
+
 	resp, _, err := c.executor.ExecuteStatements(c.opts.user, c.session, stmts, params)
 	if err != nil {
 		return c.sendError(err.Error())
@@ -612,11 +620,6 @@ func (c *v3Conn) sendResponse(resp driver.Response, formatCodes []formatCode, se
 		}
 
 		switch result := result.GetUnion().(type) {
-		case *driver.Response_Result_DDL_:
-			// Send EmptyQueryResponse.
-			c.writeBuf.initMsg(serverMsgEmptyQuery)
-			return c.writeBuf.finishMsg(c.wr)
-
 		case *driver.Response_Result_RowsAffected:
 			// Send CommandComplete.
 			// TODO(bdarnell): tags for other types of commands.
@@ -672,9 +675,9 @@ func (c *v3Conn) sendResponse(resp driver.Response, formatCodes []formatCode, se
 
 		// Ack messages do not have a corresponding protobuf field, so handle those
 		// with a default.
+		// This also includes DDLs which want CommandComplete as well.
 		default:
-			// A real Postgres will send a tag back, but testing so far shows that
-			// clients will accept an empty tag also.
+			// TODO(dt): A real Postgres will send a tag back. #3890
 			if err := c.sendCommandComplete(nil); err != nil {
 				return err
 			}

sql/pgwire_test.go

@@ -325,3 +325,43 @@ func TestPGPrepared(t *testing.T) {
 		}
 	}
 }
+
+func TestCmdCompleteVsEmptyStatements(t *testing.T) {
+	s := server.StartTestServer(t)
+	defer s.Stop()
+
+	pgUrl, cleanupFn := sqlutils.PGUrl(t, s, security.RootUser, os.TempDir(), "TestPGWire")
+	defer cleanupFn()
+
+	db, err := sql.Open("postgres", pgUrl.String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer db.Close()
+
+	if _, err := db.Exec(`CREATE DATABASE IF NOT EXISTS testing`); err != nil {
+		t.Fatal(err)
+	}
+
+	nonempty, err := db.Exec("CREATE TABLE testing.foo (k int)")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// lib/pq handles the empty query response by returning a nil driver.Result.
+	// Unfortunately sql.Exec wraps that in a non-nil sql.Result which doesn't expose
+	// the underlying driver.Result, only methods which (attempt to) dereference it.
+	//
+	// TODO(dt): This would be prettier and generate better failures with testify/assert's helpers.
+	nonempty.RowsAffected() // should not panic if lib/pq returned a non-nil result.
+
+	empty, err := db.Exec(" ")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		recover()
+	}()
+	empty.RowsAffected() // will panic if lib/pq returned a nil result as expected.
+	t.Fatal("should not get here -- empty result from empty query should panic")
+}