Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Round off random_normal() test results one more decimal place.

  2. Remove pg_regress' never-documented "ignore" feature.

  3. Upgrade the random.sql regression test.

  4. Invent random_normal() to provide normally-distributed random numbers.

  1. [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-08T21:53:23Z

    Just a utility function to generate random numbers from a normal
distribution. I find myself doing this several times a year, and I am
sure I must not be the only one.

random_normal(stddev float8 DEFAULT 1.0, mean float8 DEFAULT 0.0)

  2. Re: [PATCH] random_normal function

    David G. Johnston <david.g.johnston@gmail.com> — 2022-12-08T22:40:59Z

    On Thu, Dec 8, 2022 at 2:53 PM Paul Ramsey <pramsey@cleverelephant.ca>
wrote:

>
> random_normal(stddev float8 DEFAULT 1.0, mean float8 DEFAULT 0.0)
>

Any particular justification for placing stddev before mean?  A brief
survey seems to indicate other libraries, as well as (at least for me)
learned convention, has the mean be supplied first, then the standard
deviation.  The implementation/commentary seems to use that convention as
well.

Some suggestions:

/* Apply optional user parameters */ - that isn't important or even what is
happening though, and the body of the function shouldn't care about the
source of the values for the variables it uses.

Instead:
/* Transform the normal standard variable (z) using the target normal
distribution parameters */

Personally I'd probably make that even more explicit:

+ float8    z
...
* z = pg_prng_double_normal(&drandom_seed)
+ /* ... */
* result = (stddev * z) + mean

And a possible micro-optimization...

+ bool   rescale = true
+ if (PG_NARGS() = 0)
+    rescale = false
...
+ if (rescale)
    ... result = (stddev * z) + mean
+ else
+      result = z

David J.

  3. Re: [PATCH] random_normal function

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-08T22:46:23Z

    On Thu, Dec 08, 2022 at 01:53:23PM -0800, Paul Ramsey wrote:
> Just a utility function to generate random numbers from a normal
> distribution. I find myself doing this several times a year, and I am
> sure I must not be the only one.
> 
> random_normal(stddev float8 DEFAULT 1.0, mean float8 DEFAULT 0.0)

+++ b/src/backend/catalog/system_functions.sql
@@ -620,6 +620,13 @@ CREATE OR REPLACE FUNCTION
  STABLE PARALLEL SAFE
  AS 'sql_localtimestamp';
 
+CREATE OR REPLACE FUNCTION
+  random_normal(stddev float8 DEFAULT 1.0, mean float8 DEFAULT 0.0)
+RETURNS float8
+LANGUAGE INTERNAL
+STRICT VOLATILE PARALLEL SAFE
+AS 'make_interval';

I guess make_interval is a typo ?

This is causing it to fail tests:
http://cfbot.cputube.org/paul-ramsey.html

BTW you can run the same tests as CFBOT does from your own github
account; see:
https://www.postgresql.org/message-id/20221116232507.GO26337@telsasoft.com

-- 
Justin

  4. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-08T22:57:34Z

    
> On Dec 8, 2022, at 2:40 PM, David G. Johnston <david.g.johnston@gmail.com> wrote:
> 
> On Thu, Dec 8, 2022 at 2:53 PM Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
> random_normal(stddev float8 DEFAULT 1.0, mean float8 DEFAULT 0.0)
> 
> Any particular justification for placing stddev before mean?  A brief survey seems to indicate other libraries, as well as (at least for me) learned convention, has the mean be supplied first, then the standard deviation.  The implementation/commentary seems to use that convention as well.

No, I'm not sure what was going through my head, but I'm sure it made sense at the time (maybe something like "people will tend to jimmy with the stddev more frequently than the mean"). I've reversed the order

> Some suggestions:

Thanks! Taken :)

> And a possible micro-optimization...
> 
> + bool   rescale = true
> + if (PG_NARGS() = 0)
> +    rescale = false
> ...
> + if (rescale)
>     ... result = (stddev * z) + mean
> + else
> +      result = z

Feels a little too micro to me (relative to the overall cost of the transform from uniform to normal distribution). I'm going to leave it out unless you violently want it.

Revised patch attached.

Thanks!

P

  5. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-08T22:58:02Z

    
> On Dec 8, 2022, at 2:46 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> 
> I guess make_interval is a typo ?
> 
> This is causing it to fail tests:
> http://cfbot.cputube.org/paul-ramsey.html
> 

Yep, dumb typo, thanks! This bot is amazeballs, thank you!

P.

  6. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-08T23:25:24Z

    > 
> Revised patch attached.

And again, because I think I missed one change in the last one.

  7. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T00:44:56Z

    
> On Dec 8, 2022, at 3:25 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
>> 
>> Revised patch attached.
> 
> And again, because I think I missed one change in the last one.
> 
> <random_normal_03.patch>

Final tme, with fixes from cirrusci.

  8. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2022-12-09T04:29:27Z

    On Thu, Dec 08, 2022 at 04:44:56PM -0800, Paul Ramsey wrote:
> Final tme, with fixes from cirrusci.

Well, why not.  Seems like you would use that a lot with PostGIS.

 #include <math.h>              /* for ldexp() */
+#include <float.h>             /* for DBL_EPSILON */
And be careful with the order here.

+static void
+drandom_check_default_seed()
We always use (void) rather than empty parenthesis sets.

I would not leave that unchecked, so I think that you should add
something in ramdom.sql.  Or would you prefer switching some of
the regression tests be switched so as they use the new normal
function?

(Ahem.  Bonus points for a random_string() returning a bytea, based on
pg_strong_random().)
--
Michael

  9. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T17:17:03Z

    > On Dec 8, 2022, at 8:29 PM, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Thu, Dec 08, 2022 at 04:44:56PM -0800, Paul Ramsey wrote:
>> Final tme, with fixes from cirrusci.
> 
> Well, why not.  Seems like you would use that a lot with PostGIS.
> 
> #include <math.h>              /* for ldexp() */
> +#include <float.h>             /* for DBL_EPSILON */
> And be careful with the order here.

Should be ... alphabetical?

> +static void
> +drandom_check_default_seed()
> We always use (void) rather than empty parenthesis sets.

OK

> I would not leave that unchecked, so I think that you should add
> something in ramdom.sql.  Or would you prefer switching some of
> the regression tests be switched so as they use the new normal
> function?

