Re: Portal->commandTag as an enum

> On Feb 11, 2020, at 1:05 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>> On Feb 11, 2020, at 1:02 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> On 2020-Feb-11, Mark Dilger wrote:
>>
>>>> No thanks.
>>>
>>> I’m not sure which option you are voting for:
>>>
>>> (Option #1) Have the perl script generate the .c and .h file from a .dat file
>>>
>>> (Option #2) Have the perl script validate but not generate the .c and .h files
>>>
>>> (Option #3) Have no perl script, with all burden on the programmer to get the .c and .h files right by hand.
>>>
>>> I think you’re voting against #3, and I’m guessing you’re voting for #1, but I’m not certain.
>>
>> I was voting against #2 (burden the programmer with consistency checks
>> that must be fixed by hand, without actually doing the programmatically-
>> doable work), but I don't like #3 either.  I do like #1.
>
> Option #1 works for me.  If I don’t see any contrary votes before I get back to this patch, I’ll implement it that
wayfor the next version. 

While investigating how best to implement option #1, I took a look at how Catalog.pm does it.

Catalog.pm reads data files and eval()s chunks of them to vivify perl data.

                # We're treating the input line as a piece of Perl, so we
                # need to use string eval here. Tell perlcritic we know what
                # we're doing.
                eval '$hash_ref = ' . $_;   ## no critic (ProhibitStringyEval)

This would only make sense to me if the string held in $_ had already been checked for safety, but Catalog.pm does very
littleto verify that the string is safe to eval.  The assumption here, so far as I can infer, is that we don’t embed
anythingdangerous in our .dat files, so this should be ok.  That may be true for the moment, but I can imagine a day
whenwe start embedding perl functions as quoted text inside a data file, or shell commands as text which look enough
likeperl for eval() to be able to execute them.  Developers who edit these .dat files and mess up the quoting, and then
rerun‘make’ to get the new .c and .h files generated, may not like the side effects.  Perhaps I’m being overly
paranoid…. 

Rather than add more code generation logic based on the design of Catalog.pm, I wrote a perl based data file parser
thatparses .dat files and returns vivified perl data, as Catalog.pm does, but with much stricter parsing logic to make
certainnothing dangerous gets eval()ed.  I put the new module in DataFile.pm.  The commandtag data has been
consolidatedinto a single .dat file.  A new perl script, gencommandtag.pl, parses the commandtag.dat file and generates
the.c and .h files.  So far, only gencommandtag.pl uses DataFile.pm, but I’ve checked that it can parse all the .dat
filescurrently in the source tree. 

The new parser is more flexible about the structure of the data, which seems good to me for making it easier to add or
modifydata files in the future.  The new parser does not yet have a means of hacking up the data to add autogenerated
fieldsand such that Catalog.pm does, but I think a more clean break between parsing and autovivifying fields would be
goodanyway.  If I get generally favorable reviews of DataFile.pm, I might go refactor Catalog.pm.  For now, I’m just
leavingCatalog.pm alone.  

> 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 still have the “Martian syntax”, though now it is generated by the perl script.  I can get rid of it, but I think
Andresliked the Martian stuff. 

> 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 renamed QueryCompletionData to just QueryCompletion, and I’m passing pointers to that.



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