Re: [HACKERS] vacuumlo - use a cursor
On Sun, Jul 7, 2013 at 9:30 AM, Josh Kupershmidt schmi...@gmail.com wrote: On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan and...@dunslane.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vacuumlo - use a cursor
On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan and...@dunslane.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 vacuumlo-cursor.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vacuumlo - use a cursor
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));
!
!
Re: [HACKERS] vacuumlo - use a cursor
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++)
{
!
Re: [HACKERS] vacuumlo - use a cursor
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vacuumlo - use a cursor
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vacuumlo - use a cursor
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] vacuumlo - use a cursor
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 br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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
*** 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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