Reading through those tests... seems like they will (rarely) fail. Is that... OK? 
The tests seem to be mostly worried that random() starts returning constants, which seems like a good thing to test for (is the random number generating returning randomness).
An obvious test for this function is that the mean and stddev converge on the supplied parameters, given enough inputs, which is actually kind of the opposite test. I use the same random number generator as the uniform distribution, so that aspect is already covered by the existing tests.

> (Ahem.  Bonus points for a random_string() returning a bytea, based on
> pg_strong_random().)

Would love to. Separate patch of bundled into this one?

P


> --
> Michael

  10. Re: [PATCH] random_normal function

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-12-09T18:39:22Z

    
> On Dec 8, 2022, at 1:53 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
> Just a utility function to generate random numbers from a normal
> distribution. I find myself doing this several times a year, and I am
> sure I must not be the only one.

Thanks for the patch.  What do you think about these results?

+-- The semantics of a negative stddev are not well defined
+SELECT random_normal(mean := 0, stddev := -1);
+    random_normal    
+---------------------
+ -1.0285744583010896
+(1 row)
+
+SELECT random_normal(mean := 0, stddev := '-Inf');
+ random_normal 
+---------------
+      Infinity
+(1 row)
+
+-- This result may be defensible...
+SELECT random_normal(mean := '-Inf', stddev := 'Inf');
+ random_normal 
+---------------
+     -Infinity
+(1 row)
+
+-- but if so, why is this NaN?
+SELECT random_normal(mean := 'Inf', stddev := 'Inf');
+ random_normal 
+---------------
+           NaN
+(1 row)
+

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T18:51:53Z

    
> On Dec 9, 2022, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
>> On Dec 8, 2022, at 1:53 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>> 
>> Just a utility function to generate random numbers from a normal
>> distribution. I find myself doing this several times a year, and I am
>> sure I must not be the only one.
> 
> Thanks for the patch.  What do you think about these results?

Angels on pins time! :)

> +-- The semantics of a negative stddev are not well defined
> +SELECT random_normal(mean := 0, stddev := -1);
> +    random_normal    
> +---------------------
> + -1.0285744583010896
> +(1 row)

Question is does a negative stddev make enough sense? It is functionally using fabs(stddev), 

SELECT avg(random_normal(mean := 0, stddev := -1)) from generate_series(1,1000);
         avg         
---------------------
 0.03156106778729526

So could toss an invalid parameter on negative? Not sure if that's more helpful than just being mellow about this input.


> +
> +SELECT random_normal(mean := 0, stddev := '-Inf');
> + random_normal 
> +---------------
> +      Infinity
> +(1 row)

The existing logic around means and stddevs and Inf is hard to tease out:

SELECT avg(v),stddev(v) from (VALUES ('Inf'::float8, '-Inf'::float8)) a(v);
   avg    | stddev 
----------+--------
 Infinity |       

The return of NULL of stddev would seem to argue that null in this case means "does not compute" at some level. So return NULL on Inf stddev?

> +
> +-- This result may be defensible...
> +SELECT random_normal(mean := '-Inf', stddev := 'Inf');
> + random_normal 
> +---------------
> +     -Infinity
> +(1 row)
> +
> +-- but if so, why is this NaN?
> +SELECT random_normal(mean := 'Inf', stddev := 'Inf');
> + random_normal 
> +---------------
> +           NaN
> +(1 row)

An Inf mean only implies that one value in the distribution is Inf, but running the function in reverse (generating values) and only generating one value from the distribution implies we have to always return Inf (except in this case stddev is also Inf, so I'd go with NULL, assuming we accept the NULL premise above.

How do you read the tea leaves?

P.

  12. Re: [PATCH] random_normal function

    Joe Conway <mail@joeconway.com> — 2022-12-09T19:01:20Z

    On 12/9/22 13:51, Paul Ramsey wrote:
>> On Dec 9, 2022, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>> On Dec 8, 2022, at 1:53 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>>> 
>>> Just a utility function to generate random numbers from a normal
>>> distribution. I find myself doing this several times a year, and I am
>>> sure I must not be the only one.
>> 
>> Thanks for the patch.  What do you think about these results?
> 
> Angels on pins time! :)

I just noticed this thread -- what is lacking in the normal_rand() 
function in the tablefunc contrib?

https://www.postgresql.org/docs/current/tablefunc.html#id-1.11.7.52.5

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  13. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T19:10:25Z

    
> On Dec 9, 2022, at 11:01 AM, Joe Conway <mail@joeconway.com> wrote:
> 
> On 12/9/22 13:51, Paul Ramsey wrote:
>>> On Dec 9, 2022, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>>> On Dec 8, 2022, at 1:53 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>>>> Just a utility function to generate random numbers from a normal
>>>> distribution. I find myself doing this several times a year, and I am
>>>> sure I must not be the only one.
>>> Thanks for the patch.  What do you think about these results?
>> Angels on pins time! :)
> 
> I just noticed this thread -- what is lacking in the normal_rand() function in the tablefunc contrib?
> 
> https://www.postgresql.org/docs/current/tablefunc.html#id-1.11.7.52.5

Simplicity I guess mostly. random_normal() has a direct analogue in random() which is also a core function. I mean it could equally be pointed out that a user can implement their own Box-Muller calculation pretty trivially. Part of this submission is a personal wondering to what extent the community values convenience vs composibility. The set-returning nature of normal_rand() may be a bit of a red herring to people who just want one value (even though normal_rand (1, 0.0, 1.0) does exactly what they want).

P.

  14. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T19:11:34Z

    
> On Dec 9, 2022, at 11:10 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
> 
> 
>> On Dec 9, 2022, at 11:01 AM, Joe Conway <mail@joeconway.com> wrote:
>> 
>> On 12/9/22 13:51, Paul Ramsey wrote:
>>>> On Dec 9, 2022, at 10:39 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>>>>> On Dec 8, 2022, at 1:53 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>>>>> Just a utility function to generate random numbers from a normal
>>>>> distribution. I find myself doing this several times a year, and I am
>>>>> sure I must not be the only one.
>>>> Thanks for the patch.  What do you think about these results?
>>> Angels on pins time! :)
>> 
>> I just noticed this thread -- what is lacking in the normal_rand() function in the tablefunc contrib?
>> 
>> https://www.postgresql.org/docs/current/tablefunc.html#id-1.11.7.52.5
> 
> Simplicity I guess mostly. random_normal() has a direct analogue in random() which is also a core function. I mean it could equally be pointed out that a user can implement their own Box-Muller calculation pretty trivially. Part of this submission is a personal wondering to what extent the community values convenience vs composibility. The set-returning nature of normal_rand() may be a bit of a red herring to people who just want one value (even though normal_rand (1, 0.0, 1.0) does exactly what they want).

