Re: allow segment size to be set to < 1GiB
Hi, On 2022-11-09 12:25:09 -0800, Andres Freund wrote: > Updated patch attached. I pushed it now. > I made one autoconf and one meson CI task use a small block size, but just to > ensure it work on both. I'd probably leave it set on one, so we keep the > coverage for cfbot? It doesn't seem to cost that much, so I left it set in those two tasks for now. Greetings, Andres Freund
Re: allow segment size to be set to < 1GiB
On 2022-11-17 Th 10:48, Tom Lane wrote: > Andres Freund writes: >> On 2022-11-17 09:58:48 -0500, Andrew Dunstan wrote: >>> Are we going to impose some sane minimum, or leave it up to developers >>> to discover that for themselves? >> I don't think we should. It's actually useful to e.g. use 1 page sized >> segments for testing, and with one exceptions the tests pass with it too. Do >> you see a reason to impose one? > Yeah, I think we should allow setting it to 1 block. This switch is > only for testing purposes (I hope the docs make that clear). > > Yeah clearly if 1 is useful there's no point in limiting it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: allow segment size to be set to < 1GiB
On 2022-11-17 10:48:52 -0500, Tom Lane wrote: > Yeah, I think we should allow setting it to 1 block. This switch is > only for testing purposes (I hope the docs make that clear). "This option is only for developers, to test segment related code."
Re: allow segment size to be set to < 1GiB
Andres Freund writes: > On 2022-11-17 09:58:48 -0500, Andrew Dunstan wrote: >> Are we going to impose some sane minimum, or leave it up to developers >> to discover that for themselves? > I don't think we should. It's actually useful to e.g. use 1 page sized > segments for testing, and with one exceptions the tests pass with it too. Do > you see a reason to impose one? Yeah, I think we should allow setting it to 1 block. This switch is only for testing purposes (I hope the docs make that clear). regards, tom lane
Re: allow segment size to be set to < 1GiB
Hi, On 2022-11-17 09:58:48 -0500, Andrew Dunstan wrote: > On 2022-11-09 We 15:25, Andres Freund wrote: > > Hi, > > > > On 2022-11-09 14:44:42 -0500, Tom Lane wrote: > >> Andres Freund writes: > >>> A second question: Both autoconf and meson print the segment size as GB > >>> right > >>> now. Obviously that'll print out a size of 0 for a segsize < 1GB. > >>> The easiest way to would be to just display the number of blocks, but > >>> that's > >>> not particularly nice. > >> Well, it would be fine if you'd written --with-segsize-blocks, wouldn't > >> it? Can we make the printout format depend on which switch was used? > > Not sure why I didn't think of that... > > > > Updated patch attached. > > > > I made one autoconf and one meson CI task use a small block size, but just > > to > > ensure it work on both. I'd probably leave it set on one, so we keep the > > coverage for cfbot? > > > > Are we going to impose some sane minimum, or leave it up to developers > to discover that for themselves? I don't think we should. It's actually useful to e.g. use 1 page sized segments for testing, and with one exceptions the tests pass with it too. Do you see a reason to impose one? Greetings, Andres Freund
Re: allow segment size to be set to < 1GiB
On 2022-11-09 We 15:25, Andres Freund wrote: > Hi, > > On 2022-11-09 14:44:42 -0500, Tom Lane wrote: >> Andres Freund writes: >>> A second question: Both autoconf and meson print the segment size as GB >>> right >>> now. Obviously that'll print out a size of 0 for a segsize < 1GB. >>> The easiest way to would be to just display the number of blocks, but that's >>> not particularly nice. >> Well, it would be fine if you'd written --with-segsize-blocks, wouldn't >> it? Can we make the printout format depend on which switch was used? > Not sure why I didn't think of that... > > Updated patch attached. > > I made one autoconf and one meson CI task use a small block size, but just to > ensure it work on both. I'd probably leave it set on one, so we keep the > coverage for cfbot? > Are we going to impose some sane minimum, or leave it up to developers to discover that for themselves? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: allow segment size to be set to < 1GiB
Hi,
On 2022-11-09 14:44:42 -0500, Tom Lane wrote:
> Andres Freund writes:
> > A second question: Both autoconf and meson print the segment size as GB
> > right
> > now. Obviously that'll print out a size of 0 for a segsize < 1GB.
>
> > The easiest way to would be to just display the number of blocks, but that's
> > not particularly nice.
>
> Well, it would be fine if you'd written --with-segsize-blocks, wouldn't
> it? Can we make the printout format depend on which switch was used?
Not sure why I didn't think of that...
Updated patch attached.
I made one autoconf and one meson CI task use a small block size, but just to
ensure it work on both. I'd probably leave it set on one, so we keep the
coverage for cfbot?
Greetings,
Andres Freund
>From 7a76be232dd0587e9c84af4e4481d5a98f94bd22 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Wed, 9 Nov 2022 12:01:54 -0800
Subject: [PATCH v2] Add option to specify segment size in blocks
The tests don't have much coverage of segment related code, as we don't create
large enough tables. To make it easier to test these paths, add a new option
specifying the segment size in blocks.
Set the new option to 6 blocks in one of the CI tasks. Smaller numbers
currently fail one of the tests, for understandable reasons.
While at it, fix some segment size related issues in the meson build.
Author: Andres Freund
Discussion: https://postgr.es/m/20221107171355.c23fzwanfzq2p...@awork3.anarazel.de
---
meson.build| 24 ++---
meson_options.txt | 3 ++
configure.ac | 36 ++-
doc/src/sgml/installation.sgml | 14
.cirrus.yml| 2 ++
configure | 63 --
6 files changed, 118 insertions(+), 24 deletions(-)
diff --git a/meson.build b/meson.build
index ce2f223a409..b0fcf82a9c2 100644
--- a/meson.build
+++ b/meson.build
@@ -418,7 +418,19 @@ meson_bin = find_program(meson_binpath, native: true)
cdata.set('USE_ASSERT_CHECKING', get_option('cassert') ? 1 : false)
-cdata.set('BLCKSZ', get_option('blocksize').to_int() * 1024, description:
+blocksize = get_option('blocksize').to_int() * 1024
+
+if get_option('segsize_blocks') != 0
+ if get_option('segsize') != 1
+warning('both segsize and segsize_blocks specified, segsize_blocks wins')
+ endif
+
+ segsize = get_option('segsize_blocks')
+else
+ segsize = (get_option('segsize') * 1024 * 1024 * 1024) / blocksize
+endif
+
+cdata.set('BLCKSZ', blocksize, description:
'''Size of a disk block --- this also limits the size of a tuple. You can set
it bigger if you need bigger tuples (although TOAST should reduce the need
to have large tuples, since fields can be spread across multiple tuples).
@@ -428,7 +440,7 @@ cdata.set('BLCKSZ', get_option('blocksize').to_int() * 1024, description:
Changing BLCKSZ requires an initdb.''')
cdata.set('XLOG_BLCKSZ', get_option('wal_blocksize').to_int() * 1024)
-cdata.set('RELSEG_SIZE', get_option('segsize') * 131072)
+cdata.set('RELSEG_SIZE', segsize)
cdata.set('DEF_PGPORT', get_option('pgport'))
cdata.set_quoted('DEF_PGPORT_STR', get_option('pgport').to_string())
cdata.set_quoted('PG_KRB_SRVNAM', get_option('krb_srvnam'))
@@ -3053,9 +3065,11 @@ if meson.version().version_compare('>=0.57')
summary(
{
- 'data block size': cdata.get('BLCKSZ'),
- 'WAL block size': cdata.get('XLOG_BLCKSZ') / 1024,
- 'segment size': cdata.get('RELSEG_SIZE') / 131072,
+ 'data block size': '@0@ kB'.format(cdata.get('BLCKSZ') / 1024),
+ 'WAL block size': '@0@ kB'.format(cdata.get('XLOG_BLCKSZ') / 1024),
+ 'segment size': get_option('segsize_blocks') != 0 ?
+'@0@ blocks'.format(cdata.get('RELSEG_SIZE')) :
+'@0@ GB'.format(get_option('segsize')),
},
section: 'Data layout',
)
diff --git a/meson_options.txt b/meson_options.txt
index c7ea57994dc..0d2a34fd154 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -13,6 +13,9 @@ option('wal_blocksize', type : 'combo',
option('segsize', type : 'integer', value : 1,
description : '''Segment size, in gigabytes''')
+option('segsize_blocks', type : 'integer', value: 0,
+ description : '''Segment size, in blocks''')
+
# Miscellaneous options
diff --git a/configure.ac b/configure.ac
index f76b7ee31fc..94542e862cf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -285,15 +285,31 @@ AC_DEFINE_UNQUOTED([BLCKSZ], ${BLCKSZ}, [
#
# Relation segment size
#
-AC_MSG_CHECKING([for segment size])
PGAC_ARG_REQ(with, segsize, [SEGSIZE], [set table segment size in GB [1]],
[segsize=$withval],
[segsize=1])
-# this expression is set up to avoid unnecessary integer overflow
-# blocksize is already guaranteed to be a factor of 1024
-RELSEG_SIZE=`expr '(' 1024 / ${blocksize} ')' '*' ${segsize} '*' 1024`
-test $? -eq 0 || exit 1
-AC_MSG_RESULT([${segsize}GB])
+PGAC_ARG_REQ(with, segsize-blocks, [SEGSIZE_BLOCKS], [set
Re: allow segment size to be set to < 1GiB
Andres Freund writes: > A second question: Both autoconf and meson print the segment size as GB right > now. Obviously that'll print out a size of 0 for a segsize < 1GB. > The easiest way to would be to just display the number of blocks, but that's > not particularly nice. Well, it would be fine if you'd written --with-segsize-blocks, wouldn't it? Can we make the printout format depend on which switch was used? regards, tom lane
Re: allow segment size to be set to < 1GiB
On 2022-11-08 18:28:08 -0800, Andres Freund wrote: > On 2022-11-07 21:36:33 -0500, Tom Lane wrote: > > Andres Freund writes: > > > On 2022-11-07 12:52:25 -0500, Tom Lane wrote: > > >> How about instead allowing the segment size to be set in pages? > > > > > In addition or instead of --with-segsize/-Dsegsize? > > > > In addition to. What I meant by "instead" was to replace > > your proposal of --with-segsize-mb. > > Working on updating the patch. > > One semi-interesting bit is that <= 5 blocks per segment fails, because > corrupt_page_checksum() doesn't know about segments and > src/bin/pg_basebackup/t/010_pg_basebackup.pl does A second question: Both autoconf and meson print the segment size as GB right now. Obviously that'll print out a size of 0 for a segsize < 1GB. The easiest way to would be to just display the number of blocks, but that's not particularly nice. We could show kB, but that ends up being large. Or we can have some code to adjust the unit, but that seems a bit overkill. Opinions?
Re: allow segment size to be set to < 1GiB
On Tue, Nov 08, 2022 at 06:28:08PM -0800, Andres Freund wrote: > FWIW, with HEAD, all tests pass with -Dsegsize_blocks=6 on HEAD. Wow. The relation page size influences some of the plans in the main regression test suite, but this is nice to hear. +1 from me for more flexibility with this option at compile-time. -- Michael signature.asc Description: PGP signature
Re: allow segment size to be set to < 1GiB
Hi,
On 2022-11-07 21:36:33 -0500, Tom Lane wrote:
> Andres Freund writes:
> > On 2022-11-07 12:52:25 -0500, Tom Lane wrote:
> >> How about instead allowing the segment size to be set in pages?
>
> > In addition or instead of --with-segsize/-Dsegsize?
>
> In addition to. What I meant by "instead" was to replace
> your proposal of --with-segsize-mb.
Working on updating the patch.
One semi-interesting bit is that <= 5 blocks per segment fails, because
corrupt_page_checksum() doesn't know about segments and
src/bin/pg_basebackup/t/010_pg_basebackup.pl does
# induce further corruption in 5 more blocks
$node->stop;
for my $i (1 .. 5)
{
$node->corrupt_page_checksum($file_corrupt1, $i * $block_size);
}
$node->start;
I'd be content with not dealing with that given the use case of the
functionality? A buildfarm animal setting it to 10 seem to
suffice. Alternatively we could add segment support to
corrupt_page_checksum().
Opinions?
FWIW, with HEAD, all tests pass with -Dsegsize_blocks=6 on HEAD.
Greetings,
Andres Freund
Re: allow segment size to be set to < 1GiB
On Tue, Nov 8, 2022 at 8:06 AM Tom Lane wrote: > > Andres Freund writes: > > On 2022-11-07 12:52:25 -0500, Tom Lane wrote: > >> How about instead allowing the segment size to be set in pages? > > > In addition or instead of --with-segsize/-Dsegsize? > > In addition to. What I meant by "instead" was to replace > your proposal of --with-segsize-mb. > > > Just offering the number of pages seems like a not great UI. > > Well, it's a developer/debug focused API. I think regular users > would only care for the existing --with-segsize = so-many-GB API. > But for testing, I think --with-segsize-pages = so-many-pages > is actually a pretty good UI. Perhaps --with-segsize-blocks is a better name here as we use block instead of page for --with-blocksize and --with-wal-blocksize. If this option is for dev/debug purposes only, do we want to put a mechanism to disallow it in release builds or something like that, just in case? Or at least, add a note in the documentation? -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: allow segment size to be set to < 1GiB
Andres Freund writes: > On 2022-11-07 12:52:25 -0500, Tom Lane wrote: >> How about instead allowing the segment size to be set in pages? > In addition or instead of --with-segsize/-Dsegsize? In addition to. What I meant by "instead" was to replace your proposal of --with-segsize-mb. > Just offering the number of pages seems like a not great UI. Well, it's a developer/debug focused API. I think regular users would only care for the existing --with-segsize = so-many-GB API. But for testing, I think --with-segsize-pages = so-many-pages is actually a pretty good UI. regards, tom lane
Re: allow segment size to be set to < 1GiB
Hi, On 2022-11-07 12:52:25 -0500, Tom Lane wrote: > Andres Freund writes: > > In the attached patch I renamed --with-segsize= to --with-segsize-mb= / > > -Dsegsize= to -Dsegsize_mb=, to avoid somebody building with > > --with-segsize=2 > > or such suddenly ending up with an incompatible build. > > For the purpose of exercising these code paths with the standard > regression tests, even a megabyte seems large -- we don't create > very many test tables that are that big. Good point. > How about instead allowing the segment size to be set in pages? In addition or instead of --with-segsize/-Dsegsize? Just offering the number of pages seems like a not great UI. I guess we could add support for units or such? But that seems messy as well. Greetings, Andres Freund
Re: allow segment size to be set to < 1GiB
Andres Freund writes: > In the attached patch I renamed --with-segsize= to --with-segsize-mb= / > -Dsegsize= to -Dsegsize_mb=, to avoid somebody building with --with-segsize=2 > or such suddenly ending up with an incompatible build. For the purpose of exercising these code paths with the standard regression tests, even a megabyte seems large -- we don't create very many test tables that are that big. How about instead allowing the segment size to be set in pages? regards, tom lane
