Documentation for bootstrap data conversion

Hi,

On 2018-04-06 14:27:39 -0400, Tom Lane wrote:

John and I are probably both too close to the patch to be able to
review this documentation for clarity and usefulness, so if anyone
else wants to have a look, please comment.

Quick skim only:

diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
index 33378b4..1e7915e 100644
--- a/doc/src/sgml/bki.sgml
+++ b/doc/src/sgml/bki.sgml
@@ -1,38 +1,641 @@
<!-- doc/src/sgml/bki.sgml -->

<chapter id="bki">
- <title><acronym>BKI</acronym> Backend Interface</title>
+ <title>System Catalog Declarations and Initial Contents</title>

<para>
-  Backend Interface (<acronym>BKI</acronym>) files are scripts in a
-  special language that is understood by the
-  <productname>PostgreSQL</productname> backend when running in the
-  <quote>bootstrap</quote> mode.  The bootstrap mode allows system catalogs
-  to be created and filled from scratch, whereas ordinary SQL commands
-  require the catalogs to exist already.
-  <acronym>BKI</acronym> files can therefore be used to create the
-  database system in the first place.  (And they are probably not
-  useful for anything else.)
+  <productname>PostgreSQL</productname> uses many different system catalogs
+  to keep track of the existence and properties of database objects, such as
+  tables and functions.  Physically there is no difference between a system
+  catalog and a plain user table, but the backend C code knows the structure
+  and properties of each catalog, and can manipulate it directly at a low
+  level.  Thus, for example, it is inadvisable to attempt to alter the
+  structure of a catalog on-the-fly; that would break assumptions built into
+  the C code about how rows of the catalog are laid out.  But developers
+  can change the structure of catalogs in new major versions.
</para>

"developers" here could possibly be understood to be any sort of
developer, rather than postgres ones. Perhaps just say "But the
structure of the catalogs can change between major versions."?

<para>
-  Related information can be found in the documentation for
-  <application>initdb</application>.
+  Many of the catalogs have initial data that must be loaded into them
+  during the <quote>bootstrap</quote> phase
+  of <application>initdb</application>, to bring the system up to a point
+  where it is capable of executing SQL commands.  (For
+  example, <filename>pg_class.h</filename> must contain an entry for itself,
+  as well as one for each other system catalog and index.)  Much of this
+  initial data is kept in editable form in data files that are also stored
+  in the <filename>src/include/catalog/</filename> directory.  For example,
+  <filename>pg_proc.dat</filename> describes all the initial rows that must
+  be inserted into the <structname>pg_proc</structname> catalog.
</para>

+ <para>
+  To create the catalog files and load this initial data into them, a
+  backend running in bootstrap mode reads a <acronym>BKI</acronym>
+  (Backend Interface) file containing commands and initial data.
+  The <filename>postgres.bki</filename> file used in this mode is prepared
+  from the aforementioned header and data files, by a Perl script
+  named <filename>genbki.pl</filename>, while building
+  a <productname>PostgreSQL</productname> distribution.
+  Although it's specific to a particular <productname>PostgreSQL</productname>
+  release, <filename>postgres.bki</filename> is platform-independent and is
+  normally installed in the <filename>share</filename> subdirectory of the
+  installation tree.
+ </para>

"normally"?

+ <sect1 id="system-catalog-declarations">
+  <title>System Catalog Declaration Rules</title>
+
+  <para>
+   The key part of a catalog header file is a C structure definition
+   describing the layout of each row of the catalog.  This begins with
+   a <literal>CATALOG</literal> macro, which so far as the C compiler is
+   concerned is just shorthand for <literal>typedef struct
+   FormData_<replaceable>catalogname</replaceable></literal>.
+   Each field in the struct gives rise to a catalog column.
+   Fields can be annotated using the BKI property macros described
+   in <filename>genbki.h</filename>, to define a default value, mark the
+   field as nullable or not nullable, or specify a lookup rule that allows
+   OID values to be represented symbolically in the
+   corresponding <filename>.dat</filename> file.