No related to the "reason to exist", but normal_rand() has some interesting behaviour under Mark's test cases!

select normal_rand (1, 'Inf', 'Inf'), a from generate_series(1,2) a;
 normal_rand | a 
-------------+---
         NaN | 1
    Infinity | 2
(2 rows)

  15. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-09T23:20:35Z

    
> On Dec 9, 2022, at 9:17 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
> 
>> On Dec 8, 2022, at 8:29 PM, Michael Paquier <michael@paquier.xyz> wrote:
>> 
>> On Thu, Dec 08, 2022 at 04:44:56PM -0800, Paul Ramsey wrote:
>>> Final tme, with fixes from cirrusci.
>> 
>> Well, why not.  Seems like you would use that a lot with PostGIS.
>> 
>> #include <math.h>              /* for ldexp() */
>> +#include <float.h>             /* for DBL_EPSILON */
>> And be careful with the order here.
> 
> Should be ... alphabetical?
> 
>> +static void
>> +drandom_check_default_seed()
>> We always use (void) rather than empty parenthesis sets.
> 
> OK
> 
>> I would not leave that unchecked, so I think that you should add
>> something in ramdom.sql.  Or would you prefer switching some of
>> the regression tests be switched so as they use the new normal
>> function?
> 
> Reading through those tests... seems like they will (rarely) fail. Is that... OK? 
> The tests seem to be mostly worried that random() starts returning constants, which seems like a good thing to test for (is the random number generating returning randomness).
> An obvious test for this function is that the mean and stddev converge on the supplied parameters, given enough inputs, which is actually kind of the opposite test. I use the same random number generator as the uniform distribution, so that aspect is already covered by the existing tests.
> 
>> (Ahem.  Bonus points for a random_string() returning a bytea, based on
>> pg_strong_random().)
> 
> Would love to. Separate patch of bundled into this one?

Here's the original with suggestions applied and a random_string that applies on top of it.

Thanks!

P

  16. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-13T23:51:11Z

    
> On Dec 9, 2022, at 3:20 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> 
> 
> 
>> On Dec 9, 2022, at 9:17 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>> 
>> 
>>> On Dec 8, 2022, at 8:29 PM, Michael Paquier <michael@paquier.xyz> wrote:
>>> 
>>> On Thu, Dec 08, 2022 at 04:44:56PM -0800, Paul Ramsey wrote:
>>>> Final tme, with fixes from cirrusci.
>>> 
>>> Well, why not.  Seems like you would use that a lot with PostGIS.
>>> 
>>> #include <math.h>              /* for ldexp() */
>>> +#include <float.h>             /* for DBL_EPSILON */
>>> And be careful with the order here.
>> 
>> Should be ... alphabetical?
>> 
>>> +static void
>>> +drandom_check_default_seed()
>>> We always use (void) rather than empty parenthesis sets.
>> 
>> OK
>> 
>>> I would not leave that unchecked, so I think that you should add
>>> something in ramdom.sql.  Or would you prefer switching some of
>>> the regression tests be switched so as they use the new normal
>>> function?
>> 
>> Reading through those tests... seems like they will (rarely) fail. Is that... OK? 
>> The tests seem to be mostly worried that random() starts returning constants, which seems like a good thing to test for (is the random number generating returning randomness).
>> An obvious test for this function is that the mean and stddev converge on the supplied parameters, given enough inputs, which is actually kind of the opposite test. I use the same random number generator as the uniform distribution, so that aspect is already covered by the existing tests.
>> 
>>> (Ahem.  Bonus points for a random_string() returning a bytea, based on
>>> pg_strong_random().)
>> 
>> Would love to. Separate patch of bundled into this one?
> 
> Here's the original with suggestions applied and a random_string that applies on top of it.
> 
> Thanks!
> 
> P

Clearing up one CI failure.

  17. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2022-12-15T05:17:01Z

    On Tue, Dec 13, 2022 at 03:51:11PM -0800, Paul Ramsey wrote:
> Clearing up one CI failure.

+-- normal values converge on stddev == 2.0
+SELECT round(stddev(random_normal(2, 2)))
+  FROM generate_series(1, 10000);

I am not sure that it is a good idea to make a test based on a random
behavior that should tend to a normalized value.  This is costly in
cycles, requiring a lot of work just for generate_series().  You could
do the same kind of thing as random() a few lines above?

+SELECT bool_and(random_string(16) != random_string(16)) AS same
+  FROM generate_series(1,8);
That should be fine in terms of impossible chances :)

+       ereport(ERROR,
+           (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+            errmsg("return size must be non-negative")))
This could have a test, same for 0.

+#ifndef M_PI
+#define M_PI 3.14159265358979323846
+#endif
Postgres' float.h includes one version of that.
--
Michael

  18. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2022-12-15T20:53:40Z

    
> On Dec 14, 2022, at 9:17 PM, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Tue, Dec 13, 2022 at 03:51:11PM -0800, Paul Ramsey wrote:
>> Clearing up one CI failure.
> 
> +-- normal values converge on stddev == 2.0
> +SELECT round(stddev(random_normal(2, 2)))
> +  FROM generate_series(1, 10000);
> 
> I am not sure that it is a good idea to make a test based on a random
> behavior that should tend to a normalized value.  This is costly in
> cycles, requiring a lot of work just for generate_series().  You could
> do the same kind of thing as random() a few lines above?
> 
> +SELECT bool_and(random_string(16) != random_string(16)) AS same
> +  FROM generate_series(1,8);
> That should be fine in terms of impossible chances :)
> 
> +       ereport(ERROR,
> +           (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +            errmsg("return size must be non-negative")))
> This could have a test, same for 0.
> 
> +#ifndef M_PI
> +#define M_PI 3.14159265358979323846
> +#endif
> Postgres' float.h includes one version of that.

