Re: GenBKI emits useless open;close for catalogs without rows

On Tue, 12 Sept 2023 at 17:51, Matthias van de Meent
<boekewurm+postgres(at)gmail(dot)com> wrote:
>
> On Fri, 1 Sept 2023 at 19:52, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> > > On 2023-Sep-01, Matthias van de Meent wrote:
> > >> A potential addition to the patch would to stop manually closing
> > >> relations: initdb and check-world succeed without manual 'close'
> > >> operations because the 'open' command auto-closes the previous open
> > >> relation (in boot_openrel). Testing also suggests that the last opened
> > >> relation apparently doesn't need closing - check-world succeeds
> > >> without issues (incl. with TAP enabled). That is therefore implemented
> > >> in attached patch 2 - it removes the 'close' syntax in its entirety.
> >
> > > Hmm, what happens with the last relation in the bootstrap process? Is
> > > closerel() called via some other path for that one?
> >
> > Taking a quick census of existing closerel() callers: there is
> > cleanup() in bootstrap.c, but it's called uncomfortably late
> > and outside any transaction, so I misdoubt that it works
> > properly if asked to actually shoulder any responsibility.
> > (A little code reshuffling could fix that.)
> > There are also a couple of low-level elog warnings in CREATE
> > that would likely get triggered, though I suppose we could just
> > remove those elogs.
>
> Yes, that should be easy to fix.
>
> > I guess my reaction to this patch is "why bother?". It seems
> > unlikely to yield any measurable benefit, though of course
> > that guess could be wrong.
>
> There is a small but measurable decrease in size of the generated bki
> (2kb with both patches, on an initial 945kB), and there is some
> related code that can be eliminated. If that's not worth bothering,
> then I can drop the patch. Otherwise, I can update the patch to do the
> cleanup that was within the transaction boundaries at the end of
> boot_yyparse.
>
> If decreasing the size of postgres.bki is not worth the effort, I'll
> drop any effort on doing so, but considering that it is about 1MB of
> our uncompressed distributables, I'd say decreases in size are worth
> the effort, most of the time.

With the attached patch I've see a significant decrease in the size of
postgres.bki of about 25%, and a likely related decrease in wall clock
time spent in the bootstrap transaction: with timestamp logs inserted
around the boot_yyparse() transaction the measured time went from
around 49 ms on master to around 45 ms patched. In the grand scheme of
initdb that might not be a lot of time (initdb takes about 73ms
locally with syncing disabled) but it is a nice gain in performance.

Comparison:

master @ 9c13b681
$ du -b pg_install/share/postgres.bki
945220
$ initdb --no-instructions --auth=md5 --pwfile pwfile -N -D ~/test-dbinit/
[...]
2023-09-16 02:22:57.339 CEST [10422] LOG: Finished bootstrapping:
to_start: 10 ms, transaction: 49 ms, finishing: 1 ms, total: 59 ms
[...]

patched
$ du -b pg_install/share/postgres.bki
702574
$ initdb --no-instructions --auth=md5 --pwfile pwfile -N -D ~/test-dbinit/
[...]
2023-09-16 02:25:57.664 CEST [15645] LOG: Finished bootstrapping:
to_start: 10 ms, transaction: 45 ms, finishing: 1 ms, total: 54 ms
[...]

Various methods of reducing the size of postgres.bki were applied, as
detailed in the patch's commit message. I believe the current output
is still quite human readable.

There are other potential avenues for further reducing the bki size,
e.g. through using smaller generated OIDs (reducing the number of
characters used per OID), applying RLE on sequential NULLs (there are
3k+ occurances of /( __){2,10}/ in the generated bki file remaining),
and other tricks, but several of those are likely to be detrimental to
the readability and manual verifiability of the bki.

Kind regards,

Matthias van de Meent