This sounds like an exhaustive list of what genbki.h allows - that seems
likely to get out of date...

+  <para>
+   The system catalog cache code (and most catalog-munging code in general)
+   assumes that the fixed-length portions of all system catalog tuples are
+   in fact present, because it maps this C struct declaration onto them.
+   Thus, all variable-length fields and nullable fields must be placed at
+   the end, and they cannot be accessed as struct fields.
+   For example, if you tried to
+   set <structname>pg_type</structname>.<structfield>typrelid</structfield>
+   to be NULL, it would fail when some piece of code tried to reference
+   <literal>typetup-&gt;typrelid</literal> (or worse,
+   <literal>typetup-&gt;typelem</literal>, because that follows
+   <structfield>typrelid</structfield>).  This would result in
+   random errors or even segmentation violations.
+  </para>
+
+  <para>
+   As a partial guard against this type of error, variable-length or
+   nullable fields should not be made directly visible to the C compiler.
+   This is accomplished by wrapping them in <literal>#ifdef
+   CATALOG_VARLEN</literal> ... <literal>#endif</literal>.  This prevents C
+   code from carelessly trying to dereference fields that might not be
+   there.

Not just, also using an entirely wrong offset, no?

As an independent guard against creating incorrect rows, we
+   require that all columns that should be non-nullable are marked so
+   in <structname>pg_attribute</structname>.  The bootstrap code will
+   automatically mark catalog columns as <literal>NOT NULL</literal>
+   if they are fixed-width and are not preceded by any nullable column.
+   Where this rule is inadequate, you can force correct marking by using
+   <literal>BKI_FORCE_NOT_NULL</literal>
+   and <literal>BKI_FORCE_NULL</literal> annotations as needed.  But note
+   that <literal>NOT NULL</literal> constraints are only enforced in the
+   executor, not against tuples that are generated by random C code,
+   so care is still needed when manually creating or updating catalog rows.
+  </para>

Hm. Not about docs, but I wonder if we should write a sanity check
regression test that makes sure that's not violated from somewhere?