Thanks again!

P

  19. Re: [PATCH] random_normal function

    Fabien COELHO <coelho@cri.ensmp.fr> — 2022-12-17T16:49:15Z

    Hello Paul,

My 0.02€ about the patch:

Patch did not apply with "git apply", I had to "patch -p1 <" and there was 
a bunch of warnings.

Patches compile and make check is okay.

The first patch adds empty lines at the end of "sql/random.sql", I think 
that it should not.

# About random_normal:

I'm fine with providing a random_normal implementation at prng and SQL 
levels.

There is already such an implementation in "pgbench.c", which outputs 
integers, I'd suggest that it should also use the new prng function, there 
should not be two box-muller transformations in pg.

# About pg_prng_double_normal:

On the comment, I'd write "mean + stddev * val" instead of starting with 
the stddev part.

Usually there is an empty line between the variable declarations and the
first statement.

There should be a comment about why it needs u1 
larger than some epsilon. This constraint seems to generate a small bias?

I'd suggest to add the UNLIKELY() compiler hint on the loop.

# About random_string:

Should the doc about random_string tell that the output bytes are expected 
to be uniformly distributed? Does it return "random values" or "pseudo 
random values"?

I do not understand why the "drandom_string" function is in "float.c", as 
it is not really related to floats. Also it does not return a string but a 
bytea, so why call it "_string" in the first place? I'm do not think that 
it should use pg_strong_random which may be very costly on some platform. 
Also, pg_strong_random does not use the seed, so I do not understand why 
it needs to be checked. I'd suggest that the output should really be 
uniform pseudo-random, possibly based on the drandom() state, or maybe 
not.

Overall, I think that there should be a clearer discussion and plan about 
which random functionS postgres should provide to complement the standard 
instead of going there… randomly:-)

-- 
Fabien.

  20. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2022-12-19T03:36:25Z

    On Sat, Dec 17, 2022 at 05:49:15PM +0100, Fabien COELHO wrote:
> Overall, I think that there should be a clearer discussion and plan about
> which random functionS postgres should provide to complement the standard
> instead of going there… randomly:-)

So, what does the specification tells about seeds, normal and random
functions?  A bunch of DBMSs implement RAND, sometimes RANDOM, SEED or
even NORMAL using from time to time specific SQL keywords to do the
work.

Note that SQLValueFunction made the addition of more returning data
types a bit more complicated (not much, still) than the new
COERCE_SQL_SYNTAX by going through a mapping function, so the
keyword/function mapping is straight-forward.
--
Michael

  21. Re: [PATCH] random_normal function

    Fabien COELHO <coelho@cri.ensmp.fr> — 2022-12-21T07:47:32Z

    Bonjour Michaël,

>> Overall, I think that there should be a clearer discussion and plan about
>> which random functionS postgres should provide to complement the standard
>> instead of going there… randomly:-)
>
> So, what does the specification tells about seeds, normal and random
> functions?  A bunch of DBMSs implement RAND, sometimes RANDOM, SEED or
> even NORMAL using from time to time specific SQL keywords to do the
> work.

I do not have the SQL standard, so I have no idea about what is in there.

From a typical use case point of view, I'd say uniform, normal and 
exponential would make sense for floats. I'm also okay with generating a 
uniform bytes pseudo-randomly.

I'd be more at ease to add simple functions rather than a special 
heavy-on-keywords syntax, even if standard.

> Note that SQLValueFunction made the addition of more returning data
> types a bit more complicated (not much, still) than the new
> COERCE_SQL_SYNTAX by going through a mapping function, so the
> keyword/function mapping is straight-forward.

I'm unclear about why this paragraph is here.

-- 
Fabien.

  22. Re: [PATCH] random_normal function

    Vik Fearing <vik@postgresfriends.org> — 2022-12-21T08:18:54Z

    On 12/19/22 04:36, Michael Paquier wrote:
> On Sat, Dec 17, 2022 at 05:49:15PM +0100, Fabien COELHO wrote:
>> Overall, I think that there should be a clearer discussion and plan about
>> which random functionS postgres should provide to complement the standard
>> instead of going there… randomly:-)
> 
> So, what does the specification tells about seeds, normal and random
> functions?


Nothing at all.
-- 
Vik Fearing

  23. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2022-12-22T02:38:50Z

    On Wed, Dec 21, 2022 at 08:47:32AM +0100, Fabien COELHO wrote:
> From a typical use case point of view, I'd say uniform, normal and
> exponential would make sense for floats. I'm also okay with generating a
> uniform bytes pseudo-randomly.

I'd agree with this set.

> I'd be more at ease to add simple functions rather than a special
> heavy-on-keywords syntax, even if standard.

Okay.

>> Note that SQLValueFunction made the addition of more returning data
>> types a bit more complicated (not much, still) than the new
>> COERCE_SQL_SYNTAX by going through a mapping function, so the
>> keyword/function mapping is straight-forward.
> 
> I'm unclear about why this paragraph is here.

Just saying that using COERCE_SQL_SYNTAX for SQL keywords is easier
than the older style.  If the SQL specification mentions no SQL
keywords for such things, this is irrelevant, of course :)
--
Michael

  24. Re: [PATCH] random_normal function

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-31T03:58:04Z

    On Thu, Dec 08, 2022 at 02:58:02PM -0800, Paul Ramsey wrote:
> > On Dec 8, 2022, at 2:46 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > 
> > I guess make_interval is a typo ?
> > 
> > This is causing it to fail tests:
> > http://cfbot.cputube.org/paul-ramsey.html
> > 
> 
> Yep, dumb typo, thanks! This bot is amazeballs, thank you!

This is still failing tests - did you enable cirrusci on your own github
account to run available checks on the patch ?

-- 
Justin

  25. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2023-01-03T02:24:38Z

    On Fri, Dec 30, 2022 at 09:58:04PM -0600, Justin Pryzby wrote:
> This is still failing tests - did you enable cirrusci on your own github
> account to run available checks on the patch ?

FYI, here is the failure:
[21:23:10.814] In file included from pg_prng.c:27:
[21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct
Node’ declared inside parameter list will not be visible outside of
this definition or declaration [-Werror] 
[21:23:10.814]    46 |         struct Node *escontext);

