Thread: allow segment size to be set to < 1GiB
Hi, I was recently reminded of my previous desire to allow setting the segment size to less than 1GB. It's pretty painful to test large amount of segments with a segment size of 1GB, certainly our regression test don't cover anything with multiple segments. This likely wouldn't have detected the issue fixed in 0e758ae89a2, but it make it easier to validate that the fix doesn't break anything badly. 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. Greetings, Andres Freund
Attachment
Andres Freund <andres@anarazel.de> 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
Hi, On 2022-11-07 12:52:25 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> 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
On Tue, Nov 8, 2022 at 8:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andres Freund <andres@anarazel.de> 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
Hi,
On 2022-11-07 21:36:33 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> 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
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
Attachment
On 2022-11-08 18:28:08 -0800, Andres Freund wrote: > On 2022-11-07 21:36:33 -0500, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> 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?
Andres Freund <andres@anarazel.de> 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
Hi, On 2022-11-09 14:44:42 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
Attachment
On 2022-11-09 We 15:25, Andres Freund wrote: > Hi, > > On 2022-11-09 14:44:42 -0500, Tom Lane wrote: >> Andres Freund <andres@anarazel.de> 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
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 <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> 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
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."
On 2022-11-17 Th 10:48, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
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