Thread

1. vacuumlo - use a cursor

   Andrew Dunstan <andrew@dunslane.net> — 2012-11-12T22:14:57Z

   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
   

2. Re: vacuumlo - use a cursor

   Bruce Momjian <bruce@momjian.us> — 2013-06-29T15:23:49Z

   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@postgresql.org)
   > To make changes to your subscription:
   > http://www.postgresql.org/mailpref/pgsql-hackers
   
   
   -- 
     Bruce Momjian  <bruce@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +
   
   
   

3. Re: vacuumlo - use a cursor

   Andrew Dunstan <andrew@dunslane.net> — 2013-06-29T15:33:54Z

   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@postgresql.org)
   >> To make changes to your subscription:
   >> http://www.postgresql.org/mailpref/pgsql-hackers
   >
   
   
   
   

4. Re: vacuumlo - use a cursor

   Bruce Momjian <bruce@momjian.us> — 2013-06-29T15:35:31Z

   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@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +
   
   
   

5. Re: vacuumlo - use a cursor

   Joshua D. Drake <jd@commandprompt.com> — 2013-06-29T15:56:18Z

   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
   
   
   

6. Re: vacuumlo - use a cursor

   Andrew Dunstan <andrew@dunslane.net> — 2013-06-29T16:21:11Z

   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
   
   
   

7. Re: vacuumlo - use a cursor

   Bruce Momjian <bruce@momjian.us> — 2013-06-29T18:38:26Z

   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@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +
   
   
   

8. Re: vacuumlo - use a cursor

   Josh Kupershmidt <schmiddy@gmail.com> — 2013-07-07T13:30:38Z

   On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan <andrew@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
   

9. Re: vacuumlo - use a cursor

   Robert Haas <robertmhaas@gmail.com> — 2013-07-15T14:53:02Z

   On Sun, Jul 7, 2013 at 9:30 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
   > On Mon, Nov 12, 2012 at 5:14 PM, Andrew Dunstan <andrew@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