Re: WIP: a way forward on bootstrap data

Thanks for taking a look.

On 3/3/18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> John Naylor <jcnaylor(at)gmail(dot)com> writes:
>> Version 8, rebased against 76b6aa41f41d.
>
> I took a preliminary look through this, without yet attempting to execute
> the script against HEAD. I have a few thoughts:
>
> * I'm inclined not to commit the conversion scripts to the repo. I doubt
> there are third parties out there with a need for them, and if they do
> need them they can get 'em out of this thread in the mailing list
> archives. (If anyone has a different opinion about that, speak up!)

If no one feels strongly otherwise, I'll just attach the conversion
script along with the other patches for next version. To be clear, the
rewrite script is intended be committed, both to enforce formatting
and as a springboard for bulk editing. Now, whether that belongs in
/src/include/catalog or /src/tools is debatable.

> * I'm a little disturbed by the fact that 0002 has to "re-doublequote
> values that are macros expanded by initdb.c". I see that there are only
> a small number of affected places, so maybe it's not really worth working
> harder, but I worry that something might get missed. Is there any way to
> include this consideration in the automated conversion, or at least to
> verify that we found all the places to quote? Or, seeing that 0004 seems
> to be introducing some quoting-related hacks to genbki.pl anyway, maybe
> we could take care of the issue there?

The quoting hacks are really to keep the postgres.bki diff as small as
possible (attached). The simplest and most air-tight way to address
your concern would be to double-quote everything when writing the bki
file. That could be done last as a follow-up.

> * I notice you have a few "preliminary cleanup" changes here and there
> in the series, such as fixing the inconsistent spelling of
> Anum_pg_init_privs_initprivs. These could be applied before we start
> the main conversion process, and I'm inclined to do that since we could
> get 'em out of the way early. Ideally, the main conversion would only
> touch the header files and related scripts/makefiles.
...
> * In 0003, I'd recommend leaving the re-indentation to happen in the next
> perltidy run (assuming perltidy would fix that, which I think is true but
> I might be wrong). It's just creating more review work to do it here.
> In any case, the patch summary line is pretty misleading since it's
> *not* just reindenting, but also refactoring genbki.pl. (BTW, if that
> refactoring would work on the script as it is, maybe that's another
> thing we could do early? The more we can do before "flag day", the
> better IMO.)

I tested perltidy 20090616 and it handles it fine. I'll submit a
preliminary patch soon to get some of those items out of the way.

> * In 0006, I'm not very pleased with the introduction of
> "Makefile.headers". I'd keep those macros where they are in
> catalog/Makefile. backend/Makefile doesn't need to know about that,
> especially since it's doing an unconditional invocation of
> catalog/Makefile anyway. It could just do something like
>
> submake-schemapg:
> $(MAKE) -C catalog generated-headers
>
> and leave it to catalog/Makefile to know what needs to happen for
> both schemapg.h and the other generated files.

I wasn't happy with it either, but I couldn't get it to build
otherwise. The sticking point was the symlinks in
$(builddir)/src/include/catalog. $(MAKE) -C catalog doesn't handle
that. The makefile in /src/common relies on the backend makefile to
know what to invoke for a given header. IIRC, relpath.c includes
pg_tablespace.h, which now requires pg_tablespace_d.h to be built.

Perhaps /src/common/Makefile could invoke the catalog makefile
directly, and the pg_*_d.h headers could be written to
$(builddir)/src/include/catalog directly? I'll hack on it some more.

> Overall, though, this is looking pretty promising.
>
> regards, tom lane
>

Glad to hear it.

-John Naylor