Re: Large files for relations - Mailing list pgsql-hackers

On 5/28/23 08:48, Thomas Munro wrote:
> 
> Alright, since I had some time to kill in an airport, here is a
> starter patch for initdb --rel-segsize.  

I've gone through this patch and it looks pretty good to me. A few things:

+             * rel_setment_size, we will truncate the K+1st segment to 0 length

rel_setment_size -> rel_segment_size

+     * We used a phony GUC with a custome show function, because we don't

custome -> custom

+        if (strcmp(endptr, "kB") == 0)

Why kB here instead of KB to match MB, GB, TB below?

+    int64        relseg_size;    /* blocks per segment of large relation */

This will require PG_CONTROL_VERSION to be bumped -- but you are 
probably waiting until commit time to avoid annoying conflicts, though I 
don't think it is as likely as with CATALOG_VERSION_NO.

> Some random thoughts:
> 
> Another potential option name would be --segsize, if we think we're
> going to use this for temp files too eventually.

I feel like temp file segsize should be separately configurable for the 
same reason that we are leaving it as 1GB for now.

> Maybe it's not so beautiful to have that global variable
> rel_segment_size (which replaces REL_SEGSIZE everywhere).  

Maybe not, but it is the way these things are done in general, .e.g. 
wal_segment_size, so I don't think it will be too controversial.

> Another
> idea would be to make it static in md.c and call smgrsetsegmentsize(),
> or something like that.  That could be a nice place to compute the
> "shift" value up front, instead of computing it each time in
> blockno_to_segno(), but that's probably not worth bothering with (?).
> BSR/LZCNT/CLZ instructions are pretty fast on modern chips.  That's
> about the only place where someone could say that this change makes
> things worse for people not interested in the new feature, so I was
> careful to get rid of / and % operations with no-longer-constant RHS.

Right -- not sure we should be troubling ourselves with trying to 
optimize away ops that are very fast, unless they are computed trillions 
of times.

> I had to promote segment size to int64 (global variable, field in
> control file), because otherwise it couldn't represent
> --rel-segsize=32TB (it'd be too big by one).  Other ideas would be to
> store the shift value instead of the size, or store the max block
> number, eg subtract one, or use InvalidBlockNumber to mean "no limit"
> (with more branches to test for it).  The only problem I ran into with
> the larger type was that 'SHOW segment_size' now needs a custom show
> function because we don't have int64 GUCs.

A custom show function seems like a reasonable solution here.

> A C type confusion problem that I noticed: some code uses BlockNumber
> and some code uses int for segment numbers.  It's not really a
> reachable problem for practical reasons (you'd need over 2 billion
> directories and VFDs to reach it), but it's wrong to use int if
> segment size can be set as low as BLCKSZ (one file per block); you
> could have more segments than an int can represent.  We could go for
> uint32, BlockNumber or create SegmentNumber (which I think I've
> proposed before, and lost track of...).  We can address that
> separately (perhaps by finding my old patch...)

I think addressing this separately is fine, though maybe enforcing some 
reasonable minimum in initdb would be a good idea for this patch. For my 
2c SEGSIZE == BLOCKSZ just makes very little sense.

Lastly, I think the blockno_to_segno(), blockno_within_segment(), and 
blockno_to_seekpos() functions add enough readability that they should 
be committed regardless of how this patch proceeds.

Regards,
-David