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.
I think is a little confused about what is source and what is generated.
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.
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. There seems to be a lot of
effort in separating QueryCompletion from CommandTag; is that really
necessary?) 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.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
