Обсуждение: 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,

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.

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

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.

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

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