Thread: vacuumlo patch
Please consider adding this minor patch, or offering improvements.<br /><br /><b>Problem</b>: vacuumlo required PostgreSQLto use more locks per transaction than possible resulting in an error and a log file full of "ignored until endof transaction" warnings. <br /> (max_locks_per_transaction is limited by shmmax which is limited by RAM)<br /><br /><b>Solution</b>:ask vacuumlo to complete after N lo_unlink(OID)s.<br /><b><br />Alternate Solution</b>: ask vacuumlo tostart a new transaction after N lo_unlink(OID)s.<br /> (more complex and can be implemented with the other solution inthe shell)<br /><br /><b>Design Decisions</b>: Add a flag so the new behavior is optional and it defaults to the old behavior.<br/><br /><b>Implementation</b>: Followed code formatting style.<br /> It's likely pointless to check if an intis > 2147483647 but maybe it clarifies the code.<br />Added a friendly error message and the a line in the help.<br/><br /><span style="font-family: courier new,monospace;">git diff -U0 HEAD -- contrib/vacuumlo/vacuumlo.c</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">diff--git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">index f6e2a28..b7c7d64 100644</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">--- a/contrib/vacuumlo/vacuumlo.c</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+++b/contrib/vacuumlo/vacuumlo.c</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">@@ -50,0 +51 @@ struct _param</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ int transaction_limit;</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">@@ -284,0 +286 @@ vacuumlo(char*database, struct _param * param)</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">+ {</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">@@ -285,0 +288,5 @@ vacuumlo(char *database, struct _param * param)</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ if(param->transaction_limit!=0 && deleted>=param->transaction_limit)</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ {</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ break;</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ }</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">+ }</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">@@ -315,0 +323 @@ usage(const char *progname)</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ printf(" -l LIMIT stop after removing LIMITLOs\n");</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">@@-344,0 +353 @@ main(int argc, char **argv)</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;">+ param.transaction_limit = 0;</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">@@ -362 +371 @@ main(int argc, char **argv)</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">- c = getopt(argc, argv, "h:U:p:vnwW");</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">+ c = getopt(argc, argv, "h:U:p:l:vnwW");</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">@@ -397,0 +407,8 @@ main(intargc, char **argv)</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ case 'l':</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">+ param.transaction_limit = strtol(optarg, NULL, 10);</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ if ((param.transaction_limit< 0) || (param.transaction_limit > 2147483647))</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ {</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">+ fprintf(stderr, "%s: invalidtransaction limit number: %s, valid range is form 0(disabled) to 2147483647.\n", progname, optarg);</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ exit(1);</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ }</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ break;</span><br style="font-family: courier new,monospace;" /><br />
Excerpts from Tim's message of dom jul 24 14:48:08 -0400 2011: > Please consider adding this minor patch, or offering improvements. > > *Problem*: vacuumlo required PostgreSQL to use more locks per transaction > than possible resulting in an error and a log file full of "ignored until > end of transaction" warnings. > (max_locks_per_transaction is limited by shmmax which is limited by RAM) Interesting. I cannot vouch for or against the patch, but please resubmit without the -U0 switch. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Hi Álvaro, thanks for the response.<br /><br />Here is the requested "diff with 3 lines of context":<font color="#888888"><br/><br /></font>git diff HEAD -- contrib/vacuumlo/vacuumlo.c<br />diff --git a/contrib/vacuumlo/vacuumlo.cb/contrib/vacuumlo/vacuumlo.c<br /> index f6e2a28..b7c7d64 100644<br />--- a/contrib/vacuumlo/vacuumlo.c<br/>+++ b/contrib/vacuumlo/vacuumlo.c<br />@@ -48,6 +48,7 @@ struct _param<br /> char *pg_host;<br /> int verbose;<br /> int dry_run;<br /> + int transaction_limit;<br/> };<br /> <br /> int vacuumlo(char *, struct _param *);<br />@@ -282,7 +283,13 @@ vacuumlo(char*database, struct _param * param)<br /> fprintf(stderr, "%s", PQerrorMessage(conn));<br /> }<br /> else<br />+ {<br /> deleted++;<br />+ if(param->transaction_limit!=0&& deleted>=param->transaction_limit)<br />+ {<br />+ break;<br /> + }<br />+ }<br /> }<br /> else<br /> deleted++;<br/>@@ -313,6 +320,7 @@ usage(const char *progname)<br /> printf(" -h HOSTNAME database server host or socketdirectory\n");<br /> printf(" -n don't remove large objects, just show what would be done\n");<br /> printf(" -p PORT database server port\n");<br />+ printf(" -l LIMIT stop after removing LIMIT LOs\n");<br/> printf(" -U USERNAME user name to connect as\n");<br /> printf(" -w never prompt forpassword\n");<br /> printf(" -W force password prompt\n");<br />@@ -342,6 +350,7 @@ main(int argc, char**argv)<br /> param.pg_port = NULL;<br /> param.verbose = 0;<br /> param.dry_run = 0;<br />+ param.transaction_limit= 0;<br /> <br /> if (argc > 1)<br /> {<br />@@ -359,7 +368,7 @@ main(int argc, char **argv)<br/> <br /> while (1)<br /> {<br />- c = getopt(argc, argv, "h:U:p:vnwW");<br />+ c = getopt(argc,argv, "h:U:p:l:vnwW");<br /> if (c == -1)<br /> break;<br /> <br />@@ -395,6 +404,14 @@ main(intargc, char **argv)<br /> }<br /> param.pg_port = strdup(optarg);<br /> break;<br />+ case 'l':<br />+ param.transaction_limit = strtol(optarg, NULL, 10);<br />+ if ((param.transaction_limit < 0) || (param.transaction_limit > 2147483647))<br /> + {<br />+ fprintf(stderr, "%s: invalid transaction limit number: %s, valid range is form 0(disabled)to 2147483647.\n", progname, optarg);<br />+ exit(1);<br />+ }<br /> + break;<br /> case 'h':<br /> param.pg_host = strdup(optarg);<br /> break;<br />
Updated the patch to also apply when the no-action flag is enabled.<br /><br /><span style="font-family: courier new,monospace;">gitdiff HEAD -- contrib/vacuumlo/vacuumlo.c</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;">diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">indexf6e2a28..8e9c342 100644</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">--- a/contrib/vacuumlo/vacuumlo.c</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+++ b/contrib/vacuumlo/vacuumlo.c</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">@@ -48,6 +48,7 @@ struct _param</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> char *pg_host;</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> int verbose;</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> int dry_run;</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ int transaction_limit;</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> };</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> </span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> int vacuumlo(char*, struct _param *);</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">@@ -282,10 +283,18 @@ vacuumlo(char *database, struct _param * param)</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> fprintf(stderr,"%s", PQerrorMessage(conn));</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> }</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> else</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;">+ {</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;"> deleted++;</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ if(param->transaction_limit!=0 &&deleted>=param->transaction_limit)</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">+ break;</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ }</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;"> }</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;"> else</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;">+ {</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;"> deleted++;</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ if(param->transaction_limit!=0&& deleted>=param->transaction_limit)</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">+ break;</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ }</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> }</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> PQclear(res);</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> </span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">@@-313,6 +322,7 @@ usage(const char *progname)</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;"> printf(" -h HOSTNAME database server host or socket directory\n");</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> printf(" -n don't remove large objects, just show what would be done\n");</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> printf(" -p PORT database server port\n");</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">+ printf(" -l LIMIT stop after removing LIMIT LOs\n");</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;"> printf(" -U USERNAME user name to connectas\n");</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> printf(" -w never prompt for password\n");</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;"> printf(" -W force password prompt\n");</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">@@ -342,6+352,7 @@ main(int argc, char **argv)</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> param.pg_port = NULL;</span><br style="font-family: courier new,monospace;"/><span style="font-family: courier new,monospace;"> param.verbose = 0;</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> param.dry_run = 0;</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ param.transaction_limit= 0;</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> </span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> if (argc > 1)</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;"> {</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">@@-359,7 +370,7 @@ main(int argc, char **argv)</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;"> </span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> while (1)</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> {</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">- c = getopt(argc, argv, "h:U:p:vnwW");</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">+ c = getopt(argc, argv, "h:U:p:l:vnwW");</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> if (c == -1)</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;"> break;</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> </span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">@@ -395,6 +406,14 @@ main(int argc, char **argv)</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> }</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;"> param.pg_port = strdup(optarg);</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;"> break;</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;">+ case 'l':</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;">+ param.transaction_limit = strtol(optarg, NULL, 10);</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ if ((param.transaction_limit < 0) || (param.transaction_limit > 2147483647))</span><br style="font-family: couriernew,monospace;" /><span style="font-family: courier new,monospace;">+ {</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;">+ fprintf(stderr,"%s: invalid transaction limit number: %s, valid range is form 0(disabled) to 2147483647.\n", progname, optarg);</span><brstyle="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ exit(1);</span><br style="font-family: courier new,monospace;" /><span style="font-family: courier new,monospace;">+ }</span><br style="font-family: courier new,monospace;" /><span style="font-family: couriernew,monospace;">+ break;</span><br style="font-family: courier new,monospace;" /><span style="font-family:courier new,monospace;"> case 'h':</span><br style="font-family: courier new,monospace;" /><spanstyle="font-family: courier new,monospace;"> param.pg_host = strdup(optarg);</span><br style="font-family:courier new,monospace;" /><span style="font-family: courier new,monospace;"> break;</span><brstyle="font-family: courier new,monospace;" /><br /><br />
On Mon, Jul 25, 2011 at 9:37 AM, Tim <elatllat@gmail.com> wrote: > Updated the patch to also apply when the no-action flag is enabled. You may want to read this: http://wiki.postgresql.org/wiki/Submitting_a_Patch And add your patch here: https://commitfest.postgresql.org/action/commitfest_view/open -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Here's another small change to the patch, it works fine for me and it quite saved my day. I try to submit the patch by email. diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c index f6e2a28..1f88d72 100644 --- a/contrib/vacuumlo/vacuumlo.c +++ b/contrib/vacuumlo/vacuumlo.c @@ -48,6 +48,7 @@ struct _param char *pg_host; int verbose; int dry_run; + int transaction_limit;};int vacuumlo(char *, struct _param *); @@ -286,6 +287,8 @@ vacuumlo(char *database, struct _param * param) } else deleted++; + if(param->transaction_limit!=0 && deleted>=param->transaction_limit) + break; } PQclear(res); @@ -313,6 +316,7 @@ usage(const char *progname) printf(" -h HOSTNAME database server host or socket directory\n"); printf(" -n don't remove large objects, just show what would be done\n"); printf(" -p PORT database server port\n"); + printf(" -l LIMIT stop after removing LIMIT LOs\n"); printf(" -U USERNAME user name to connect as\n"); printf(" -w never prompt for password\n"); printf(" -W force password prompt\n"); @@ -342,6 +346,7 @@ main(int argc, char **argv) param.pg_port = NULL; param.verbose = 0; param.dry_run = 0; + param.transaction_limit = 0; if (argc > 1) { @@ -359,7 +364,7 @@ main(int argc, char **argv) while (1) { - c = getopt(argc, argv, "h:U:p:vnwW"); + c = getopt(argc, argv, "h:U:p:l:vnwW"); if (c == -1) break; @@ -395,6 +400,14 @@ main(int argc, char **argv) } param.pg_port = strdup(optarg); break; + case 'l': + param.transaction_limit = strtol(optarg, NULL, 10); + if ((param.transaction_limit < 0) || (param.transaction_limit > 2147483647)) + { + fprintf(stderr, "%s: invalid transaction limit number: %s, valid range is form 0(disabled) to 2147483647.\n", progname, optarg); + exit(1); + } + break; case 'h': param.pg_host = strdup(optarg); break; -- View this message in context: http://postgresql.1045698.n5.nabble.com/vacuumlo-patch-tp4628522p4634026.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
Excerpts from Aron's message of mar jul 26 04:18:55 -0400 2011: > Here's another small change to the patch, it works fine for me and it quite > saved my day. > > I try to submit the patch by email. There's a problem with this patch: long lines are being wrapped by your email client, which makes it impossible to apply. I think it would be safer if you sent it as an attachment instead of pasting it in the body of the message. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Hi Tim,
you correctly assumed I was just trying to clean up the code. There was no reason to change the behavior, so please ignore my change to the patch.
Aron
On 26.07.2011, at 15:44, Tim Lewis wrote:
Hi Aron,
Thanks for the input. The "small change" you suggest would change the behavior of the patch which I would prefer not to do as I have reasons for the previous behavior.
Because you gave no reasons and "stop after removing LIMIT LOs" was not changed to "stop after attempting to remove LIMIT LOs" I suspect you were just trying to keep the code clean.
The difference:
In your version of the patch vacuumlo will stop after N lo_unlink(OID) attempts.
The previous behavior of the patch is that vacuumlo will stop after N successful lo_unlink(OID)s.
If you have good reason for your behavior please add another flag so that it is optional.
There should be a clear distinction between "counting vs not", and "aborting vs continuing" when a lo_unlink(OID) is unsuccessful.On Tue, Jul 26, 2011 at 4:18 AM, Aron <me@eunice.de> wrote:Here's another small change to the patch, it works fine for me and it quite
saved my day.
I try to submit the patch by email.index f6e2a28..1f88d72 100644
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c--- a/contrib/vacuumlo/vacuumlo.c@@ -286,6 +287,8 @@ vacuumlo(char *database, struct _param * param)
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -48,6 +48,7 @@ struct _param
char *pg_host;
int verbose;
int dry_run;
+ int transaction_limit;
};
int vacuumlo(char *, struct _param *);}@@ -313,6 +316,7 @@ usage(const char *progname)
else
deleted++;
+ if(param->transaction_limit!=0 &&
deleted>=param->transaction_limit)
+ break;
}
PQclear(res);printf(" -h HOSTNAME database server host or socket directory\n");@@ -342,6 +346,7 @@ main(int argc, char **argv)
printf(" -n don't remove large objects, just show what would be
done\n");
printf(" -p PORT database server port\n");
+ printf(" -l LIMIT stop after removing LIMIT LOs\n");
printf(" -U USERNAME user name to connect as\n");
printf(" -w never prompt for password\n");
printf(" -W force password prompt\n");param.pg_port = NULL;@@ -359,7 +364,7 @@ main(int argc, char **argv)
param.verbose = 0;
param.dry_run = 0;
+ param.transaction_limit = 0;
if (argc > 1)
{@@ -395,6 +400,14 @@ main(int argc, char **argv)
while (1)
{
- c = getopt(argc, argv, "h:U:p:vnwW");
+ c = getopt(argc, argv, "h:U:p:l:vnwW");
if (c == -1)
break;}--
param.pg_port = strdup(optarg);
break;
+ case 'l':
+ param.transaction_limit = strtol(optarg, NULL, 10);
+ if ((param.transaction_limit < 0) || (param.transaction_limit >
2147483647))
+ {
+ fprintf(stderr, "%s: invalid transaction limit number: %s, valid
range is form 0(disabled) to 2147483647.\n", progname, optarg);
+ exit(1);
+ }
+ break;
case 'h':
param.pg_host = strdup(optarg);
break;
View this message in context: http://postgresql.1045698.n5.nabble.com/vacuumlo-patch-tp4628522p4634026.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
NoodleConnecting People, Content & Capabilities within the Enterprise
Toll Free: 866-258-6951 x 701
Tim.Lewis@vialect.com
http://www.vialect.com
Noodle is a product of Vialect Inc
Follow Noodle on Twitter
http://www.twitter.com/noodle_news
Hi Tim, I have to correct my previous answer, my change does not alter the behavior of your patch significantly. > The difference: > In your version of the patch vacuumlo will stop after N lo_unlink(OID) attempts. > The previous behavior of the patch is that vacuumlo will stop after N successful lo_unlink(OID)s. > > If you have good reason for your behavior please add another flag so that it is optional. > There should be a clear distinction between "counting vs not", and "aborting vs continuing" when a lo_unlink(OID) is unsuccessful. if (param->dry_run == 0) { if (lo_unlink(conn, lo) < 0) { fprintf(stderr, "\nFailed to remove lo %u: ", lo); fprintf(stderr, "%s", PQerrorMessage(conn)); } else deleted++; } else deleted++; if(param->transaction_limit!=0&& deleted>=param->transaction_limit) break; The variable "deleted" is only incremented if a lo_unlink was successful, so my patch only introduces a negligible overheadbut no actual change in behavior. I'm very grateful for your patch and I think it should be accepted as soon as possible, one or two "if" does not matter tome. Aron
> Excerpts from Aron's message of mar jul 26 04:18:55 -0400 2011: >> Here's another small change to the patch, it works fine for me and it quite >> saved my day. >> >> I try to submit the patch by email. > > There's a problem with this patch: long lines are being wrapped by > your email client, which makes it impossible to apply. I think it would > be safer if you sent it as an attachment instead of pasting it in the > body of the message. I posted using nabble, edited the post and uploaded the file as an attachment. But here you go again, this time as an attachment.
Attachment
Hi Aron,<br /><br />Thanks for the input. The "small change" you suggest would change the behavior of the patch which I wouldprefer not to do as I have reasons for the previous behavior.<br />Because you gave no reasons and "stop after removingLIMIT LOs" was not changed to "stop after attempting to remove LIMIT LOs" I suspect you were just trying to keepthe code clean.<br /><br />The difference:<br />In your version of the patch vacuumlo will stop after N lo_unlink(OID)attempts.<br />The previous behavior of the patch is that vacuumlo will stop after N successful lo_unlink(OID)s.<br/><br />If you have good reason for your behavior please add another flag so that it is optional.<br />There should be a clear distinction between "counting vs not", and "aborting vs continuing" when a lo_unlink(OID) is unsuccessful.<br/><br /><br /><div class="gmail_quote">On Tue, Jul 26, 2011 at 4:18 AM, Aron <span dir="ltr"><<a href="mailto:me@eunice.de">me@eunice.de</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px#ccc solid;padding-left:1ex;">Here's another small change to the patch, it works fine for me and it quite<br/> saved my day.<br /><br /> I try to submit the patch by email.<br /><div class="im"><br /><br /> diff --git a/contrib/vacuumlo/vacuumlo.cb/contrib/vacuumlo/vacuumlo.c<br /></div>index f6e2a28..1f88d72 100644<br /><div class="im">---a/contrib/vacuumlo/vacuumlo.c<br /> +++ b/contrib/vacuumlo/vacuumlo.c<br /> @@ -48,6 +48,7 @@ struct _param<br/> char *pg_host;<br /> int verbose;<br /> int dry_run;<br /> + int transaction_limit;<br /> };<br /><br /> int vacuumlo(char*, struct _param *);<br /></div>@@ -286,6 +287,8 @@ vacuumlo(char *database, struct _param * param)<br /><divclass="im"> }<br /> else<br /> deleted++;<br /> + if(param->transaction_limit!=0 &&<br /> deleted>=param->transaction_limit)<br /> + break;<br /> }<br /> PQclear(res);<br /><br /></div>@@ -313,6 +316,7 @@ usage(const char *progname)<br /><divclass="im"> printf(" -h HOSTNAME database server host or socket directory\n");<br /> printf(" -n don't remove large objects, just show what would be<br /> done\n");<br /> printf(" -p PORT databaseserver port\n");<br /> + printf(" -l LIMIT stop after removing LIMIT LOs\n");<br /> printf(" -UUSERNAME user name to connect as\n");<br /> printf(" -w never prompt for password\n");<br /> printf(" -W force password prompt\n");<br /></div>@@ -342,6 +346,7 @@ main(int argc, char **argv)<br /><divclass="im"> param.pg_port = NULL;<br /> param.verbose = 0;<br /> param.dry_run = 0;<br /> + param.transaction_limit = 0;<br /><br /> if (argc > 1)<br /> {<br /></div>@@ -359,7 +364,7 @@ main(intargc, char **argv)<br /><div class="im"><br /> while (1)<br /> {<br /> - c = getopt(argc,argv, "h:U:p:vnwW");<br /> + c = getopt(argc, argv, "h:U:p:l:vnwW");<br /> if (c== -1)<br /> break;<br /><br /></div>@@ -395,6 +400,14 @@ main(int argc, char **argv)<br /><divclass="im"> }<br /> param.pg_port = strdup(optarg);<br/> break;<br /> + case 'l':<br /> + param.transaction_limit = strtol(optarg, NULL, 10);<br /> + if ((param.transaction_limit< 0) || (param.transaction_limit ><br /><a href="tel:2147483647" value="+12147483647">2147483647</a>))<br/> + {<br /> + fprintf(stderr,"%s: invalid transaction limit number: %s, valid<br /> range is form 0(disabled) to <a href="tel:2147483647"value="+12147483647">2147483647</a>.\n", progname, optarg);<br /> + exit(1);<br /> + }<br /> + break;<br /> case 'h':<br /> param.pg_host = strdup(optarg);<br /> break;<br /><br /><br /></div>--<br /> View this message in context: <a href="http://postgresql.1045698.n5.nabble.com/vacuumlo-patch-tp4628522p4634026.html" target="_blank">http://postgresql.1045698.n5.nabble.com/vacuumlo-patch-tp4628522p4634026.html</a><br/> Sent from the PostgreSQL- hackers mailing list archive at Nabble.com.<br /><font color="#888888"><br /> --<br /> Sent via pgsql-hackersmailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br /> To makechanges to your subscription:<br /><a href="http://www.postgresql.org/mailpref/pgsql-hackers" target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/></font></blockquote></div><br /><br clear="all"/><br />-- <br /><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"><div style="font-family:'ArialRounded MT Bold';color:rgb(11, 119, 176);font-size:26px;font-weight:bold"><br />Noodle</div><divstyle="font-family:Helvetica;font-size:12px"><div style="font-style:italic;color:rgb(0, 3, 5)">Connecting <spanstyle="font-weight:bold">People, Content & Capabilities</span> within the <span style="font-weight:bold">Enterprise</span><br/><br /></div><br />Toll Free: 866-258-6951 x 701<br /><a href="mailto:Tim.Lewis@vialect.com"style="color:rgb(17, 65, 112)" target="_blank">Tim.Lewis@vialect.com</a><br /><a href="http://www.vialect.com"style="color:rgb(17, 65, 112)" target="_blank">http://www.vialect.com</a><br /><br />Noodleis a product of Vialect Inc<br /><br />Follow Noodle on Twitter<br /><a href="http://www.twitter.com/noodle_news"style="color:rgb(17, 65, 112)" target="_blank">http://www.twitter.com/noodle_news</a></div></span><br/>
I'm not sure what David did for me so as per Roberts suggestion I have <a href="https://commitfest.postgresql.org/action/patch_view?id=609">added</a>this patch to the commit fest. <br />I'm hopingI have not put this patch in more than one <a href="http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_review_and_commit">workflow</a>.<br/><br /><div class="gmail_quote">OnMon, Jul 25, 2011 at 7:28 PM, Robert Haas <span dir="ltr"><<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">On Mon, Jul 25, 2011 at 9:37 AM, Tim<<a href="mailto:elatllat@gmail.com">elatllat@gmail.com</a>> wrote:<br /> > Updated the patch to also apply whenthe no-action flag is enabled.<br /><br /></div>You may want to read this:<br /><br /><a href="http://wiki.postgresql.org/wiki/Submitting_a_Patch" target="_blank">http://wiki.postgresql.org/wiki/Submitting_a_Patch</a><br/><br /> And add your patch here:<br /><br /><ahref="https://commitfest.postgresql.org/action/commitfest_view/open" target="_blank">https://commitfest.postgresql.org/action/commitfest_view/open</a><br/><font color="#888888"><br /> --<br/> Robert Haas<br /> EnterpriseDB: <a href="http://www.enterprisedb.com" target="_blank">http://www.enterprisedb.com</a><br/> The Enterprise PostgreSQL Company<br /></font></blockquote></div><br/>
On Tue, Jul 26, 2011 at 12:18:31PM -0400, Timothy D. F. Lewis wrote: > On Mon, Jul 25, 2011 at 7:28 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > > On Mon, Jul 25, 2011 at 9:37 AM, Tim <elatllat@gmail.com> wrote: > > > Updated the patch to also apply when the no-action flag is enabled. > > > > You may want to read this: > > > > http://wiki.postgresql.org/wiki/Submitting_a_Patch > > > > And add your patch here: > > > > https://commitfest.postgresql.org/action/commitfest_view/open > > > > -- > > Robert Haas > > EnterpriseDB: http://www.enterprisedb.com > > The Enterprise PostgreSQL Company > > I'm not sure what David did for me so as per Roberts suggestion I have > added<https://commitfest.postgresql.org/action/patch_view?id=609>this > patch to the commit fest. > I'm hoping I have not put this patch in more than one > workflow<http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_review_and_commit> > . I just put your name as you've asked me to phrase it into the "pending patches" part of the PostgreSQL Weekly News. Thanks for the contribution, and here's hoping your experience here will be pleasant and productive :) Cheers, David. p.s. We don't top-post on the PostgreSQL mailing lists. As with the customary "reply-to-all," it's a convention we've decided works well for us. :) -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Jul 26, 2011 at 12:18 PM, Timothy D. F. Lewis <elatllat@gmail.com> wrote: > I'm not sure what David did for me so as per Roberts suggestion I have added > this patch to the commit fest. > I'm hoping I have not put this patch in more than one workflow. Hi Tim, I would be willing to review this patch for the next CommitFest. I'd like to request that you send an updated version of your patch *as an attachment* to avoid the problems with long lines getting automatically wrapped, as Alvaro mentioned. I had trouble getting the existing patches to apply. A few preliminary comments about the patch: 1. It wasn't clear to me whether you're OK with Aron's suggested tweak, please include it in your patch if so. 2. I think it might be better to use INT_MAX instead of hardcoding 2147483647. 3. Your patch has some minor code style differences wrt. the existing code, e.g. + if(param->transaction_limit!=0 && deleted>=param->transaction_limit) should have a space before the first '(' and around the '!=' and '>=' 4. The rest of the usage strings spell out 'large object(s)' instead of abbreviating 'LO' + printf(" -l LIMIT stop after removing LIMIT LOs\n"); 5. transaction_limit is an int, yet you're using strtol() which returns long. Maybe you want to use atoi() or make transaction_limit a long? Thanks Josh
Hi Josh,
Thanks for help. Attached is a patch including changes suggested in your comments.
Thanks for help. Attached is a patch including changes suggested in your comments.
Excerpts from Josh's message On Sat, Aug 6, 2011 at 9:57 PM:
I decided to and included Aron's tweak.
I'm not sure if the PostgreSQL project prefers simplified code over faster run time (if only under rare conditions).
Who knows maybe the compiler will re-optimize it anyway.
Fixed, though I'm not sure if I put the include in the preferred order.
Fixed.
Fixed, I omitted the brackets around the s to conform with the other usage strings.
The other INT in the code is using strtol() so I also used strtol instead of changing more code.
I'm not sure if there are any advantages or disadvantages.
maybe it's easier to prevent/detect/report overflow wraparound.
I can't think of a good reason anyone would want to limit transaction to something more than INT_MAX.
Implementing that would best be done in separate and large patch as PQntuples returns an int and is used on 271 lines across PostgreSQL.
Thanks again for the help.
1. It wasn't clear to me whether you're OK with Aron's suggested
tweak, please include it in your patch if so.
I decided to and included Aron's tweak.
I'm not sure if the PostgreSQL project prefers simplified code over faster run time (if only under rare conditions).
Who knows maybe the compiler will re-optimize it anyway.
2. I think it might be better to use INT_MAX instead of hardcoding 2147483647.
Fixed, though I'm not sure if I put the include in the preferred order.
3. ... should have a space before the first '(' and around the '!=' and '>='
Fixed.
4. The rest of the usage strings spell out 'large object(s)' instead
of abbreviating 'LO'...
Fixed, I omitted the brackets around the s to conform with the other usage strings.
5. transaction_limit is an int, yet you're using strtol() which
returns long. Maybe you want to use atoi() or make transaction_limit a
long?
The other INT in the code is using strtol() so I also used strtol instead of changing more code.
I'm not sure if there are any advantages or disadvantages.
maybe it's easier to prevent/detect/report overflow wraparound.
I can't think of a good reason anyone would want to limit transaction to something more than INT_MAX.
Implementing that would best be done in separate and large patch as PQntuples returns an int and is used on 271 lines across PostgreSQL.
Thanks again for the help.
Attachment
On Sun, Aug 7, 2011 at 12:41 AM, Tim <elatllat@gmail.com> wrote: > Hi Josh, > > Thanks for help. Attached is a patch including changes suggested in your > comments. > > Excerpts from Josh's message On Sat, Aug 6, 2011 at 9:57 PM: >> >> 1. It wasn't clear to me whether you're OK with Aron's suggested >> tweak, please include it in your patch if so. > > > I decided to and included Aron's tweak. > I'm not sure if the PostgreSQL project prefers simplified code over faster > run time (if only under rare conditions). > Who knows maybe the compiler will re-optimize it anyway. Thanks for the quick update. It was pretty hard for me to compare your initial versions with Aron's, since I had trouble with those patches. But if it's just a minor change to how transaction_limit is handled, I wouldn't worry about it; the vast majority of vacuumlo's time is going to be spent in the database AFAICT. Incidentally, if someone wants to optimize vacuumlo, I see some low-hanging fruit there, such as maybe avoiding that expensive loop of DELETEs out of vacuum_l entirely with a bit smarter queries. But that's a problem for another day. >> 2. I think it might be better to use INT_MAX instead of hardcoding >> 2147483647. > > Fixed, though I'm not sure if I put the include in the preferred order. Hrm yeah.. maybe keep it so that all the system headers are together (i.e. put <limits.h> after <fcntl.h>). At least, that's how similar header files like pg_upgrade.h seem to be structured. >> 5. transaction_limit is an int, yet you're using strtol() which >> returns long. Maybe you want to use atoi() or make transaction_limit a >> long? > > The other INT in the code is using strtol() so I also used strtol instead of > changing more code. > I'm not sure if there are any advantages or disadvantages. > maybe it's easier to prevent/detect/report overflow wraparound. Ugh, yeah you're right. I think this vacuumlo.c code is not such a great role model for clean code :-) Probably not a big deal, since you are checking for param.transaction_limit < 0 which would detect overflow. I'm not sure if the other half of that check, (param.transaction_limit > INT_MAX) has any meaning, i.e. how can an int ever be > INT_MAX? > I can't think of a good reason anyone would want to limit transaction to > something more than INT_MAX. > Implementing that would best be done in separate and large patch as > PQntuples returns an int and is used on 271 lines across PostgreSQL. Right, I wasn't suggesting that would actually be useful - I just thought the return type of strtol() or atoi() should agree with its lvalue. I've only spent a little bit of time with this patch and LOs so far, but one general question/idea I have is whether the "-l LIMIT" option could be made smarter in some way. Say, instead of making the user figure out how many LOs he can feasibly delete in a single transaction (how is he supposed to know anyway, trial and error?) could we figure out what that limit should be based on max_locks_per_transaction? and handle deleting all the ophan LOs in several transactions for the user automatically? Josh
On sön, 2011-08-07 at 00:41 -0400, Tim wrote: > Thanks for help. Attached is a patch including changes suggested in your > comments. Please put the new option 'l' in some sensible order in the code and the help output (normally alphabetical). Also, there should probably be some update to the documentation.
Excerpts from Josh's message On Sun, Aug 7, 2011 at 2:36 AM:
could we figure out what that limit should be based on max_locks_per_transaction?
It would be nice to implement via "-l max" instead of making users do it manually or something like this "-l $(grep "max_locks_per_transaction.*=" postgresql.conf | perl -p -e 's/#.*//g;s/^.*?([0-9]+).*?$/$1/g' | grep . |head -1 )".
For this patch I just want to enable the limit functionality, leaving higher level options like max to the user for now.
handle deleting all the ophan LOs in several transactions for the user automatically?
I addressed this option before and basically said it is an undesirable alternative because It is a less flexible option that is easily implemented in a shell script.
Again it would be a welcome extra option but it can be left to the user for now.
Excerpts from Peter's message On Sun, Aug 7, 2011 at 3:49 AM:
I have alphabetized the help output, and one piece of code.
I'm hesitate to alphabetize some portions of the code because they are grouped by type and not already alphabetized.
If I left something un-alphabetized please be explicit about about what should be changed and why.
Documentation is now also in the patch.
Thanks for the tips.
Please put the new option 'l' in some sensible order in the code and the
help output (normally alphabetical). Also, there should probably be
some update to the documentation.
I have alphabetized the help output, and one piece of code.
I'm hesitate to alphabetize some portions of the code because they are grouped by type and not already alphabetized.
If I left something un-alphabetized please be explicit about about what should be changed and why.
Documentation is now also in the patch.
Thanks for the tips.
Attachment
On Sun, Aug 7, 2011 at 3:54 AM, Tim <elatllat@gmail.com> wrote: > > Excerpts from Josh's message On Sun, Aug 7, 2011 at 2:36 AM: >> >> could we figure out what that limit should be based on >> max_locks_per_transaction? > > It would be nice to implement via "-l max" instead of making users do it > manually or something like this "-l $(grep "max_locks_per_transaction.*=" > postgresql.conf | perl -p -e 's/#.*//g;s/^.*?([0-9]+).*?$/$1/g' | grep . > |head -1 )". > For this patch I just want to enable the limit functionality, leaving higher > level options like max to the user for now. > >> >> handle deleting all the ophan LOs in several transactions for the user >> automatically? > > I addressed this option before and basically said it is an undesirable > alternative because It is a less flexible option that is easily implemented > in a shell script. > Again it would be a welcome extra option but it can be left to the user for > now. As a preface, I appreciate the work you're putting into this module, and I am all for keeping the scope of this change as small as possible so that we actually get somewhere. Having said that, it's a bit unfortunate that this module seems to be rather neglected, and has some significant usability problems. For instance, if you do blow out the max_locks_per_transaction limit, you get a very reasonable error message and hint like: Failed to remove lo 44459: ERROR: out of shared memory HINT: You might need to increase max_locks_per_transaction. Unfortunately, the code doesn't 'break;' after that, it just keeps plowing through the lo_unlink() calls, generating a ton of rather unhelpful messages like: Failed to remove lo 47087: ERROR: current transaction is aborted, commands ignored until end of transaction block which clog up stderr, and make it easy to miss the one helpful error message at the beginning. Now, here's where your patch might really help things with a minor adjustment. How about structuring that lo_unlink() call so that users will see only a reasonably helpful error message if they happen to come across this problem, like this in non-verbose mode: WARNING: out of shared memory Failed to remove lo 46304: ERROR: out of shared memory HINT: You might need to increase max_locks_per_transaction. Bailing out. Try using -l LIMIT flag, with a LIMIT of 1845 (Side note: I was asking upthread about how to figure out what LIMIT value to use, because I don't understand how max_locks_per_transaction relates to the number of lo_unlink() calls one can make in a transaction... I seem to be able use a limit of 1845, but 1846 will error out, with max_locks_per_transaction = 64. Anyone know why this is?) A related problem I noticed that's not really introduced by your script, but which might easily be fixed, is the return value from vacuumlo(). The connection and query failures at the top of the function all return -1 upon failure, but if an lo_unlink() call fails and the entire transaction gets rolled back, vacuumlo() happily returns 0. I've put together an updated version of your patch (based off your latest version downstream with help output alphabetized) showing how I envision these two problems being fixed. Also, a small adjustment to your SGML changes to blend in better. Let me know if that all looks OK or if you'd rather handle things differently. The new error messages might well need some massaging. I didn't edit the INT_MAX stuff either, will leave that for you. Josh
Attachment
Thanks Josh,<br />I like the ability to bail out on PQTRANS_INERROR, and I think it's a small enough fix to be appropriateto include in this patch.<br />I did consider it before but did not implement it because I am still new to pgsql-hackersand did not know how off-the-cuff.<br /> So thanks for the big improvement.<br />
On Sun, Aug 7, 2011 at 7:53 PM, Tim <elatllat@gmail.com> wrote: > Thanks Josh, > I like the ability to bail out on PQTRANS_INERROR, and I think it's a small > enough fix to be appropriate to include in this patch. > I did consider it before but did not implement it because I am still new to > pgsql-hackers and did not know how off-the-cuff. > So thanks for the big improvement. I've committed this patch with some changes, mostly cosmetic. One not-quite-so-cosmetic change is that I removed the suggestion that -l should be used with a limit one lower than whatever provoked the previous failure. That might be true on a completely idle system, but is an oversimplification in real life. Maybe some kind of hint is appropriate here, but I think if we're going to have one it ought to be more generically worded. I also updated the failure error message to avoid saying that we "failed to remove NNN objects". Instead, it now says how many objects it wanted to remove and how far it had gotten when it failed, which I think is more useful. Please feel free to propose further patches if you don't like what I've done here, or want to further build on it or fine-tune... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company