Re: vacuumlo - use a cursor

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. 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.

cheers

andrew

Is there a reason this patch was not applied?

---------------------------------------------------------------------------

On Mon, Nov 12, 2012 at 05:14:57PM -0500, Andrew Dunstan 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. 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.
>
> cheers
>
> andrew

> *** a/contrib/vacuumlo/vacuumlo.c
> --- b/contrib/vacuumlo/vacuumlo.c
> ***************
> *** 290,362 **** vacuumlo(const char *database, const struct _param * param)
> PQclear(res);
>
> buf[0] = '\0';
> ! strcat(buf, "SELECT lo FROM vacuum_l");
> ! res = PQexec(conn, buf);
> ! if (PQresultStatus(res) != PGRES_TUPLES_OK)
> ! {
> ! fprintf(stderr, "Failed to read temp table:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! PQclear(res);
> PQfinish(conn);
> return -1;
> ! }
>
> - matched = PQntuples(res);
> deleted = 0;
> ! for (i = 0; i < matched; i++)
> {
> ! Oid lo = atooid(PQgetvalue(res, i, 0));
>
> ! if (param->verbose)
> {
> ! fprintf(stdout, "\rRemoving lo %6u ", lo);
> ! fflush(stdout);
> }
>
> ! if (param->dry_run == 0)
> {
> ! if (lo_unlink(conn, lo) < 0)
> {
> ! fprintf(stderr, "\nFailed to remove lo %u: ", lo);
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! if (PQtransactionStatus(conn) == PQTRANS_INERROR)
> {
> ! success = false;
> ! break;
> }
> }
> else
> deleted++;
> ! }
> ! else
> ! deleted++;
> ! if (param->transaction_limit > 0 &&
> ! (deleted % param->transaction_limit) == 0)
> ! {
> ! res2 = PQexec(conn, "commit");
> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
> {
> ! fprintf(stderr, "Failed to commit transaction:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> PQclear(res2);
> ! PQclear(res);
> ! PQfinish(conn);
> ! return -1;
> ! }
> ! PQclear(res2);
> ! res2 = PQexec(conn, "begin");
> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
> ! {
> ! fprintf(stderr, "Failed to start transaction:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> PQclear(res2);
> - PQclear(res);
> - PQfinish(conn);
> - return -1;
> }
> - PQclear(res2);
> }
> }
> PQclear(res);
>
> /*
> --- 290,389 ----
> PQclear(res);
>
> buf[0] = '\0';
> ! strcat(buf,
> ! "DECLARE myportal CURSOR WITH HOLD FOR SELECT lo FROM vacuum_l");
> ! res = PQexec(conn, buf);
> ! if (PQresultStatus(res) != PGRES_COMMAND_OK)
> ! {
> ! fprintf(stderr, "DECLARE CURSOR failed: %s", PQerrorMessage(conn));
> ! PQclear(res);
> PQfinish(conn);
> return -1;
> ! }
> ! PQclear(res);
> !
> ! snprintf(buf, BUFSIZE, "FETCH FORWARD " INT64_FORMAT " IN myportal",
> ! param->transaction_limit > 0 ? param->transaction_limit : 1000);
>
> deleted = 0;
> !
> ! while (1)
> {
> ! res = PQexec(conn, buf);
> ! if (PQresultStatus(res) != PGRES_TUPLES_OK)
> ! {
> ! fprintf(stderr, "Failed to read temp table:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! PQclear(res);
> ! PQfinish(conn);
> ! return -1;
> ! }
>
> ! matched = PQntuples(res);
> !
> ! if (matched <= 0)
> {
> ! /* at end of resultset */
> ! break;
> }
>
> ! for (i = 0; i < matched; i++)
> {
> ! Oid lo = atooid(PQgetvalue(res, i, 0));
> !
> ! if (param->verbose)
> ! {
> ! fprintf(stdout, "\rRemoving lo %6u ", lo);
> ! fflush(stdout);
> ! }
> !
> ! if (param->dry_run == 0)
> {
> ! if (lo_unlink(conn, lo) < 0)
> {
> ! fprintf(stderr, "\nFailed to remove lo %u: ", lo);
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! if (PQtransactionStatus(conn) == PQTRANS_INERROR)
> ! {
> ! success = false;
> ! break;
> ! }
> }
> + else
> + deleted++;
> }
> else
> deleted++;
> !
> ! if (param->transaction_limit > 0 &&
> ! (deleted % param->transaction_limit) == 0)
> {
> ! res2 = PQexec(conn, "commit");
> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
> ! {
> ! fprintf(stderr, "Failed to commit transaction:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! PQclear(res2);
> ! PQclear(res);
> ! PQfinish(conn);
> ! return -1;
> ! }
> PQclear(res2);
> ! res2 = PQexec(conn, "begin");
> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
> ! {
> ! fprintf(stderr, "Failed to start transaction:\n");
> ! fprintf(stderr, "%s", PQerrorMessage(conn));
> ! PQclear(res2);
> ! PQclear(res);
> ! PQfinish(conn);
> ! return -1;
> ! }
> PQclear(res2);
> }
> }
> }
> +
> PQclear(res);
>
> /*

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Nobody seemed interested. But I do think it's a good idea still.

cheers

andrew

On 06/29/2013 11:23 AM, Bruce Momjian wrote:
> Is there a reason this patch was not applied?
>
> ---------------------------------------------------------------------------
>
> On Mon, Nov 12, 2012 at 05:14:57PM -0500, Andrew Dunstan 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. 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.
>>
>> cheers
>>
>> andrew
>> *** a/contrib/vacuumlo/vacuumlo.c
>> --- b/contrib/vacuumlo/vacuumlo.c
>> ***************
>> *** 290,362 **** vacuumlo(const char *database, const struct _param * param)
>> PQclear(res);
>>
>> buf[0] = '\0';
>> ! strcat(buf, "SELECT lo FROM vacuum_l");
>> ! res = PQexec(conn, buf);
>> ! if (PQresultStatus(res) != PGRES_TUPLES_OK)
>> ! {
>> ! fprintf(stderr, "Failed to read temp table:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! PQclear(res);
>> PQfinish(conn);
>> return -1;
>> ! }
>>
>> - matched = PQntuples(res);
>> deleted = 0;
>> ! for (i = 0; i < matched; i++)
>> {
>> ! Oid lo = atooid(PQgetvalue(res, i, 0));
>>
>> ! if (param->verbose)
>> {
>> ! fprintf(stdout, "\rRemoving lo %6u ", lo);
>> ! fflush(stdout);
>> }
>>
>> ! if (param->dry_run == 0)
>> {
>> ! if (lo_unlink(conn, lo) < 0)
>> {
>> ! fprintf(stderr, "\nFailed to remove lo %u: ", lo);
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! if (PQtransactionStatus(conn) == PQTRANS_INERROR)
>> {
>> ! success = false;
>> ! break;
>> }
>> }
>> else
>> deleted++;
>> ! }
>> ! else
>> ! deleted++;
>> ! if (param->transaction_limit > 0 &&
>> ! (deleted % param->transaction_limit) == 0)
>> ! {
>> ! res2 = PQexec(conn, "commit");
>> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
>> {
>> ! fprintf(stderr, "Failed to commit transaction:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> PQclear(res2);
>> ! PQclear(res);
>> ! PQfinish(conn);
>> ! return -1;
>> ! }
>> ! PQclear(res2);
>> ! res2 = PQexec(conn, "begin");
>> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
>> ! {
>> ! fprintf(stderr, "Failed to start transaction:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> PQclear(res2);
>> - PQclear(res);
>> - PQfinish(conn);
>> - return -1;
>> }
>> - PQclear(res2);
>> }
>> }
>> PQclear(res);
>>
>> /*
>> --- 290,389 ----
>> PQclear(res);
>>
>> buf[0] = '\0';
>> ! strcat(buf,
>> ! "DECLARE myportal CURSOR WITH HOLD FOR SELECT lo FROM vacuum_l");
>> ! res = PQexec(conn, buf);
>> ! if (PQresultStatus(res) != PGRES_COMMAND_OK)
>> ! {
>> ! fprintf(stderr, "DECLARE CURSOR failed: %s", PQerrorMessage(conn));
>> ! PQclear(res);
>> PQfinish(conn);
>> return -1;
>> ! }
>> ! PQclear(res);
>> !
>> ! snprintf(buf, BUFSIZE, "FETCH FORWARD " INT64_FORMAT " IN myportal",
>> ! param->transaction_limit > 0 ? param->transaction_limit : 1000);
>>
>> deleted = 0;
>> !
>> ! while (1)
>> {
>> ! res = PQexec(conn, buf);
>> ! if (PQresultStatus(res) != PGRES_TUPLES_OK)
>> ! {
>> ! fprintf(stderr, "Failed to read temp table:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! PQclear(res);
>> ! PQfinish(conn);
>> ! return -1;
>> ! }
>>
>> ! matched = PQntuples(res);
>> !
>> ! if (matched <= 0)
>> {
>> ! /* at end of resultset */
>> ! break;
>> }
>>
>> ! for (i = 0; i < matched; i++)
>> {
>> ! Oid lo = atooid(PQgetvalue(res, i, 0));
>> !
>> ! if (param->verbose)
>> ! {
>> ! fprintf(stdout, "\rRemoving lo %6u ", lo);
>> ! fflush(stdout);
>> ! }
>> !
>> ! if (param->dry_run == 0)
>> {
>> ! if (lo_unlink(conn, lo) < 0)
>> {
>> ! fprintf(stderr, "\nFailed to remove lo %u: ", lo);
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! if (PQtransactionStatus(conn) == PQTRANS_INERROR)
>> ! {
>> ! success = false;
>> ! break;
>> ! }
>> }
>> + else
>> + deleted++;
>> }
>> else
>> deleted++;
>> !
>> ! if (param->transaction_limit > 0 &&
>> ! (deleted % param->transaction_limit) == 0)
>> {
>> ! res2 = PQexec(conn, "commit");
>> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
>> ! {
>> ! fprintf(stderr, "Failed to commit transaction:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! PQclear(res2);
>> ! PQclear(res);
>> ! PQfinish(conn);
>> ! return -1;
>> ! }
>> PQclear(res2);
>> ! res2 = PQexec(conn, "begin");
>> ! if (PQresultStatus(res2) != PGRES_COMMAND_OK)
>> ! {
>> ! fprintf(stderr, "Failed to start transaction:\n");
>> ! fprintf(stderr, "%s", PQerrorMessage(conn));
>> ! PQclear(res2);
>> ! PQclear(res);
>> ! PQfinish(conn);
>> ! return -1;
>> ! }
>> PQclear(res2);
>> }
>> }
>> }
>> +
>> PQclear(res);
>>
>> /*
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Sat, Jun 29, 2013 at 11:33:54AM -0400, Andrew Dunstan wrote:
>
> Nobody seemed interested. But I do think it's a good idea still.

Well, if no one replied, and you thought it was a good idea, then it was
a good idea. ;-)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On 06/29/2013 08:35 AM, Bruce Momjian wrote:
>
> On Sat, Jun 29, 2013 at 11:33:54AM -0400, Andrew Dunstan wrote:
>>
>> Nobody seemed interested. But I do think it's a good idea still.
>
> Well, if no one replied, and you thought it was a good idea, then it was
> a good idea. ;-)
>

I think it is a good idea just of limited use. I only have one customer
that still uses large objects. Which is a shame really as they are more
efficient that bytea.

JD

--
Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
For my dreams of your image that blossoms
a rose in the deeps of my heart. - W.B. Yeats

On 06/29/2013 11:35 AM, Bruce Momjian wrote:
> On Sat, Jun 29, 2013 at 11:33:54AM -0400, Andrew Dunstan wrote:
>> Nobody seemed interested. But I do think it's a good idea still.
> Well, if no one replied, and you thought it was a good idea, then it was
> a good idea. ;-)
>

I try not to assume that even if I think it's a good idea it will be
generally wanted unless at least one other person speaks up. But now
that Joshua has I will revive it and add it to the next commitfest.

cheers

andrew

On Sat, Jun 29, 2013 at 12:21:11PM -0400, Andrew Dunstan wrote:
>
> On 06/29/2013 11:35 AM, Bruce Momjian wrote:
> >On Sat, Jun 29, 2013 at 11:33:54AM -0400, Andrew Dunstan wrote:
> >>Nobody seemed interested. But I do think it's a good idea still.
> >Well, if no one replied, and you thought it was a good idea, then it was
> >a good idea. ;-)
> >
>
>
> I try not to assume that even if I think it's a good idea it will be
> generally wanted unless at least one other person speaks up. But now
> that Joshua has I will revive it and add it to the next commitfest.

Great.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

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).

Josh

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