Re: Portal->commandTag as an enum

> On Feb 28, 2020, at 5:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> writes:
>>> On Feb 28, 2020, at 3:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Is there a way to drop that logic altogether by making the tagname string
>>> be "INSERT 0" for the INSERT case? Or would the zero bleed into other
>>> places where we don't want it?
>
>> In general, I don't think we want to increase the number of distinct
>> tags. Which command you finished running and whether you want a
>> rowcount and/or lastoid are orthogonal issues.
>
> Well, my thought is that last_oid is gone and it isn't ever coming back.
> So the less code we use supporting a dead feature, the better.
>
> If we can't remove the special case in EndCommand() altogether, I'd be
> inclined to hard-code it as "if (tag == CMDTAG_INSERT ..." rather than
> expend infrastructure on treating last_oid as a live option for commands
> to have.

You may want to think about the embedding of InvalidOid into the EndCommand output differently from how you think about the embedding of the rowcount into the EndCommand output, but my preference is to treat these issues the same and make a strong distinction between the commandtag and the embedded oid and/or rowcount. It's hard to say how many future features would be crippled by having the embedded InvalidOid in the commandtag, but as an example *right now* in the works, we have a feature to count how many commands of a given type have been executed. It stands to reason that feature, whether accepted in its current form or refactored, would not want to show users a pg_stats table like this:

cnt command
---- -------------
5 INSERT 0
37 SELECT

What the heck is the zero doing after the INSERT? That's the hardcoded InvalidOid that you are apparently arguing for. You could get around that by having the pg_sql_stats patch have its own separate set of command tag strings, but why would we intentionally design that sort of duplication into the solution?

As for hardcoding the behavior of whether to embed a rowcount in the output from EndCommand; In src/backend/replication/walsender.c, exec_replication_command() returns "SELECT" from EndCommand, and not "SELECT $rowcount" like everywhere else. The patch as submitted does not change behavior. It only refactors the code while preserving the current behavior. So we would have to agree that the patch can change how exec_replication_command() behaves and start embedding a rowcount there, too, if we want to make SELECT behave the same everywhere.

There is another problem, though, which is that if we're hoping to eventually abate this historical behavior and stop embedding InvalidOid and/or rowcount in the commandtag returned from EndCommand, it might be necessary (for backward compatibility with clients) to do that incrementally, in which case we still need the distinction between commandtags and formats to exist in the code. How else can you say that, for example, in the next rev of the protocol that we're not going to embed InvalidOid anymore, but we will continue to return it for clients who connect via the older protocol? What if the next rev of the protocol still returns rowcount, but in a way that doesn't require the clients to implement (or link to) a parser that extracts the rowcount by parsing a string?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company