Re: Portal->commandTag as an enum

> On Feb 11, 2020, at 11:09 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2020-Feb-07, Mark Dilger wrote:
>
>> Andres,
>>
>> The previous patch set seemed to cause confusion, having separated
>> changes into multiple files. The latest patch, heavily influenced by
>> your review, is all in one file, attached.
>
> Cool stuff.

Thanks for the review!

> I think is a little confused about what is source and what is generated.

The perl file generates nothing. It merely checks that the .h and .c files are valid and consistent with each other.

> I'm not clear why commandtag.c is a C file at all; wouldn't it be
> simpler to have it as a Data::Dumper file or some sort of Perl struct,
> so that it can be read easily by the Perl file? Similar to the
> src/include/catalog/pg_*.dat files. That script can then generate all
> the needed .c and .h files, which are not going to be part of the source
> tree, and will always be in-sync and won't have the formatting
> strictness about it. And you won't have the Martian syntax you had to
> use in the commandtag.c file.

I thought about generating the files rather than merely checking them. I could see arguments both ways. I wasn’t sure whether there would be broad support for having yet another perl script generating source files, nor for the maintenance burden of having to do that on all platforms. Having a perl script that merely sanity checks the source files has the advantage that there is no requirement for it to function on all platforms. There’s not even a requirement for it to be committed to the tree, since you could also argue that the maintenance burden of the script outweighs the burden of getting the source files right by hand.

> As for API, please don't shorten things such as SetQC, just use
> SetQueryCompletion. Perhaps call it SetCompletionTag or SetCommandTag?.
> (I'm not sure about the name "QueryCompletionData"; maybe CommandTag or
> QueryTag would work better for that struct.

I am happy to rename it as SetQueryCompletion.

> There seems to be a lot of
> effort in separating QueryCompletion from CommandTag; is that really
> necessary?)

For some code paths, prior to this patch, the commandTag gets changed before returning, and I’m not just talking about the change where the rowcount gets written into the commandTag string. I have a work-in-progress patch to provide system views to track the number of commands executed of each type, and that patch includes the ability to distinguish between what the command started as and what it completed as, so I do want to keep those concepts separate. I rejected the “SetCommandTag” naming suggestion above because we’re really setting information about the completion (what it finished as) and not the command (what it started as). I rejected the “SetCompletionTag” naming because it’s not just the tag that is being set, but both the tag and the row count. I am happy with “SetQueryCompletion” because that naming is consistent with setting the pair of values.

> Lastly, we have a convention that we have a struct called
> FooData, and a typedef FooData *Foo, then use the latter in the API.
> We don't adhere to that 100%, and some people dislike it, but I'd rather
> be consistent and not be passing "FooData *" around; it's just noisier.

I’m familiar with the convention, and don’t like it, so I’ll have to look at a better way of naming this. I specifically don’t like it because it makes a mess of using the const qualifier.

Once again, thanks for the review! I will work to get another version of this patch posted around the time I post (separately) the command stats patch.

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