On Fri, Feb 5, 2016 at 12:53 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>> Something is wrong with patch d.  I noticed two things,
>> 1. the total_weight stuff can overflow,
> It can generate an error on overflow by checking the total_weight while it
> is being computed. I've switched total_weight to int64 so it is now really
> impossible to overflow with the 32 bit int_max limit on weight.

+       /* compute total_weight */
+       for (i = 0; i < num_scripts; i++)
+       {
+               total_weight += sql_script[i].weight;
+               /* detect overflow... */
+               if (total_weight < 0)
+               {
+                       fprintf(stderr, "script weight overflow at
script %d\n", i+1);
+                       exit(1);
+               }
+       }
If let as int64, you may want to remove this overflow check, or keep
it as int32.

>> 2. the chooseScript stuff is broken, or something.
> Sorry, probably a <=/< error. I think I've fixed it and I've simplified the
> code a little bit.

+       w = getrand(thread, 0, total_weight - 1);
+       do
+       {
+               w -= sql_script[i++].weight;
+       } while (w >= 0);
+       return i - 1;
This portion looks fine to me.

>> Another thing is that the "transaction type" output really deserves some
>> more work.  I think "multiple scripts" really doesn't cut it; we should
>> have some YAML-like as in the latency reports, which lists all scripts
>> in use and their weights.
> For me the current output is clear for the reader, which does not mean that
> it cannot be improve, but how is rather a matter of taste.
> I've tried to improve it further, see attached patch.
> If you want something else, it would help to provide a sample of what you
> expect.

You could do that with an additional option here as well:
--output-format=normal|yamljson. The tastes of each user is different.
>> Attached is my version of the patch.  While you're messing with it, it'd
>> be nice if you added comments on top of your recently added functions
>> such as findBuiltin, process_builtin, chooseScript.
> Why not.

        const char *name;
+       int                     weight;
        Command   **commands;
-       StatsData stats;
+       StatsData       stats;
Noise here?

> Find attached a 18-d which addresses these concerns, and a actualized 18-e
> for the prefix.

(I could live without that personally)

-/* return builtin script "name", or fail if not found */
+/* return commands for selected builtin script, if unambiguous */
 static script_t *
 findBuiltin(const char *name)
This comment needs a refresh. This does not return a set of commands,
but the script itself.

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to