Thread: Re: Shared memory size computation oversight?
On 27.02.21 09:08, Julien Rouhaud wrote: > PFA a patch that fixes pg_prewarm and pg_stat_statements explicit alignment to > CACHELINEALIGN, and also update the alignment in hash_estimate_size() to what I > think ShmemInitHash will actually consume. For extensions, wouldn't it make things better if RequestAddinShmemSpace() applied CACHELINEALIGN() to its argument? If you consider the typical sequence of RequestAddinShmemSpace(mysize()) and later ShmemInitStruct(..., mysize(), ...), then the size gets rounded up to cache-line size in the second case and not the first. The premise in your patch is that the extension should figure that out before calling RequestAddinShmemSpace(), but that seems wrong. Btw., I think your patch was wrong to apply CACHELINEALIGN() to intermediate results rather than at the end.
On Thu, Aug 12, 2021 at 9:34 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > > On 27.02.21 09:08, Julien Rouhaud wrote: > > PFA a patch that fixes pg_prewarm and pg_stat_statements explicit alignment to > > CACHELINEALIGN, and also update the alignment in hash_estimate_size() to what I > > think ShmemInitHash will actually consume. > > For extensions, wouldn't it make things better if > RequestAddinShmemSpace() applied CACHELINEALIGN() to its argument? > > If you consider the typical sequence of RequestAddinShmemSpace(mysize()) > and later ShmemInitStruct(..., mysize(), ...), But changing RequestAddinShmemSpace() to apply CACHELINEALIGN() would only really work for that specific usage only? If an extension does multiple allocations you can't rely on correct results. > then the size gets > rounded up to cache-line size in the second case and not the first. The > premise in your patch is that the extension should figure that out > before calling RequestAddinShmemSpace(), but that seems wrong. > > Btw., I think your patch was wrong to apply CACHELINEALIGN() to > intermediate results rather than at the end. I'm not sure why you think it's wrong. It's ShmemInitStruct() that will apply the padding, so if the extension calls it multiple times (whether directly or indirectly), then the padding will be added multiple times. Which means that in theory the extension should account for it multiple time in the amount of memory it's requesting. But given the later discussion, ISTM that there's an agreement that any number of CACHELINEALIGN() overhead for any number of allocation can be ignored as it should be safely absorbed by the 100kB extra space. I think that the real problem, as mentioned in my last email, is that the shared htab size estimation doesn't really scale and can easily eat the whole extra space if you use some not that unrealistic parameters. I still don't know if I made any mistake trying to properly account for the htab allocation, but if I didn't it can be problematic.
On 12.08.21 16:18, Julien Rouhaud wrote: > On Thu, Aug 12, 2021 at 9:34 PM Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: >> >> On 27.02.21 09:08, Julien Rouhaud wrote: >>> PFA a patch that fixes pg_prewarm and pg_stat_statements explicit alignment to >>> CACHELINEALIGN, and also update the alignment in hash_estimate_size() to what I >>> think ShmemInitHash will actually consume. >> >> For extensions, wouldn't it make things better if >> RequestAddinShmemSpace() applied CACHELINEALIGN() to its argument? >> >> If you consider the typical sequence of RequestAddinShmemSpace(mysize()) >> and later ShmemInitStruct(..., mysize(), ...), > > But changing RequestAddinShmemSpace() to apply CACHELINEALIGN() would > only really work for that specific usage only? If an extension does > multiple allocations you can't rely on correct results. I think you can do different things here to create inconsistent results, but I think there should be one common, standard, normal, straightforward way to get a correct result. >> Btw., I think your patch was wrong to apply CACHELINEALIGN() to >> intermediate results rather than at the end. > > I'm not sure why you think it's wrong. It's ShmemInitStruct() that > will apply the padding, so if the extension calls it multiple times > (whether directly or indirectly), then the padding will be added > multiple times. Which means that in theory the extension should > account for it multiple time in the amount of memory it's requesting. Yeah, in that case it's probably rather the case that there is one CACHELINEALIGN() too few, since pg_stat_statements does two separate shmem allocations.
On Fri, Aug 13, 2021 at 10:52:50AM +0200, Peter Eisentraut wrote: > On 12.08.21 16:18, Julien Rouhaud wrote: > > > > But changing RequestAddinShmemSpace() to apply CACHELINEALIGN() would > > only really work for that specific usage only? If an extension does > > multiple allocations you can't rely on correct results. > > I think you can do different things here to create inconsistent results, but > I think there should be one common, standard, normal, straightforward way to > get a correct result. Unless I'm missing something, the standard and straightforward way to get a correct result is to account for padding bytes in the C code, as it's currently done in all contrib modules. The issue is that those modules aren't using the correct alignment anymore. > > > > Btw., I think your patch was wrong to apply CACHELINEALIGN() to > > > intermediate results rather than at the end. > > > > I'm not sure why you think it's wrong. It's ShmemInitStruct() that > > will apply the padding, so if the extension calls it multiple times > > (whether directly or indirectly), then the padding will be added > > multiple times. Which means that in theory the extension should > > account for it multiple time in the amount of memory it's requesting. > > Yeah, in that case it's probably rather the case that there is one > CACHELINEALIGN() too few, since pg_stat_statements does two separate shmem > allocations. I still don't get it. Aligning the total shmem size is totally different from properly padding all single allocation sizes, and is almost never the right answer. Using a naive example, if your extension needs to ShmemInitStruct() twice 64B, postgres will consume 2*128B. But if you only rely on RequestAddinShmemSpace() to add a CACHELINEALIGN(), then no padding at all will be added, and you'll then be not one but two CACHELINEALIGN() too few. But again, the real issue is not the CACHELINEALIGN() roundings, as those have a more or less stable size and are already accounted for in the extra 100kB, but the dynahash size estimation which seems to be increasingly off as the number of entries grows.