And a link to it, from the CF bot:
https://cirrus-ci.com/task/5969961391226880?logs=gcc_warning#L452
--
Michael

  26. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-03T02:38:28Z

    Michael Paquier <michael@paquier.xyz> writes:
> FYI, here is the failure:
> [21:23:10.814] In file included from pg_prng.c:27:
> [21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct
> Node’ declared inside parameter list will not be visible outside of
> this definition or declaration [-Werror] 
> [21:23:10.814]    46 |         struct Node *escontext);

Hmm ... this looks an awful lot like it is the fault of ccff2d20e
not of the random_normal patch; that is, we probably need a
"struct Node" stub declaration in float.h.  However, why are we
not seeing any reports of this from elsewhere?  I'm concerned now
that there are more places also needing stub declarations, but
my test process didn't expose it.

			regards, tom lane

  27. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-03T16:41:37Z

    I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> FYI, here is the failure:
>> [21:23:10.814] In file included from pg_prng.c:27:
>> [21:23:10.814] ../../src/include/utils/float.h:46:16: error: ‘struct
>> Node’ declared inside parameter list will not be visible outside of
>> this definition or declaration [-Werror] 
>> [21:23:10.814]    46 |         struct Node *escontext);

> Hmm ... this looks an awful lot like it is the fault of ccff2d20e
> not of the random_normal patch; that is, we probably need a
> "struct Node" stub declaration in float.h.

[ ... some head-scratching later ... ]

No, we don't per our normal headerscheck rules, which are that
headers such as utils/float.h need to be compilable after including
just postgres.h.  The "struct Node" stub declaration in elog.h will
be visible, making the declaration of float8in_internal kosher.

So the problem in this patch is that it's trying to include
utils/float.h in a src/common file, where we have not included
postgres.h.  Question is, why did you do that?  I see nothing in
pg_prng_double_normal() that looks like it should require that header.
If it did, it'd be questionable whether it's okay to be in src/common.

			regards, tom lane

  28. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-09T00:20:40Z

    I wrote:
> So the problem in this patch is that it's trying to include
> utils/float.h in a src/common file, where we have not included
> postgres.h.  Question is, why did you do that?

(Ah, for M_PI ... but our practice is just to duplicate that #define
where needed outside the backend.)

I spent some time reviewing this patch.  I'm on board with
inventing random_normal(): the definition seems solid and
the use-case for it seems reasonably well established.
I'm not necessarily against inventing similar functions for
other distributions, but this patch is not required to do so.
We can leave that discussion until somebody is motivated to
submit a patch for one.

On the other hand, I'm much less on board with inventing
random_string(): we don't have any clear field demand for it
and the appropriate definitional details are a lot less obvious
(for example, whether it needs to be based on pg_strong_random()
rather than the random() sequence).  I think we should leave that
out, and I have done so in the attached updated patch.

I noted several errors in the submitted patch.  It was creating
the function as PARALLEL SAFE which is just wrong, and the whole
business with checking PG_NARGS is useless because it will always
be 2.  (That's not how default arguments work.)

The business with checking against DBL_EPSILON seems wrong too.
All we need is to ensure that u1 > 0 so that log(u1) will not
choke; per spec, log() is defined for any positive input.  I see that
that seems to have been modeled on the C++ code in the Wikipedia
page, but I'm not sure that C++'s epsilon means the same thing, and
if it does then their example code is just wrong.  See the discussion
about "tails truncation" immediately above it: artificially
constraining the range of u1 just limits how much of the tail
of the distribution we can reproduce.  So that led me to doing
it the same way as in the existing Box-Muller code in pgbench,
which I then deleted per Fabien's advice.

BTW, the pgbench code was using sin() not cos(), which I duplicated
because using cos() causes the expected output of the pgbench tests
to change.  I'm not sure whether there was any hard reason to prefer
one or the other, and we can certainly change the expected output
if there's some reason to prefer cos().

I concur with not worrying about the Inf/NaN cases that Mark
pointed out.  It's not obvious that the results the proposed code
produces are wrong, and it's even less obvious that anyone will
ever care.

Also, I tried running the new random.sql regression cases over
and over, and found that the "not all duplicates" test fails about
one time in 100000 or so.  We could probably tolerate that given
that the random test is marked "ignore" in parallel_schedule, but
I thought it best to add one more iteration so we could knock the
odds down.  Also I changed the test iterations so they weren't
all invoking random_normal() in exactly the same way.

This version seems committable to me, barring objections.

			regards, tom lane

  29. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-09T02:17:21Z

    I wrote:
> Also, I tried running the new random.sql regression cases over
> and over, and found that the "not all duplicates" test fails about
> one time in 100000 or so.  We could probably tolerate that given
> that the random test is marked "ignore" in parallel_schedule, but
> I thought it best to add one more iteration so we could knock the
> odds down.

Hmm ... it occurred to me to try the same check on the existing
random() tests (attached), and darn if they don't fail even more
often, usually within 50K iterations.  So maybe we should rethink
that whole thing.

			regards, tom lane

  30. Re: [PATCH] random_normal function

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-09T12:03:34Z

    On Mon, 9 Jan 2023 at 00:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> This version seems committable to me, barring objections.
>

Whilst I have no objection to adding random_normal(), ISTM that we're
at risk of adding an arbitrary set of random functions without a clear
idea of where we'll end up, and how they'll affect one another (shared
state or not).

For example, random_normal() uses a PRNG and it shares the same state
as random()/setseed(). That's fine, and presumably if we add other
continuous distributions like exponential, they'll follow suit.

But what if we add something like random_int() to return a random
integer in a range (something that has been suggested before, and I
think would be very useful), or other discrete random functions? Would
they use a PRNG and share the same state? If so, having that state in
float.c wouldn't make sense anymore. If not, will they have their own
seed-setting functions, and how many seed-setting functions will we
end up with?

Over on [1] we're currently heading towards adding array_shuffle() and
array_sample() using a PRNG with a separate state shared just by those
2 functions, with no way to seed it, and not marking them as PARALLEL
RESTRICTED, so they can't be made deterministic.

ISTM that random functions should fall into 1 of 2 clearly defined
categories -- strong random functions and pseudorandom functions. IMO
all pseudorandom functions should be PARALLEL RESTRICTED and have a
way to set their seed, so that they can be made deterministic --
something that's very useful for users writing tests.

Now maybe having multiple seed-setting functions (one per datatype?)
is OK, but it seems unnecessary and cumbersome to me. Why would you
ever want to seed the random_int() sequence and the random() sequence
differently? No other library of random functions I know of behaves
that way.

So IMO all pseudorandom functions should share the same PRNG state and
seed-setting functions. That would mean they should all be in the same
(new) C file, so that the PRNG state can be kept private to that file.

I think it would also make sense to add a new "Random Functions"
section to the docs, and move the descriptions of random(),
random_normal() and setseed() there. That way, all the functions
affected by setseed() can be kept together on one page (future random
functions may not all be sensibly classified as "mathematical").

Regards,
Dean

[1] https://www.postgresql.org/message-id/flat/9d160a44-7675-51e8-60cf-6d64b76db831@aboutsource.net

  31. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-09T15:26:23Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> So IMO all pseudorandom functions should share the same PRNG state and
> seed-setting functions. That would mean they should all be in the same
> (new) C file, so that the PRNG state can be kept private to that file.

I agree with this in principle, but I feel no need to actually reshuffle
the code before we accept a proposal for such a function that wouldn't
logically belong in float.c.

> I think it would also make sense to add a new "Random Functions"
> section to the docs, and move the descriptions of random(),
> random_normal() and setseed() there.

Likewise, this'll just add confusion in the short run.  A <sect1>
with only three functions in it is going to look mighty odd.

			regards, tom lane

  32. Re: [PATCH] random_normal function

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-09T16:12:37Z

    On Mon, 9 Jan 2023 at 15:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > So IMO all pseudorandom functions should share the same PRNG state and
> > seed-setting functions. That would mean they should all be in the same
> > (new) C file, so that the PRNG state can be kept private to that file.
>
> I agree with this in principle, but I feel no need to actually reshuffle
> the code before we accept a proposal for such a function that wouldn't
> logically belong in float.c.
>
> > I think it would also make sense to add a new "Random Functions"
> > section to the docs, and move the descriptions of random(),
> > random_normal() and setseed() there.
>
> Likewise, this'll just add confusion in the short run.  A <sect1>
> with only three functions in it is going to look mighty odd.
>

OK, that's fair enough, while we're just adding random_normal().

BTW, "UUID Functions" only has 1 function in it :-)

