sql/pgwire: send proper tags when returning an Ack message

[nvanbenschoten]

We currently have a comment here which I have found to be incorrect in practice. For instance, Go's pg driver requires that these tags be returned for operations like BEGIN, COMMIT, or ROLLBACK. See here.

[tamird]

How come this doesn't manifest as a test failure? Can you reproduce this in a test?

[dt]

@mjibson and I were looking at this yesterday (in process of looking at #3849). The tags are described more at http://www.postgresql.org/docs/current/static/protocol-message-formats.html

[nvanbenschoten]

@tamird I don't think we ever use any of these commands in a test with a postgre driver. For instance, the only place in our entire codebase where we open a transaction from a sql/driver is when testing the old cockroach driver.

[tamird]

Ah, right. Seems like we send BEGIN as a "statement" in the logic tests, side-stepping this issue.

[dt]

like #3852, despite being pg-wire specific, this will probably want to get propagated through the driver and handled by the actual parser, right?

[nvanbenschoten]

It seems like the link @dt provided is missing some of the tags that are sent back by postgres. I'm not sure if that means postgres returns more than the actual protocol defines, but it seems like drivers are expecting more that the protocol. Here's the list of all tags returned by postgres. It requires a handle on the parse tree, so I assume this would need to get propagated through the driver and handled by the parser.

[dt]

It's possible the go driver is unusual in actually looking at the tag and not just the result code:
looking at ruby's pg, (which leaves most of the work to libpq) it doesn't check.
psycopg2, if I've managed to follow all the c linked c modules, doesn't either https://github.com/psycopg/psycopg2/blob/master/psycopg/pqpath.c#L461

[nvanbenschoten]

I think you're right that it's not completely standard to check, but it seems to me like a good idea to comply with what Postgres sends if possible. Also, interestingly the commit that introduced the tag checking to Go's primary sql/driver cited libpg as doing the same.

[dt]

I'm guessing that commit comment is referring to https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/fe-protocol2.c#L1074 though it isn't completely clear to me what xactStatus is for -- I guess it is surfaced in PQtransactionStatus though.