Re: vacuumlo patch

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)

*Solution*: ask vacuumlo to complete after N lo_unlink(OID)s.
*
Alternate Solution*: ask vacuumlo to start a new transaction after N
lo_unlink(OID)s.
(more complex and can be implemented with the other solution in the shell)

*Design Decisions*: Add a flag so the new behavior is optional and it
defaults to the old behavior.

*Implementation*: Followed code formatting style.
It's likely pointless to check if an int is > 2147483647 but maybe it
clarifies the code.
Added a friendly error message and the a line in the help.

git diff -U0 HEAD -- contrib/vacuumlo/vacuumlo.c
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index f6e2a28..b7c7d64 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -50,0 +51 @@ struct _param
+ int transaction_limit;
@@ -284,0 +286 @@ vacuumlo(char *database, struct _param * param)
+ {
@@ -285,0 +288,5 @@ vacuumlo(char *database, struct _param * param)
+ if(param->transaction_limit!=0 &&
deleted>=param->transaction_limit)
+ {
+ break;
+ }
+ }
@@ -315,0 +323 @@ usage(const char *progname)
+ printf(" -l LIMIT stop after removing LIMIT LOs\n");
@@ -344,0 +353 @@ main(int argc, char **argv)
+ param.transaction_limit = 0;
@@ -362 +371 @@ main(int argc, char **argv)
- c = getopt(argc, argv, "h:U:p:vnwW");
+ c = getopt(argc, argv, "h:U:p:l:vnwW");
@@ -397,0 +407,8 @@ main(int argc, char **argv)
+ 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;

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi Álvaro, thanks for the response.

Here is the requested "diff with 3 lines of context":

git diff HEAD -- contrib/vacuumlo/vacuumlo.c
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index f6e2a28..b7c7d64 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 *);
@@ -282,7 +283,13 @@ vacuumlo(char *database, struct _param * param)
fprintf(stderr, "%s", PQerrorMessage(conn));
}
else
+ {
deleted++;
+ if(param->transaction_limit!=0 &&
deleted>=param->transaction_limit)
+ {
+ break;
+ }
+ }
}
else
deleted++;
@@ -313,6 +320,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 +350,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 +368,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 +404,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;

Updated the patch to also apply when the no-action flag is enabled.

git diff HEAD -- contrib/vacuumlo/vacuumlo.c
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index f6e2a28..8e9c342 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 *);
@@ -282,10 +283,18 @@ vacuumlo(char *database, struct _param * param)
fprintf(stderr, "%s", PQerrorMessage(conn));
}
else
+ {
deleted++;
+ if(param->transaction_limit!=0 &&
deleted>=param->transaction_limit)
+ break;
+ }
}
else
+ {
deleted++;
+ if(param->transaction_limit!=0 &&
deleted>=param->transaction_limit)
+ break;
+ }
}
PQclear(res);

@@ -313,6 +322,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 +352,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 +370,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 +406,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;

On Mon, Jul 25, 2011 at 9:37 AM, Tim <elatllat(at)gmail(dot)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.

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(at)eunice(dot)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.
>
>
> 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.
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

--

Noodle
Connecting People, Content & Capabilities within the Enterprise

Toll Free: 866-258-6951 x 701
Tim(dot)Lewis(at)vialect(dot)com
http://www.vialect.com

Noodle is a product of Vialect Inc

Follow Noodle on Twitter
http://www.twitter.com/noodle_news

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(at)commandprompt(dot)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(at)eunice(dot)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.
>
>
> 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.
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
>
> --
>
> Noodle
> Connecting People, Content & Capabilities within the Enterprise
>
>
> Toll Free: 866-258-6951 x 701
> Tim(dot)Lewis(at)vialect(dot)com
> http://www.vialect.com
>
> Noodle is a product of Vialect Inc
>
> Follow Noodle on Twitter
> http://www.twitter.com/noodle_news
>

> 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 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 overhead but 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 to me.

Aron

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

On Mon, Jul 25, 2011 at 7:28 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jul 25, 2011 at 9:37 AM, Tim <elatllat(at)gmail(dot)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
>

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(at)gmail(dot)com> wrote:
>
> > On Mon, Jul 25, 2011 at 9:37 AM, Tim <elatllat(at)gmail(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)gmail(dot)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.

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.

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

On Sun, Aug 7, 2011 at 12:41 AM, Tim <elatllat(at)gmail(dot)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:

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

On Sun, Aug 7, 2011 at 3:54 AM, Tim <elatllat(at)gmail(dot)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,
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.

On Sun, Aug 7, 2011 at 7:53 PM, Tim <elatllat(at)gmail(dot)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