> On Feb 2, 2020, at 6:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Mark Dilger <mark.dil...@enterprisedb.com> writes:
>> I put the CommandTag enum in src/common because there wasn’t any reason
>> not to do so. It seems plausible that frontend test frameworks might want
>> access to this enum.
>
Thanks for looking!
> Au contraire, that's an absolutely fundamental mistake. There is
> zero chance of this enum holding still across PG versions, so if
> we expose it to frontend code, we're going to have big problems
> with cross-version compatibility. See our historical problems with
> assuming the enum for character set encodings was the same between
> frontend and backend ... and that set is orders of magnitude more
> stable than this one.
I completely agree that this enum cannot be expected to remain stable across
versions.
For the purposes of this patch, which has nothing to do with frontend tools,
this issue doesn’t matter to me. I’m happy to move this into src/backend.
Is there no place to put code which would be useful for frontend tools without
implying stability? Sure, psql and friends can’t use it, because they need to
be able to connect to servers of other versions. But why couldn’t a test
framework tool use something like this? Could we have someplace like
src/common/volatile for this sort of thing?
>
> As I recall, the hardest problem with de-string-ifying this is the fact
> that for certain tags we include a rowcount in the string. I'd like to
> see that undone --- we have to keep it like that on-the-wire to avoid a
> protocol break, but it'd be best if noplace inside the backend did it that
> way, and we converted at the last moment before sending a CommandComplete
> to the client. Your references to "completion tags" make it sound like
> you've only half done the conversion (though I've not read the patch
> in enough detail to verify).
In v1, I stayed closer to the existing code structure than you are requesting.
I like the direction you’re suggesting that I go, and I’ve begun that
transition in anticipation of posting a v2 patch set soon.
> BTW, the size of the patch is rather depressing, especially if it's
> only half done. Unlike Andres, I'm not even a little bit convinced
> that this is worth the amount of code churn it'll cause. Have you
> done any code-size or speed comparisons?
A fair amount of the code churn is replacing strings with their enum
equivalent, creating the enum itself, and creating a data table mapping enums
to strings. The churn doesn’t look too bad to me when viewing the original vs
new code diff side-by-side.
The second file (v1-0002…) is entirely an extension of the regression tests.
Applying v1-0001… doesn’t entail needing to apply v1-0002… as the code being
tested exists before and after the patch. If you don’t want to apply that
regression test change, that’s fine. It just provides more extensive coverage
of event_triggers over different command tags.
There will be a bit more churn in v2, since I’m changing the code flow a bit
more to avoid generating the strings until they are about to get sent to the
client, per your comments above. That has the advantage that multiple places
in the old code where the completionTag was parsed to get the nprocessed count
back out now doesn’t need any parsing.
I’ll include stats about code-size and speed when I post v2.
Thanks again for reviewing my patch idea!
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
