Re: vacuumlo - use a cursor

On Sun, Jul 7, 2013 at 9:30 AM, Josh Kupershmidt <schmiddy(at)gmail(dot)com> wrote:
> On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> vacuumlo is rather simpleminded about dealing with the list of LOs to be
>> removed - it just fetches them as a straight resultset. For one of my our
>> this resulted in an out of memory condition.
>
> Wow, they must have had a ton of LOs. With about 2M entries to pull
> from vacuum_l, I observed unpatched vacuumlo using only about 45MB
> RES. Still, the idea of using a cursor for the main loop seems like a
> reasonable idea.
>
>> The attached patch tries to
>> remedy that by using a cursor instead. If this is wanted I will add it to
>> the next commitfest. The actualy changes are very small - most of the patch
>> is indentation changes due to the introduction of an extra loop.
>
> I had some time to review this, some comments about the patch:
>
> 1. I see this new compiler warning:
>
> vacuumlo.c: In function ‘vacuumlo’:
> vacuumlo.c:306:5: warning: format ‘%lld’ expects argument of type
> ‘long long int’, but argument 4 has type ‘long int’ [-Wformat]
>
> 2. It looks like the the patch causes all the intermediate result-sets
> fetched from the cursor to be leaked, rather negating its stated
> purpose ;-) The PQclear() call should be moved up into the main loop.
> With this fixed, I confirmed that vacuumlo now consumes a negligible
> amount of memory when chewing through millions of entries.
>
> 3. A few extra trailing whitespaces were added.
>
> 4. The FETCH FORWARD count comes from transaction_limit. That seems
> like a good-enough guesstimate, but maybe a comment could be added to
> rationalize?
>
> Some suggested changes attached with v2 patch (all except #4).

I've committed this version of the patch, with a slight change to one
of the error messages.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company