+ <sect1 id="system-catalog-initial-data">
+  <title>System Catalog Initial Data</title>
+
+  <para>
+   Each catalog that has any manually-created initial data (some do not)
+   has a corresponding <literal>.dat</literal> file that contains its
+   initial data in an editable format.
+  </para>
+
+  <sect2 id="system-catalog-initial-data-format">
+   <title>Data File Format</title>
+
+   <para>
+    Each <literal>.dat</literal> file contains Perl data structure literals
+    that are simply eval'd to produce an in-memory data structure consisting
+    of an array of hash references, one per catalog row.
+    A slightly modified excerpt from <filename>pg_database.dat</filename>
+    will demonstrate the key features:
+   </para>
+
+<programlisting>
+[
+
+# LC_COLLATE and LC_CTYPE will be replaced at initdb time with user choices
+# that might contain non-word characters, so we must double-quote them.

Hm. Couldn't we get rid of that requirement and do the escaping in
genbki? Seems awkward, failure prone (people will forget and it'll often
work during development) and unnecessary in the new format.

+    <listitem>
+     <para>
+      Null values are represented by <literal>_null_</literal>.
+     </para>
+    </listitem>

wonder if it'd be more natural to use $null or such for this kind of thing.

+       <listitem>
+        <para>
+         If the catalog's <literal>.h</literal> file specifies a default
+         value for a column, and a data entry has that same
+         value, <filename>reformat_dat_file.pl</filename> will omit it from
+         the data file.  This keeps the data representation compact.
+        </para>
+       </listitem>

This'll be fun if we ever decide to change a default :)

+  <sect2 id="system-catalog-oid-assignment">
+   <title>OID Assignment</title>
+
+   <para>
+    A catalog row appearing in the initial data can be given a
+    manually-assigned OID by writing an <literal>oid
+    =&gt; <replaceable>nnnn</replaceable></literal> metadata field.
+    Furthermore, if an OID is assigned, a C macro for that OID can be
+    created by writing an <literal>oid_symbol
+    =&gt; <replaceable>name</replaceable></literal> metadata field.
+   </para>
+
+   <para>
+    Pre-loaded catalog rows must have preassigned OIDs if there are OID
+    references to them in other pre-loaded rows.

Why is that?

A preassigned OID is
+    also needed if the row's OID must be referenced from C code.
+    If neither case applies, the <literal>oid</literal> metadata field can
+    be omitted, in which case the bootstrap code assigns an OID
+    automatically, or leaves it zero in a catalog that has no OIDs.
+    In practice we usually preassign OIDs for all or none of the pre-loaded
+    rows in a given catalog, even if only some of them are actually
+    cross-referenced.
+   </para>

I think we'd reduce the pain of maintaining uncommitted patches across a
few CFs if this were a more relaxed rule.

+   <para>
+    To find an available OID for a new pre-loaded row, run the
+    script <filename>src/include/catalog/unused_oids</filename>.
+    It prints inclusive ranges of unused OIDs (e.g., the output
+    line <quote>45-900</quote> means OIDs 45 through 900 have not been
+    allocated yet).

Might be worthwhile to not (or fix) that one has to be inside the
src/include/catalog/ directory to run it?

This is quite the nice improvement!

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Quick skim only:

"developers" here could possibly be understood to be any sort of
developer, rather than postgres ones. Perhaps just say "But the
structure of the catalogs can change between major versions."?

OK.

This sounds like an exhaustive list of what genbki.h allows - that seems
likely to get out of date...

I'll put in "for example".

+# LC_COLLATE and LC_CTYPE will be replaced at initdb time with user choices
+# that might contain non-word characters, so we must double-quote them.

Hm. Couldn't we get rid of that requirement and do the escaping in
genbki? Seems awkward, failure prone (people will forget and it'll often
work during development) and unnecessary in the new format.

Well, only if you want genbki.pl to embed knowledge of what initdb will
substitute for, which seems a bit outside its charter.

+    <listitem>
+     <para>
+      Null values are represented by <literal>_null_</literal>.
+     </para>
+    </listitem>

wonder if it'd be more natural to use $null or such for this kind of thing.

Considering we're eval'ing these structs, I think that's just an
invitation to trouble :-)

+       <listitem>
+        <para>
+         If the catalog's <literal>.h</literal> file specifies a default
+         value for a column, and a data entry has that same
+         value, <filename>reformat_dat_file.pl</filename> will omit it from
+         the data file.  This keeps the data representation compact.
+        </para>
+       </listitem>

This'll be fun if we ever decide to change a default :)

Well, see recipe for how to do that, a bit further down.

+    Pre-loaded catalog rows must have preassigned OIDs if there are OID
+    references to them in other pre-loaded rows.

Why is that?

Uh, because otherwise we don't know what to put in?

Perhaps genbki could be made to do OID assignment, but that's not in its
charter right now, and I'm not sure it'd be an improvement. There would
still be OIDs that have to be assigned during the bootstrap run.

+    In practice we usually preassign OIDs for all or none of the pre-loaded
+    rows in a given catalog, even if only some of them are actually
+    cross-referenced.

I think we'd reduce the pain of maintaining uncommitted patches across a
few CFs if this were a more relaxed rule.

Meh ... I'm just documenting the existing state of affairs here. In any
case, I think once a function is committed it's a good thing if it has
a stable OID. There are probably people depending on that externally
for fastpath calls. (In fact, I wonder why we don't simplify libpq to
assume it knows the OIDs of loread et al, rather than painfully looking
them up. If they haven't changed since 1996, they aren't going to.)

Might be worthwhile to not (or fix) that one has to be inside the
src/include/catalog/ directory to run it?

Don't much care, but if you do, fix away.

regards, tom lane

On 4/7/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

John and I are probably both too close to the patch to be able to
review this documentation for clarity and usefulness, so if anyone
else wants to have a look, please comment.

