> On Feb 28, 2020, at 5:42 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Mark Dilger <mark.dil...@enterprisedb.com> writes:
>>> On Feb 28, 2020, at 3:05 PM, Tom Lane <t...@sss.pgh.pa.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
