Re: WIP: a way forward on bootstrap data

On 3/21/18, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> John Naylor <jcnaylor(at)gmail(dot)com> writes:
>> [ v11-bootstrap-data-conversion.tar.gz ]
>
> I've done a round of review work on this, focusing on the Makefile
> infrastructure. I found a bunch of problems with parallel builds and
> VPATH builds, which are addressed in the attached incremental patch.
>
[explanation of Make issues and stamp files]

> In the attached, I've also done some more work on the README file
> and cleaned up a few other little things.

Thanks for pulling my attempt at makefile hackery across the finish
line. It sounds like there are no more obvious structural issues
remaining (fingers crossed). Your other improvements make sense.

> I did note a bit of distressing inconsistency in the formatting of
> the catalog struct declarations, some of which predates this patch but it
> seems like you've introduced more. I think what we ought to standardize
> on is a format similar to this in pg_opclass.h:
>
> CATALOG(pg_opclass,2616)
> {
> /* index access method opclass is for */
> Oid opcmethod BKI_LOOKUP(pg_am);
>
[snip]

That is the most sensible format. Did you mean all 62 catalog headers
for future-proofing, or just the ones with annotations now?

> The former convention used in some places, of field descriptions in
> same-line comments, clearly won't work anymore if we're sticking
> BKI_DEFAULT annotations there.

Yeah.

> I also don't like the format used in, eg,
> pg_aggregate.h of putting field descriptions in a separate comment block
> before the struct proper. Bitter experience has shown that there are a
> lot of people on this project who won't update comments that are more than
> about two lines away from the code they change; so the style in
> pg_aggregate.h is just inviting maintenance oversights.

Okay.

> I've got mixed feelings about the whitespace lines between fields. They
> seem like they are mostly bulking up the code and we could do without 'em.
> On the other hand, pgindent will insist on putting one before any
> multi-line field comment, and so that would create inconsistent formatting
> if we don't use 'em elsewhere. Thoughts?

I'll do it both ways for one header and post the results for people to look at.

> Speaking of pgindent, those prettily aligned BKI annotations are a waste
> of effort, because when pgindent gets done with the code it will look
> like
>
[snip]
> regproc aggmtransfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
> regproc aggminvtransfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
> regproc aggmfinalfn BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);

> I'm not sure if there's anything much to be done about this. BKI_DEFAULT
> isn't so bad, but additional annotations get unreadable fast. Maybe
> BKI_LOOKUP was a bad idea after all, and we should just invent more
> Oid-equivalent typedef names.

Well, until version 7, I used "fake" type aliases that only genbki.pl
could see. The C compiler and postgres.bki could only see the actual
Oid/oid type. Perhaps it was a mistake to model their appearance after
regproc (regtype, etc), because that was misleading. Maybe something
more obviously transient like 'lookup_typeoid? I'm leaning towards
this idea.

Another possibility is to teach the pgindent pre_/post_indent
functions to preserve annotation formatting, but I'd rather not add
yet another regex to that script. Plus, over the next 10+ years, I
could see people adding several more BKI_* macros, leading to
readability issues regardless of formatting, so maybe we should nip
this one in the bud.

> The attached is just one incremental patch on top of your v11 series.
> I couldn't think of an easy way to migrate the changes back into the
> most relevant diffs of your series, so I didn't try.

I've done that quite a few times while developing this patch series,
so I'm used to it. I'll incorporate your changes soon and also rebase
over the new pg_class column that landed recently. I'll have a new
version by this weekend, assuming we conclude the formatting
discussion, so if you or others have any more comments by then, I'll
include them.

-John Naylor