Re: Re: bulk_multi_insert infinite loops with large rows and small fill factors

On 12.12.2012 13:27, Andres Freund wrote:
> On 2012-12-12 03:04:19 -0800, David Gould wrote:
>> One more point, in the case where we don't insert any rows, we still do all the
>> dirtying and logging work even though we did not modify the page. I have tried
>> skip all this if no rows are added (nthispage == 0), but my access method foo
>> is sadly out of date, so someone should take a skeptical look at that.
>>
>> A test case and patch against 9.2.2 is attached. It fixes the problem and passes
>> make check. Most of the diff is just indentation changes. Whoever tries this will
>> want to test this on a small partition by itself.
>
> ISTM this would be fixed with a smaller footprint by just making
>
> if (PageGetHeapFreeSpace(page)< MAXALIGN(heaptup->t_len) + saveFreeSpace)
>
> if (!PageIsEmpty(page)&&
> PageGetHeapFreeSpace(page)< MAXALIGN(heaptup->t_len) + saveFreeSpace)
>
> I think that should work?

Yeah, seems that it should, although PageIsEmpty() is no guarantee that
the tuple fits, because even though PageIsEmpty() returns true, there
might be dead line pointers consuming so much space that the tuple at
hand doesn't fit. However, RelationGetBufferForTuple() won't return such
a page, it guarantees that the first tuple does indeed fit on the page
it returns. For the same reason, the later check that at least one tuple
was actually placed on the page is not necessary.

I committed a slightly different version, which unconditionally puts the
first tuple on the page, and only applies the freespace check to the
subsequent tuples. Since RelationGetBufferForTuple() guarantees that the
first tuple fits, we can trust that, like heap_insert does.

--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2172,8 +2172,12 @@ heap_multi_insert(Relation relation, HeapTuple
*tuples, int ntuples,
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();

- /* Put as many tuples as fit on this page */
- for (nthispage = 0; ndone + nthispage < ntuples; nthispage++)
+ /*
+ * RelationGetBufferForTuple has ensured that the first tuple fits.
+ * Put that on the page, and then as many other tuples as fit.
+ */
+ RelationPutHeapTuple(relation, buffer, heaptuples[ndone]);
+ for (nthispage = 1; ndone + nthispage < ntuples; nthispage++)
{
HeapTuple heaptup = heaptuples[ndone + nthispage];

Thanks for the report!

- Heikki