Re: vacuumlo patch

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