Thread: Some memory allocations in gin fastupdate code are a bit brain dead
I recently stumbled upon the following code in ginfast.c: while (collector->ntuples + nentries > collector->lentuples) { collector->lentuples *= 2; collector->tuples = (IndexTuple *) repalloc(collector->tuples, sizeof(IndexTuple) * collector->lentuples); } it does not seem very smart to perform the repalloc() inside the loop here as there could be many loops before we double the lentuples so that it's large enough to allow storage of all the required values. The attached patch changes things around so that the repalloc() is done outside of the loop. i.e. done only once, after we've determined the correct size to reallocate it to. I've also added an else condition so that we only bother checking this case when the tuples[] array is not already allocated. I tested with the following: create table t1 (a int[], b int[]); create index on t1 using gin (a,b) with (fastupdate = on); truncate t1; insert into t1 select '{1}'::int[],('{' || string_agg(y::text, ',') || '}')::int[] from generate_Series(1,1000000) x, generate_Series(1,10) y group by x order by x desc; In the above case with an unpatched master, I measured the repalloc() to only consume 0.6% of the total runtime of the INSERT, so this does not really improve performance by much, but I thought it was worth fixing never-the-less. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
David Rowley <david.rowley@2ndquadrant.com> writes: > I recently stumbled upon the following code in ginfast.c: > while (collector->ntuples + nentries > collector->lentuples) > { > collector->lentuples *= 2; > collector->tuples = (IndexTuple *) repalloc(collector->tuples, > sizeof(IndexTuple) * collector->lentuples); > } I agree that's pretty stupid, but I wonder whether we should also try harder in the initial-allocation path. Right now, if the initial pass through this code sees say 3 tuples to insert, it will then work with 3 * 2^n entries in subsequent enlargements, which is likely to be pretty inefficient. Should we try to force the initial allocation to be a power of 2? Also, do we need to worry about integer overflow? regards, tom lane
On Wed, 19 Dec 2018 at 04:24, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <david.rowley@2ndquadrant.com> writes: > > I recently stumbled upon the following code in ginfast.c: > > while (collector->ntuples + nentries > collector->lentuples) > > { > > collector->lentuples *= 2; > > collector->tuples = (IndexTuple *) repalloc(collector->tuples, > > sizeof(IndexTuple) * collector->lentuples); > > } > > I agree that's pretty stupid, but I wonder whether we should also try > harder in the initial-allocation path. Right now, if the initial > pass through this code sees say 3 tuples to insert, it will then work > with 3 * 2^n entries in subsequent enlargements, which is likely to > be pretty inefficient. Should we try to force the initial allocation > to be a power of 2? Yeah, I think that's a good idea. > Also, do we need to worry about integer overflow? Good point. I didn't think of it before, but the previous code would have ensured that we got an ERROR before any overflow could have occurred as the repalloc() would fail once the allocation request went beyond MaxAllocSize. However, I think an overflow is sufficiently unlikely that we don't need to do any batching, but we should keep it an error and not a crash. I propose the attached. If we hit the overflow case, we'll still attempt the repalloc(), but it'll fail, just as it would have before, just without the needless calls. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
David Rowley <david.rowley@2ndquadrant.com> writes: > On Wed, 19 Dec 2018 at 04:24, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Also, do we need to worry about integer overflow? > Good point. I didn't think of it before, but the previous code would > have ensured that we got an ERROR before any overflow could have > occurred as the repalloc() would fail once the allocation request went > beyond MaxAllocSize. However, I think an overflow is sufficiently > unlikely that we don't need to do any batching, but we should keep it > an error and not a crash. Agreed. > I propose the attached. If we hit the overflow case, we'll still > attempt the repalloc(), but it'll fail, just as it would have before, > just without the needless calls. I don't think this is quite bulletproof against overflow, especially in view of the rather careless mixing of int32 and uint32 variables that exists here. The easiest way to make it bulletproof is to add an explicit test, so I did that and pushed it. regards, tom lane
On Thu, 20 Dec 2018 at 05:44, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't think this is quite bulletproof against overflow, especially > in view of the rather careless mixing of int32 and uint32 variables > that exists here. The easiest way to make it bulletproof is to add > an explicit test, so I did that and pushed it. Thanks for tidying that up and for pushing. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services