Regards,
Dean

  33. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-09T18:52:16Z

    I wrote:
> Hmm ... it occurred to me to try the same check on the existing
> random() tests (attached), and darn if they don't fail even more
> often, usually within 50K iterations.  So maybe we should rethink
> that whole thing.

I pushed Paul's patch with the previously-discussed tests, but
the more I look at random.sql the less I like it.  I propose
that we nuke the existing tests from orbit and replace with
something more or less as attached.  This is faster than what
we have, removes the unnecessary dependency on the "onek" table,
and I believe it to be a substantially more thorough test of the
random functions' properties.  (We could probably go further
than this, like trying to verify distribution properties.  But
it's been too long since college statistics for me to want to
write such tests myself, and I'm not real sure we need them.)

BTW, if this does bring the probability of failure down to the
one-in-a-billion range, I think we could also nuke the whole
"ignore:" business, simplifying pg_regress and allowing the
random test to be run in parallel with others.

			regards, tom lane

  34. Re: [PATCH] random_normal function

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-09T22:38:36Z

    On Mon, 9 Jan 2023 at 18:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I pushed Paul's patch with the previously-discussed tests, but
> the more I look at random.sql the less I like it.  I propose
> that we nuke the existing tests from orbit and replace with
> something more or less as attached.

Looks like a definite improvement.

