MIME-Version: 1.0
References: <20230901174330.6jbe4izkew4nlt7k@alvherre.pgsql> <564720.1693590749@sss.pgh.pa.us>
In-Reply-To: <564720.1693590749@sss.pgh.pa.us>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Tue, 12 Sep 2023 17:51:30 +0200
Message-ID: <CAEze2Wi2Xgf9u+oLo3zgOZ-C=VsgpqQ-cXTNRKwSCXTG+y2Rqg@mail.gmail.com>
Subject: Re: GenBKI emits useless open;close for catalogs without rows
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAEze2Wi2Xgf9u%2BoLo3zgOZ-C%3DVsgpqQ-cXTNRKwSCXTG%2By2Rqg%40mail.gmail.com>
Precedence: bulk

On Fri, 1 Sept 2023 at 19:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.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.

Kind regards,

Matthias van de Meent