On Tue, 9 Mar 2021 at 18:39, John Naylor <john.nay...@enterprisedb.com> wrote: > > I wrote: > > > That seems like the proper fix, and I see you've started a thread for that. > > I don't think that change in behavior would be backpatchable, but patch > > here might have a chance at that. > > I remembered after the fact that truncating line pointers would only allow > for omitting the 2% slack logic (and has other benefits), but the rest of > this patch would be needed regardless.
Regarding the 2% slack logic, could we change it to use increments of line pointers instead? That makes it more clear what problem this solution is trying to work around; that is to say line pointers not (all) being truncated away. The currently subtracted value accounts for the size of 40 line pointers on 8k-pages (~ 13.7% of MaxHeapTuplesPerPage), and slightly higher fractions (up to 13.94%) for larger page sizes. As the to-be-inserted tuple is already _at least_ 10% of MaxHeapTupleSize when it hits this new code. Also, even with this patch, we do FSM-requests of for sizes between MaxHeapTupleSize - 2% and MaxHeapTupleSize, if len+saveFreeSpace falls between those two numbers. I think we better clamp the fsm request between `len` and `MaxHeapTupleSize - PAGE_SIZE_DEPENDENT_FACTOR`. So, I sugges the following incremental patch: bool needLock; + const Size maxPaddedFsmRequest = MaxHeapTupleSize - (MaxHeapTuplesPerPage / 8 * sizeof(ItemIdData)); ... - if (len + saveFreeSpace > MaxHeapTupleSize) + if (len + saveFreeSpace > maxPaddedFsmRequest) ... - targetFreeSpace = Max(len, MaxHeapTupleSize - (MaxHeapTupleSize * 2 / 100)); + targetFreeSpace = Max(len, maxPaddedFsmRequest); ... Other than this, I think this is a good fix. With regards, Matthias van de Meent.