>  (We could probably go further
> than this, like trying to verify distribution properties.  But
> it's been too long since college statistics for me to want to
> write such tests myself, and I'm not real sure we need them.)

I played around with the Kolmogorov–Smirnov test, to test random() for
uniformity. The following passes roughly 99.9% of the time:

CREATE OR REPLACE FUNCTION ks_test_uniform_random()
RETURNS boolean AS
$$
DECLARE
  n int := 1000;        -- Number of samples
  c float8 := 1.94947;  -- Critical value for 99.9% confidence
/*  c float8 := 1.62762;  -- Critical value for 99% confidence */
/*  c float8 := 1.22385;  -- Critical value for 90% confidence */
  ok boolean;
BEGIN
  ok := (
    WITH samples AS (
      SELECT random() r FROM generate_series(1, n) ORDER BY 1
    ), indexed_samples AS (
      SELECT (row_number() OVER())-1.0 i, r FROM samples
    )
    SELECT max(abs(i/n-r)) < c / sqrt(n) FROM indexed_samples
  );
  RETURN ok;
END
$$
LANGUAGE plpgsql;

and is very fast. So that gives decent confidence that random() is
indeed uniform.

With a one-in-a-thousand chance of failing, if you wanted something
with around a one-in-a-billion chance of failing, you could just try
it 3 times:

SELECT ks_test_uniform_random() OR
       ks_test_uniform_random() OR
       ks_test_uniform_random();

but it feels pretty hacky, and probably not really necessary.

Rigorous tests for other distributions are harder, but also probably
not necessary if we have confidence that the underlying PRNG is
uniform.

> BTW, if this does bring the probability of failure down to the
> one-in-a-billion range, I think we could also nuke the whole
> "ignore:" business, simplifying pg_regress and allowing the
> random test to be run in parallel with others.

I didn't check the one-in-a-billion claim, but +1 for that.

Regards,
Dean

  35. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-09T23:38:39Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Mon, 9 Jan 2023 at 18:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (We could probably go further
>> than this, like trying to verify distribution properties.  But
>> it's been too long since college statistics for me to want to
>> write such tests myself, and I'm not real sure we need them.)

> I played around with the Kolmogorov–Smirnov test, to test random() for
> uniformity. The following passes roughly 99.9% of the time:

Ah, cool, thanks for this code.

> With a one-in-a-thousand chance of failing, if you wanted something
> with around a one-in-a-billion chance of failing, you could just try
> it 3 times:
> SELECT ks_test_uniform_random() OR
>        ks_test_uniform_random() OR
>        ks_test_uniform_random();
> but it feels pretty hacky, and probably not really necessary.

That seems like a good way, because I'm not satisfied with
one-in-a-thousand odds if we want to remove the "ignore" marker.
It's still plenty fast enough: on my machine, the v2 patch below
takes about 19ms, versus 22ms for the script as it stands in HEAD.

> Rigorous tests for other distributions are harder, but also probably
> not necessary if we have confidence that the underlying PRNG is
> uniform.

Agreed.

>> BTW, if this does bring the probability of failure down to the
>> one-in-a-billion range, I think we could also nuke the whole
>> "ignore:" business, simplifying pg_regress and allowing the
>> random test to be run in parallel with others.

> I didn't check the one-in-a-billion claim, but +1 for that.

I realized that we do already run random in a parallel group;
the "ignore: random" line in parallel_schedule just marks it
as failure-ignorable, it doesn't schedule it.  (The comment is a
bit misleading about this, but I want to remove that not rewrite it.)
Nonetheless, nuking the whole ignore-failures mechanism seems like
good cleanup to me.

Also, I tried this on some 32-bit big-endian hardware (NetBSD on macppc)
to verify my thesis that the results of random() are now machine
independent.  That part works, but the random_normal() tests didn't;
I saw low-order-bit differences from the results on x86_64 Linux.
Presumably, that's because one or more of sqrt()/log()/sin() are
rounding off a bit differently there.  v2 attached deals with this by
backing off to "extra_float_digits = 0" for that test.  Once it hits the
buildfarm we might find we have to reduce extra_float_digits some more,
but that was enough to make NetBSD/macppc happy.

			regards, tom lane

  36. Re: [PATCH] random_normal function

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-10T08:33:53Z

    On Mon, 9 Jan 2023 at 23:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I tried this on some 32-bit big-endian hardware (NetBSD on macppc)
> to verify my thesis that the results of random() are now machine
> independent.  That part works, but the random_normal() tests didn't;

Ah yes, I wondered about that.

> I saw low-order-bit differences from the results on x86_64 Linux.
> Presumably, that's because one or more of sqrt()/log()/sin() are
> rounding off a bit differently there.  v2 attached deals with this by
> backing off to "extra_float_digits = 0" for that test.

Makes sense.

I double-checked the one-in-a-billion claim, and it looks about right
for each test.

The one I wasn't sure about was the chance of duplicates for
random_normal(). Analysing it more closely, it actually has a smaller
chance of duplicates, since the difference between 2 standard normal
distributions is another normal distribution with a standard deviation
of sqrt(2), and so the probability of a pair of random_normal()'s
being the same is about 2*sqrt(pi) ~ 3.5449 times lower than for
random(). So you can call random_normal() around 5600 times (rather
than 3000 times) before having a 1e-9 chance of duplicates. So, as
with the random() duplicates test, the probability of failure with
just 1000 values should be well below 1e-9. Intuitively, that was
always going to be true, but I wanted to know the details.

The rest looks good to me, except there's a random non-ASCII character
instead of a hyphen in "Kolmogorov-Smirnov" (because I copy-pasted the
name from some random website).

Regards,
Dean

  37. Re: [PATCH] random_normal function

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2023-01-10T10:20:00Z

    On Tue, 10 Jan 2023 at 08:33, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> The rest looks good to me, except there's a random non-ASCII character
> instead of a hyphen in "Kolmogorov-Smirnov" (because I copy-pasted the
> name from some random website).
>

Oh, never mind. I see you already fixed that.
I should finish reading all my mail before hitting reply.

Regards,
Dean

  38. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-10T14:34:39Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I double-checked the one-in-a-billion claim, and it looks about right
> for each test.

Thanks for double-checking my arithmetic.

> The rest looks good to me, except there's a random non-ASCII character
> instead of a hyphen in "Kolmogorov-Smirnov" (because I copy-pasted the
> name from some random website).

Yeah, I caught that before committing.

The AIX buildfarm members were still showing low-order diffs in the
random_normal results at extra_float_digits = 0, but they seem OK
after reducing it to -1.

			regards, tom lane

  39. Re: [PATCH] random_normal function

    Paul Ramsey <pramsey@cleverelephant.ca> — 2023-01-10T16:11:33Z

    On Tue, Jan 10, 2023 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > I double-checked the one-in-a-billion claim, and it looks about right
> > for each test.
>
> Thanks for double-checking my arithmetic.
>
> > The rest looks good to me, except there's a random non-ASCII character
> > instead of a hyphen in "Kolmogorov-Smirnov" (because I copy-pasted the
> > name from some random website).
>
> Yeah, I caught that before committing.
>
> The AIX buildfarm members were still showing low-order diffs in the
> random_normal results at extra_float_digits = 0, but they seem OK
> after reducing it to -1.

I should leave the country more often... thanks for cleaning up my
patch and committing it Tom! It's a Christmas miracle (at least, for
me :)
P.

  40. Re: [PATCH] random_normal function

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-01-19T05:39:27Z

    On 1/9/23 23:52, Tom Lane wrote:
> BTW, if this does bring the probability of failure down to the
> one-in-a-billion range, I think we could also nuke the whole
> "ignore:" business, simplifying pg_regress and allowing the
> random test to be run in parallel with others.
With 'ignore' option we get used to cover by tests some of the time 
dependent features, such as "statement_timeout", 
"idle_in_transaction_session_timeout", usage of user timeouts in 
extensions and so on.

We have used the pg_sleep() function to interrupt a query at certain 
execution phase. But on some platforms, especially in containers, the 
query can vary execution time in so widely that the pg_sleep() timeout, 
required to get rid of dependency on a query execution time, has become 
unacceptable. So, the "ignore" option was the best choice.

For Now, Do we only have the "isolation tests" option to create stable 
execution time-dependent tests now? Or I'm not aware about some test 
machinery?

-- 
Regards
Andrey Lepikhov
Postgres Professional

  41. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-19T06:01:35Z

    Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
> On 1/9/23 23:52, Tom Lane wrote:
>> BTW, if this does bring the probability of failure down to the
>> one-in-a-billion range, I think we could also nuke the whole
>> "ignore:" business, simplifying pg_regress and allowing the
>> random test to be run in parallel with others.

> We have used the pg_sleep() function to interrupt a query at certain 
> execution phase. But on some platforms, especially in containers, the 
> query can vary execution time in so widely that the pg_sleep() timeout, 
> required to get rid of dependency on a query execution time, has become 
> unacceptable. So, the "ignore" option was the best choice.

