[Peter Eisentraut <peter.eisentraut@2ndquadrant.com>]

Adjacent to the discussion in [0] I wanted to document the factorial() 
function and expand the tests for that slightly with some edge cases.

I noticed that the current implementation returns 1 for the factorial of 
all negative numbers:

SELECT factorial(-4);
  factorial
-----------
          1

While there are some advanced mathematical constructions that define 
factorials for negative numbers, they certainly produce different 
answers than this.

Curiously, before the reimplementation of factorial using numeric 
(04a4821adef38155b7920ba9eb83c4c3c29156f8), it returned 0 for negative 
numbers, which is also not correct by any theory I could find.

I propose to change this to error out for negative numbers.

See attached patches for test and code changes.


[0]: 
https://www.postgresql.org/message-id/flat/38ca86db-42ab-9b48-2902-337a0d6b8311%402ndquadrant.com

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments: download all (3 files, .tar.gz)

0001-doc-Document-factorial-function.patch (text/plain, 1.2K) download
From 700f6618fdbd72defcfcd597a00690ce30e7dba1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jun 2020 08:51:46 +0200
Subject: [PATCH 1/3] doc: Document factorial function

This has existed for a very long time, equivalent to the ! and !!
operators, but it was never documented.
---
 doc/src/sgml/func.sgml | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index b65aa28f34..5006c35fb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1347,6 +1347,23 @@ <title>Mathematical Functions</title>
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>factorial</primary>
+        </indexterm>
+        <function>factorial</function> ( <type>numeric</type> )
+        <returnvalue>numeric</returnvalue>
+       </para>
+       <para>
+        Factorial
+       </para>
+       <para>
+        <literal>factorial(5)</literal>
+        <returnvalue>120</returnvalue>
+       </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
-- 
2.27.0



0002-Expand-tests-for-factorial.patch (text/plain, 2.9K) download
From 4e8b05d209f61be222c7eb776edffa206b706299 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jun 2020 08:59:19 +0200
Subject: [PATCH 2/3] Expand tests for factorial

Move from int4 to numeric test.  (They were originally int4 functions,
but were reimplemented for numeric in
04a4821adef38155b7920ba9eb83c4c3c29156f8.)  Add some tests for edge
cases.
---
 src/test/regress/expected/int4.out    | 12 ---------
 src/test/regress/expected/numeric.out | 35 +++++++++++++++++++++++++++
 src/test/regress/sql/int4.sql         |  4 ---
 src/test/regress/sql/numeric.sql      | 10 ++++++++
 4 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/src/test/regress/expected/int4.out b/src/test/regress/expected/int4.out
index c384af18ee..77f43739a7 100644
--- a/src/test/regress/expected/int4.out
+++ b/src/test/regress/expected/int4.out
@@ -299,18 +299,6 @@ SELECT int4 '1000' < int4 '999' AS false;
  f
 (1 row)
 
-SELECT 4! AS twenty_four;
- twenty_four 
--------------
-          24
-(1 row)
-
-SELECT !!3 AS six;
- six 
------
-   6
-(1 row)
-
 SELECT 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 AS ten;
  ten 
 -----
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
index c7fe63d037..05e9a2b9e9 100644
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -2315,3 +2315,38 @@ FROM (VALUES (0::numeric, 0::numeric),
 
 SELECT lcm(9999 * (10::numeric)^131068 + (10::numeric^131068 - 1), 2); -- overflow
 ERROR:  value overflows numeric format
+--
+-- Tests for factorial
+--
+SELECT 4!;
+ ?column? 
+----------
+       24
+(1 row)
+
+SELECT !!3;
+ ?column? 
+----------
+        6
+(1 row)
+
+SELECT factorial(15);
+   factorial   
+---------------
+ 1307674368000
+(1 row)
+
+SELECT 100000!;
+ERROR:  value overflows numeric format
+SELECT 0!;
+ ?column? 
+----------
+        1
+(1 row)
+
+SELECT factorial(-4);
+ factorial 
+-----------
+         1
+(1 row)
+
diff --git a/src/test/regress/sql/int4.sql b/src/test/regress/sql/int4.sql
index a9e90a96c4..b00c9dea2a 100644
--- a/src/test/regress/sql/int4.sql
+++ b/src/test/regress/sql/int4.sql
@@ -114,10 +114,6 @@ CREATE TABLE INT4_TBL(f1 int4);
 
 SELECT int4 '1000' < int4 '999' AS false;
 
-SELECT 4! AS twenty_four;
-
-SELECT !!3 AS six;
-
 SELECT 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 + 1 AS ten;
 
 SELECT 2 + 2 / 2 AS three;
diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql
index 41475a9a24..483c81b9cd 100644
--- a/src/test/regress/sql/numeric.sql
+++ b/src/test/regress/sql/numeric.sql
@@ -1111,3 +1111,13 @@ CREATE TABLE num_input_test (n1 numeric);
              (4232.820::numeric, 132.72000::numeric)) AS v(a, b);
 
 SELECT lcm(9999 * (10::numeric)^131068 + (10::numeric^131068 - 1), 2); -- overflow
+
+--
+-- Tests for factorial
+--
+SELECT 4!;
+SELECT !!3;
+SELECT factorial(15);
+SELECT 100000!;
+SELECT 0!;
+SELECT factorial(-4);
-- 
2.27.0



0003-Disallow-factorial-of-negative-numbers.patch (text/plain, 1.4K) download
From 9ae0d1135d0a75b4560f2a4c22be8b8bbfde0803 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 15 Jun 2020 09:02:14 +0200
Subject: [PATCH 3/3] Disallow factorial of negative numbers

The previous implementation returned 1 for all negative numbers, which
is not sensible under any definition.
---
 src/backend/utils/adt/numeric.c       | 6 +++++-
 src/test/regress/expected/numeric.out | 6 +-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index eea4239854..f4fb438669 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -2946,7 +2946,11 @@ numeric_fac(PG_FUNCTION_ARGS)
 	NumericVar	fact;
 	NumericVar	result;
 
-	if (num <= 1)
+	if (num < 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+				 errmsg("factorial of negative numbers is undefined")));
+	if (num == 0 || num == 1)
 	{
 		res = make_result(&const_one);
 		PG_RETURN_NUMERIC(res);
diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out
index 05e9a2b9e9..cff9cde5e8 100644
--- a/src/test/regress/expected/numeric.out
+++ b/src/test/regress/expected/numeric.out
@@ -2345,8 +2345,4 @@ SELECT 0!;
 (1 row)
 
 SELECT factorial(-4);
- factorial 
------------
-         1
-(1 row)
-
+ERROR:  factorial of negative numbers is undefined
-- 
2.27.0