Re: Portal->commandTag as an enum - Mailing list pgsql-hackers

> On Feb 11, 2020, at 11:09 AM, Alvaro Herrera <alvherre@2ndquadrant.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
whetherthere would be broad support for having yet another perl script generating source files, nor for the maintenance
burdenof having to do that on all platforms.  Having a perl script that merely sanity checks the source files has the
advantagethat there is no requirement for it to function on all platforms.  There’s not even a requirement for it to be
committedto the tree, since you could also argue that the maintenance burden of the script outweighs the burden of
gettingthe 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
thechange where the rowcount gets written into the commandTag string.  I have a work-in-progress patch to provide
systemviews to track the number of commands executed of each type, and that patch includes the ability to distinguish
betweenwhat 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
finishedas) and not the command (what it started as).  I rejected the “SetCompletionTag” naming because it’s not just
thetag that is being set, but both the tag and the row count.  I am happy with “SetQueryCompletion” because that naming
isconsistent 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
specificallydon’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