But does such a test have any actual value?  If your test infrastructure
ignores the result, what makes you think you'd notice if the test did
indeed detect a problem?

I think "ignore:" was a kluge we put in twenty-plus years ago when our
testing standards were a lot lower, and it's way past time we got
rid of it.

			regards, tom lane

  42. Re: [PATCH] random_normal function

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-01-19T06:48:20Z

    On 1/19/23 11:01, Tom Lane wrote:
> Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
>> On 1/9/23 23:52, Tom Lane wrote:
>>> BTW, if this does bring the probability of failure down to the
>>> one-in-a-billion range, I think we could also nuke the whole
>>> "ignore:" business, simplifying pg_regress and allowing the
>>> random test to be run in parallel with others.
> 
>> We have used the pg_sleep() function to interrupt a query at certain
>> execution phase. But on some platforms, especially in containers, the
>> query can vary execution time in so widely that the pg_sleep() timeout,
>> required to get rid of dependency on a query execution time, has become
>> unacceptable. So, the "ignore" option was the best choice.
> 
> But does such a test have any actual value?  If your test infrastructure
> ignores the result, what makes you think you'd notice if the test did
> indeed detect a problem?
Yes, it is good to catch SEGFAULTs and assertions which may be frequent 
because of a logic complexity in the case of timeouts.

> 
> I think "ignore:" was a kluge we put in twenty-plus years ago when our
> testing standards were a lot lower, and it's way past time we got
> rid of it.
Ok, I will try to invent alternative way for deep (and stable) testing 
of timeouts. Thank you for the answer.

-- 
Regards
Andrey Lepikhov
Postgres Professional

  43. Re: [PATCH] random_normal function

    David E. Wheeler <david@justatheory.com> — 2025-10-04T14:30:05Z

    Hello old thread.

On Jan 19, 2023, at 01:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I think "ignore:" was a kluge we put in twenty-plus years ago when our
> testing standards were a lot lower, and it's way past time we got
> rid of it.

Today I discovered a bad regex in the pgTAP Makefile that matched 18.0 as an 8.0 release and used it `ignore:` a test requiring RLS. I found it after I saw this test failure on Postgres 18:

# syntax error in schedule file "test/build/run.sch" line 3: ignore: policy

I fixed the regex (removed any detection of pre-9.0 matching, in fact), so all good there. But I wanted to point out that `ignore:` is documented in the wiki[1], so other extensions may use it. If no one else notices perhaps it’s not a big deal, but I want to call out that pg_regress features, “documented” or not, might be used outside the core.

Maybe the proper way to address this issue is to add formal docs for pg_regress. I always have the damnedest time finding its docs and usually end up on the Wiki. If there was official documentation for it like there is for `psql`, etc., it might help to prevent such issues in the future. Minor as they may be, admittedly.

Thoughts?

Best,

David

[1]: https://wiki.postgresql.org/wiki/Regression_test_authoring

  44. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-04T17:47:53Z

    "David E. Wheeler" <david@justatheory.com> writes:
> Hello old thread.
> On Jan 19, 2023, at 01:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think "ignore:" was a kluge we put in twenty-plus years ago when our
>> testing standards were a lot lower, and it's way past time we got
>> rid of it.

> I fixed the regex (removed any detection of pre-9.0 matching, in fact), so all good there. But I wanted to point out that `ignore:` is documented in the wiki[1], so other extensions may use it. If no one else notices perhaps it’s not a big deal, but I want to call out that pg_regress features, “documented” or not, might be used outside the core.

I didn't know about that page, but I'm happy to go remove its
mention of "ignore:" ...

			regards, tom lane

  45. Re: [PATCH] random_normal function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-04T18:07:45Z

    I wrote:
> I didn't know about that page, but I'm happy to go remove its
> mention of "ignore:" ...

Actually, there is a *ton* of obsolete information on that page:
there's no serial_schedule file anymore, and we don't have
dynamically-generated test files anymore (input/output subdirectories)
and therefore there's no substituted variables.  The age of the whole
thing can be dated by its references to CVS :-(

Maybe we should indeed try to replace it with something in the main
docs, which are a bit more likely to get maintained.  There is an
existing chapter "Regression Tests", but it presently only talks
about how to run the tests not how to write new ones.

			regards, tom lane

  46. Re: [PATCH] random_normal function

    Michael Paquier <michael@paquier.xyz> — 2025-10-06T04:30:01Z

    On Sat, Oct 04, 2025 at 02:07:45PM -0400, Tom Lane wrote:
> Actually, there is a *ton* of obsolete information on that page:
> there's no serial_schedule file anymore, and we don't have
> dynamically-generated test files anymore (input/output subdirectories)
> and therefore there's no substituted variables.  The age of the whole
> thing can be dated by its references to CVS :-(
> 
> Maybe we should indeed try to replace it with something in the main
> docs, which are a bit more likely to get maintained.  There is an
> existing chapter "Regression Tests", but it presently only talks
> about how to run the tests not how to write new ones.

Yes, most of the content of this page could be deleted.

IMO, the main docs would benefit greatly from the addition of
PG_ABS_BUILDDIR, PG_ABS_BUILDDIR, PG_LIBDIR and PG_DLSUFFIX, which are
still missing.  These are available to extension developers through
installcheck, and are a great way to write more flexible tests with
arbitrary inputs, like a dependency to on-disk files.  I would
certainly +1 this addition, at least.
--
Michael

  47. Re: [PATCH] random_normal function

    David E. Wheeler <david@justatheory.com> — 2025-10-19T18:58:36Z

    On Oct 6, 2025, at 06:30, Michael Paquier <michael@paquier.xyz> wrote:

> IMO, the main docs would benefit greatly from the addition of
> PG_ABS_BUILDDIR, PG_ABS_BUILDDIR, PG_LIBDIR and PG_DLSUFFIX, which are
> still missing.  These are available to extension developers through
> installcheck, and are a great way to write more flexible tests with
> arbitrary inputs, like a dependency to on-disk files.  I would
> certainly +1 this addition, at least.

I’ve never heard of these! Seems like there’s an awful lot of stuff in the testing infrastructure that would be useful to document.

D