Re: vacuumlo patch

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