No argument there, but I did want to note some minor details:

1.
<filename>reformat_dat_file.pl</filename> preserves blank lines
and comment lines as-is.

As it is now, it will actually collapse consecutive blank lines into
one. I maintain that was necessary during conversion to get some
semblance of consistency, but now it may not be a good idea to tie
developers' hands in surprising ways if they want double blank lines
in some places. It would be pretty easy to remove this behavior.
Apologies if it was not documented well enough.

2. I noticed the use of
<structname>pg_xxx.h</structname>
<structname>pg_xxx_d.h</structname>
where I would expect <filename>. Not sure if it matters.

3. It seems the preferred style is to refer to "bootstrap" relations
rather than "bootstrapped" relations. The attached patch makes code
comments more like the docs in this regard.

-John Naylor

John Naylor <jcnaylor@gmail.com> writes:

On 4/7/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

<filename>reformat_dat_file.pl</filename> preserves blank lines
and comment lines as-is.

As it is now, it will actually collapse consecutive blank lines into
one. I maintain that was necessary during conversion to get some
semblance of consistency, but now it may not be a good idea to tie
developers' hands in surprising ways if they want double blank lines
in some places. It would be pretty easy to remove this behavior.
Apologies if it was not documented well enough.

I did note that in some internal comments, but forgot it when writing
this. I agree that now that the conversion is done, it'd be better
to remove that special case. Would you send a patch for that?

2. I noticed the use of
<structname>pg_xxx.h</structname>
<structname>pg_xxx_d.h</structname>
where I would expect <filename>. Not sure if it matters.

Good point. It likely doesn't matter in terms of the output, but
considering I was going to the trouble of using those instead of just
<literal> everywhere, would be better to choose the right one.

3. It seems the preferred style is to refer to "bootstrap" relations
rather than "bootstrapped" relations. The attached patch makes code
comments more like the docs in this regard.

