Re: Portal->commandTag as an enum
От | John Naylor |
---|---|
Тема | Re: Portal->commandTag as an enum |
Дата | |
Msg-id | CACPNZCs=WzuC_j0UsuEqzhdaHJtdRRq_UwQ-zUK2vZnv+DEwEQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Portal->commandTag as an enum (Mark Dilger <mark.dilger@enterprisedb.com>) |
Ответы |
Re: Portal->commandTag as an enum
|
Список | pgsql-hackers |
On Thu, Feb 20, 2020 at 5:16 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > On Feb 19, 2020, at 3:31 AM, John Naylor <john.naylor@2ndquadrant.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’mmindful of Tom’s concern expressed early in this thread about the amount of code churn. Is there sufficient demand forrefactoring this stuff? There are more reasons in the conversation below to refactor the perl modules and code generationscripts. 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 optionalwith 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 enoughperl-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 andonly writes it after all the code generation has succeeded. There is no principled basis for these two modules tacklingthe 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 thisfunction, 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
В списке pgsql-hackers по дате отправления: