SLRU_PAGES_PER_SEGMENT as configure parameter

On 6 Feb 2025, at 11:53, Daniil Davydov <3danissimo@gmail.com> wrote:

The constant SLRU_PAGES_PER_SEGMENT defines the size of the SLRU
segments. It is currently hardcoded in slru.h. It would be nice to be
able to set this parameter during configuration (for example, by
analogy with --with-segsize-blocks).

Not commenting on the gist of the patch, but the mechanics:

configure | 52 ++++++++

You should include the configure.ac changes and not just the generated code in
configure (which is fine to include for review, but the committer will
re-generate it regardless). Please also include the corresponding Meson change
to keep the buildsystems in sync.

--
Daniel Gustafsson

On Thu, Feb 6, 2025 at 8:23 PM Daniel Gustafsson <daniel@yesql.se> wrote:

You should include the configure.ac changes and not just the generated code in
configure (which is fine to include for review, but the committer will
re-generate it regardless). Please also include the corresponding Meson change
to keep the buildsystems in sync.

Thank you for your comment. I'm applying a patch that also includes
configure.ac, meson_options.txt and meson.build changes.
It would also be interesting to know what you think about the idea of
allowing such a parameter to be configured.

--
Best regards,
Daniil Davydov

On 7 Feb 2025, at 06:54, Daniil Davydov <3danissimo@gmail.com> wrote:

On Thu, Feb 6, 2025 at 8:23 PM Daniel Gustafsson <daniel@yesql.se> wrote:

You should include the configure.ac changes and not just the generated code in
configure (which is fine to include for review, but the committer will
re-generate it regardless). Please also include the corresponding Meson change
to keep the buildsystems in sync.

Thank you for your comment. I'm applying a patch that also includes
configure.ac, meson_options.txt and meson.build changes.

+AC_DEFINE_UNQUOTED([SLRU_PAGES_PER_SEGMENT], ${slru_pages_per_segment}, [
+  SLRU segment size in pages. A page is the same BLCKSZ as is used everywhere
+	else in Postgres.  The segment size can be chosen somewhat arbitrarily;
+       ...
It's not per project style to add large paragraphs like that to autoconf or
Meson, which segways nicely into commenting that the GUC and the build-time
parameters are missing from the documentation.  Something like the above
message should be in the documentation rather than the build infrastructure.

- elog(NOTICE, "Called SlruSyncFileTag() for segment %lld on path %s",
- (long long) ftag.segno, path);
-
...
- elog(NOTICE, "Called SlruDeleteSegment() for segment %lld",
- (long long) ftag.segno);
-
What's the motivation for these changes?

It would also be interesting to know what you think about the idea of
allowing such a parameter to be configured.

It could perhaps be useful for (mostly) testing purposes, but since it requires
recompilation why not just change SLRU_PAGES_PER_SEGMENT in slru.h and
recompile? That would save a lot of code while achieving the same thing no?

If you foresee a performance enchancement in production usecases at certaint
values I recommend doing benchmarking to show this.

--
Daniel Gustafsson