Meh, I think either is fine really. I do recall changing something
in bki.sgml that referred to both "bootstrap relations" and "bootstrap
catalogs" in practically the same sentence. I think that *is* confusing,
because it's not obvious whether relation and catalog are meant to be
interchangeable terms (and, in general, they aren't). If we wanted to
do anything here I'd be more interested in s/relation/catalog/g in this
usage.

regards, tom lane

On 4/9/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

As it is now, it will actually collapse consecutive blank lines into
one. I maintain that was necessary during conversion to get some
semblance of consistency, but now it may not be a good idea to tie
developers' hands in surprising ways if they want double blank lines
in some places. It would be pretty easy to remove this behavior.
Apologies if it was not documented well enough.

I did note that in some internal comments, but forgot it when writing
this. I agree that now that the conversion is done, it'd be better
to remove that special case. Would you send a patch for that?

Sure, attached.

-John Naylor

John Naylor <jcnaylor@gmail.com> writes:

On 4/9/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I did note that in some internal comments, but forgot it when writing
this. I agree that now that the conversion is done, it'd be better
to remove that special case. Would you send a patch for that?

Sure, attached.

Pushed, thanks. I fixed the sgml markup too.

regards, tom lane

On 4/9/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

3. It seems the preferred style is to refer to "bootstrap" relations
rather than "bootstrapped" relations. The attached patch makes code
comments more like the docs in this regard.

Meh, I think either is fine really. I do recall changing something
in bki.sgml that referred to both "bootstrap relations" and "bootstrap
catalogs" in practically the same sentence. I think that *is* confusing,
because it's not obvious whether relation and catalog are meant to be
interchangeable terms (and, in general, they aren't). If we wanted to
do anything here I'd be more interested in s/relation/catalog/g in this
usage.

I was curious, so did these searches

git grep 'bootstrap\S* relation'
git grep 'system .*relation'

and dug through a bit to find cases where 'catalog' is clearly a
better term. Most of these are in the pg_*.h/.dat file boilerplate
comments, which would be easy enough to change with a script. If we're
going to do that, the word order of this phrase now seems a bit
awkward:

definition of the system "access method" catalog (pg_am)

so we may as well go the extra step and do:

definition of the "access method" system catalog (pg_am)

Thoughts?

Beyond that, there are many cases where the language might be
debatable, or at least it's not obvious to the casual observer whether
it could also be referring to an index or toast table, so it's
probably best to leave well enough alone for most cases.

-John Naylor

John Naylor <jcnaylor@gmail.com> writes:

On 4/9/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Meh, I think either is fine really. I do recall changing something
in bki.sgml that referred to both "bootstrap relations" and "bootstrap
catalogs" in practically the same sentence. I think that *is* confusing,
because it's not obvious whether relation and catalog are meant to be
interchangeable terms (and, in general, they aren't). If we wanted to
do anything here I'd be more interested in s/relation/catalog/g in this
usage.

I was curious, so did these searches

git grep 'bootstrap\S* relation'
git grep 'system .*relation'

and dug through a bit to find cases where 'catalog' is clearly a
better term. Most of these are in the pg_*.h/.dat file boilerplate
comments, which would be easy enough to change with a script. If we're
going to do that, the word order of this phrase now seems a bit
awkward:
definition of the system "access method" catalog (pg_am)
so we may as well go the extra step and do:
definition of the "access method" system catalog (pg_am)

Thoughts?

Makes sense to me --- I suspect that the original phrasing was chosen
by somebody whose native language wasn't English.

Beyond that, there are many cases where the language might be
debatable, or at least it's not obvious to the casual observer whether
it could also be referring to an index or toast table, so it's
probably best to leave well enough alone for most cases.

Agreed, there's no need to go overboard here. But let's fix that
awkward construction in the catalog header files, as well as anyplace
else where it seems like a clear win. Will you send a patch?

regards, tom lane

On 4/18/18, Tom Lane <tgl@sss.pgh.pa.us> wrote:

John Naylor <jcnaylor@gmail.com> writes:

and dug through a bit to find cases where 'catalog' is clearly a
better term. Most of these are in the pg_*.h/.dat file boilerplate
comments, which would be easy enough to change with a script. If we're
going to do that, the word order of this phrase now seems a bit
awkward:
definition of the system "access method" catalog (pg_am)
so we may as well go the extra step and do:
definition of the "access method" system catalog (pg_am)

Thoughts?

Makes sense to me --- I suspect that the original phrasing was chosen
by somebody whose native language wasn't English.

Beyond that, there are many cases where the language might be
debatable, or at least it's not obvious to the casual observer whether
it could also be referring to an index or toast table, so it's
probably best to leave well enough alone for most cases.

Agreed, there's no need to go overboard here. But let's fix that
awkward construction in the catalog header files, as well as anyplace
else where it seems like a clear win. Will you send a patch?

I've attached a patch that mostly touches boilerplate comments in the
headers and data files. I couldn't resist editing some comments for
clarity and consistency.

Not in the patch, but I'll also mention this stanza in tablecmds.c
around line 4370, where the comment doesn't quite match the code and
error message:

/*
* We don't support rewriting of system catalogs; there are too
* many corner cases and too little benefit. In particular this
* is certainly not going to work for mapped catalogs.
*/
if (IsSystemRelation(OldHeap))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot rewrite system relation \"%s\"",
RelationGetRelationName(OldHeap))));

BTW, thanks committing the quoting imrovements.

-John Naylor

John Naylor <jcnaylor@gmail.com> writes:

I've attached a patch that mostly touches boilerplate comments in the
headers and data files. I couldn't resist editing some comments for
clarity and consistency.

Pushed, along with a little bit of extra tweaking to fix random
discrepancies in the header comments.

Not in the patch, but I'll also mention this stanza in tablecmds.c
around line 4370, where the comment doesn't quite match the code and
error message:

Yeah. That's a translatable error message though, so I'm hesitant
to mess with it just for neatnik-ism. Doubt it's worth the translators'
time to change it.

regards, tom lane