Re: Portal->commandTag as an enum

On Thu, Feb 20, 2020 at 5:16 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
> > On Feb 19, 2020, at 3:31 AM, John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> wrote:
> > thought it through. For one advantage, I think it might be nicer to
> > have indexing.dat and toasting.dat instead of having to dig the info
> > out of C macros. This was evident while recently experimenting with
> > generating catcache control data.
>
> I guess you mean macros DECLARE_UNIQUE_INDEX and DECLARE_TOAST. I don’t mind converting that to .dat files, though I’m mindful of Tom’s concern expressed early in this thread about the amount of code churn. Is there sufficient demand for refactoring this stuff? There are more reasons in the conversation below to refactor the perl modules and code generation scripts.

Yes, I was referring to those macros, but I did not mean to imply you
should convert them, either now or ever. I was just thinking out loud
about the possibility. :-)

That said, if we ever want Catalog.pm to delegate vivification to
DataFile.pm, there will eventually need to be a way to optionally
preserve comments and whitespace.

> I have taken all this advice in v5 of the patch. --inputfile and --outputdir (previously named --sourcedir) are now optional with the defaults as you suggested.

+my $inputfile = '../../include/utils/commandtag.dat';

I think we should skip the default for the input file, since the
relative path is fragile and every such script I've seen requires it
to be passed in.

> > DataFiles.pm
> > [...]
> > I'm not familiar with using different IO routines depending on the OS
> > -- what's the benefit of that?
>
> I think you are talking about the slurp_file routine. That came directly from the TestLib.pm module. I don't have enough perl-on-windows experience to comment about why it does things that way.

Yes, that one, sorry I wasn't explicit.

> Should I submit a separate patch refactoring the location of perl modules and functions of general interest into src/tools? src/tools/perl?

We may have accumulated enough disparate functionality by now to
consider this, but it sounds like PG14 material in any case.

> I expect there will have to be a v6 once this has been adequately debated.

So far I haven't heard any justification for why it should remain in
src/backend/catalog, when it has nothing to do with catalogs. We don't
have to have a standard other-place for there to be a better
other-place.

> > --
> > gencommandtag.pl

> > sanity_check_data() seems longer than the main body of the script
> > (minus header boilerplate), and I wonder if we can pare it down some.
> > For one, I have difficulty imagining anyone would accidentally type an
> > unprintable or non-ascii character in a command tag and somehow not
> > realize it.
> > [...]
> > For another, duplicating checks that were done earlier
> > seems like a maintenance headache.
>
> [ detailed rebuttals about the above points ]

Those were just examples that stuck out at me, so even if I were
convinced to your side on those, my broader point was: the sanity
check seems way over-engineered for something that spits out an enum
and an array. Maybe I'm not imaginative enough. I found it hard to
read in any case.

> Catalog.pm writes a temporary file and then moves it to the final file name at the end. DataFile buffers the output and only writes it after all the code generation has succeeded. There is no principled basis for these two modules tackling the same problem in two different ways.

Not the same problem. The temp files were originally for parallel Make
hazards, and now to prevent large portions of the backend to be
rebuilt. I actually think partially written files can be helpful for
debugging, so I don't even think it's a problem. But as I said, it
doesn't matter terribly much either way.

> > Speaking of boilerplate, it's better for readability to separate that
> > from actual code such as:
> >
> > typedef enum CommandTag
> > {
> > #define FIRST_COMMANDTAG COMMANDTAG_$sorted[0]->{taglabel})
>
> Good idea. While I was doing this, I also consolidated the duplicated boilerplate into just one function. I think this function, too, should go in just one perl module somewhere. See boilerplate_header() for details.

I like this a lot.

While thinking, I wonder if it makes sense to have a CodeGen module,
which would contain e.g. the new ParseData function,
FindDefinedSymbol, and functions for writing boilerplate.

--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services