Re: pgbench stats per script & other stuff - Mailing list pgsql-hackers

Hello Alvaro,

>>> If somebody specifies thousands of -f switches, they will waste a few
>>> bytes with each, but I'm hardly concerned about a few dozen kilobytes
>>> there ...
>>
>> Ok, so you prefer a memory leak. I hate it on principle.
>
> I don't "prefer" memory leaks -- I prefer interfaces that make sense.

C is not designed to return two things, and if it is what is needed it 
looks awkward whatever is done. The static variable trick is dirty, but it 
is the minimal fuss solution, IMO. So we are only trading awkward code 
against awkward code.

> Speaking of which, I don't think the arrangement in your patch really
> does.  I know I suggested it,

Yep:-)

> but now that I look again, it turns out I chose badly and you 
> implemented a bad idea, so can we go back and fix it, please?

Yep.

I have very little time available, so I'm trying to minimize the effort. 
I've tried "argue my point with committers", but it has proven very 
ineffective. I've switched to "do whatever is asked if it still works", 
but it is not very effective either.

> What I now think should really happen is that the current sql_scripts
> array, currently under an anonymous struct, should be a typedef, say
> ParsedScript,

Why not.

> and get a new member for the weight;

Hm... it already contains "weight".

> process_file and process_builtin return a ParsedScript.  The weight and 
> Command ** should not be part of script_t at all.

Sure.

> In fact, with ParsedScript I don't think we need to give a name to the 
> anon struct used for builtin scripts.

It is useful that it has a name so that find_builtin can return it.

> Rename the current sql_scripts.name to "desc", to mirror what
> is actually put in there from the builtin array struct.  Make addScript
> receive a ParsedScript and weight, fill in the weight into the struct,
> and put it to the array after sanity-checking.  (I'm OK with keeping
> "name" instead of renaming to "desc", if that change becomes too
> invasive.)

See attached a v24 & v25.

The awkwardness in v24 is that functions allocate a struct which is freed 
afterwards, really just to return two data. Whether it is better or worst 
than a static is really a matter of taste.

Version v25 results a script which is then passed as an argument, so it 
avoid the dynamic allocation & later free. Maybe it is better. I had to 
cut short the error handling if a file does not exists, though, and it 
passes a struct by value.

Feel free to pick whichever you like most.

> No need for N_BUILTIN; we can use lengthof(builtin_script) instead.

Indeed. "lengthof" does not seem to be standard... ok, it is a macro in 
some header file. I really wanted to avoid an ugly sizeof divide hack, but 
as it is hidden elsewhere this is fine.

-- 
Fabien.