Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

I don't want to go too deep into it, but you get stuff like this:

Select pow(2.0, -3)::text = pow(2, -3)::text;


Sure. It does so with any overloaded operator or function:

  fabien=# SELECT (2.0 + 3)::TEXT = (2 + 3)::TEXT; # f

Patch applies, make check ok in pgbench, doc gen ok.

ipow code is nice and simple.

I switched the patch to "Ready for Committer"

Let's now hope that a committer gets around to consider these patch some 
day.


--
Fabien.


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

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi,


Indeed, this is quite strange...


 I don't want to go too deep into it, but you get stuff like this:

Select pow(2.0, -3)::text = pow(2, -3)::text;
 ?column?
--
 f
(1 row)


 - you can simplify the ipow function by removing handling of y<0 case,
>maybe add an assert to be sure to avoid it.

I agree, done.

 - you should add more symmetry and simplify the evaluation:

Done too.

 Add a test case to show what happens on NULL arguments, hopefully the
> result is NULL.

Done and it does.

Thanks again for the review.

On Mon, Nov 6, 2017 at 4:14 PM, Fabien COELHO  wrote:

>
> Hello,
>
> Sorry for the confusion, I wasn't aware that SQL pow changed types
>> depending on the input value.
>>
>
> Indeed, this is quite strange...
>
>   fabien=# SELECT i, POW(2, i) FROM generate_series(-2, 2) AS i;
>-2 | 0.25
>-1 | 0.5
> 0 | 1
> 1 | 2
> 2 | 4
>
> I've modified the function to match more closely the behaviour of SQL,
>> except that 0^(negative) returns 'double inf'. Do you think there is any
>> value in raising an error instead?
>>
>
>   fabien=# SELECT POW(0,-1);
>   ERROR:  zero raised to a negative power is undefined
>
> H... I'm fine with double inf, because exception in pgbench means the
> end of the script, which is not desirable for benchmarking purposes.
>
> I think that:
>
>  - you can simplify the ipow function by removing handling of y<0 case,
>maybe add an assert to be sure to avoid it.
>
>  - you should add more symmetry and simplify the evaluation:
>
>if (int & int)
>{
>   i1, i2 = ...;
>   if (i2 >= 0)
> setIntValue(retval, ipow(i1, i2));
>   else
> // conversion is done by C, no need to coerce again
> setDoubleValue(retval, pow(i1, i2));
>}
>else
>{
>  d1, d2 = ...;
>  setDoubleValue(retval, pow(d1, d2));
>}
>
> Add a test case to show what happens on NULL arguments, hopefully the
> result is NULL.
>
> --
> Fabien.
>



-- 

*Raúl Marín Rodríguez *carto.com
From 47f6ec396d3bc11c39066dbd4b31e75b76102094 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Fri, 13 Oct 2017 17:42:23 +0200
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 
 src/bin/pgbench/exprparse.y  |  3 ++
 src/bin/pgbench/pgbench.c| 62 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 15 +++
 5 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1f55967e40a..32c94ba0dc1 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1233,6 +1233,13 @@ pgbench  options  d
sqrt(2.0)
1.414213562
   
+  
+   pow(x, y)
+   integer if x and y are integers and y >= 0, else double
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0
+  
  
  

diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 770be981f06..290bca99d12 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -334,6 +334,9 @@ static const struct
 	{
 		"!case_end", -2, PGBENCH_CASE
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index add653bf90c..3781e75721d 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -739,6 +739,27 @@ getPoissonRand(TState *thread, int64 center)
 }
 
 /*
+ * pow() for integer values with exp >= 0. Matches SQL pow() behaviour
+ */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result = 1;
+
+	Assert(exp >= 0);
+
+	while (exp)
+	{
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
+/*
  * Initialize the given SimpleStats struct to all zeroes
  */
 static void
@@ -1918,6 +1939,47 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+
+Assert(nargs == 2);
+
+/*
+ * If both operands are int and exp >= 0 use
+ * the ipow() function, else use pow()
+ */
+if (lval->type == PGBT_INT &&
+	 rval->type == PGBT_INT)
+{
+
+	int64		li,
+ri;
+
+	if (!coerceToInt(lval, ) ||
+		!coerceToInt(rval, ))
+		return false;
+
+	if (ri >= 0)
+		setIntValue(retval, ipow(li, ri));
+	else
+		setDoubleValue(retval, pow(li, ri));
+}
+else
+{
+	double		ld,
+rd;
+
+	if (!coerceToDouble(lval, ) ||
+		!coerceToDouble(rval, ))
+		return false;
+
+	setDoubleValue(retval, pow(ld, rd));
+}
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index e1277a1dde6..9f26af92bf6 

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello,

Sorry for the confusion, I wasn't aware that SQL pow changed types 
depending on the input value.


Indeed, this is quite strange...

  fabien=# SELECT i, POW(2, i) FROM generate_series(-2, 2) AS i;
   -2 | 0.25
   -1 | 0.5
0 | 1
1 | 2
2 | 4

I've modified the function to match more closely the behaviour of SQL, 
except that 0^(negative) returns 'double inf'. Do you think there is any 
value in raising an error instead?


  fabien=# SELECT POW(0,-1);
  ERROR:  zero raised to a negative power is undefined

H... I'm fine with double inf, because exception in pgbench means the 
end of the script, which is not desirable for benchmarking purposes.


I think that:

 - you can simplify the ipow function by removing handling of y<0 case,
   maybe add an assert to be sure to avoid it.

 - you should add more symmetry and simplify the evaluation:

   if (int & int)
   {
  i1, i2 = ...;
  if (i2 >= 0)
setIntValue(retval, ipow(i1, i2));
  else
// conversion is done by C, no need to coerce again
setDoubleValue(retval, pow(i1, i2));
   }
   else
   {
 d1, d2 = ...;
 setDoubleValue(retval, pow(d1, d2));
   }

Add a test case to show what happens on NULL arguments, hopefully the 
result is NULL.


--
Fabien.


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

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi Fabien,

Sorry for the confusion, I wasn't aware that SQL pow changed types
depending on
the input value.

I've modified the function to match more closely the behaviour of SQL,
except
that 0^(negative) returns 'double inf'. Do you think there is any value in
raising an error instead?


On Mon, Nov 6, 2017 at 2:12 PM, Fabien COELHO  wrote:

>
> Hello Raúl,
>
> I've fixed the documentation and added an ipow function that handles both
>> positive and negative ints, having 0^0 == 1 and 0^(negative) ==
>> PG_INT64_MAX
>> since that's what my glibc math.h pow() is returning.
>>
>
> From the comment:
>
>  * For exp < 0 return 0 except when the base is 1 or -1
>
> I think that it should do what POW does in psql, i.e.:
>
>  fabien=# SELECT POW(2, -2); # 0.25
>
> that is if exp < 0 the double version should be used, it should
> not return 0.
>
> Basically the idea is that the pgbench client-side version should behave
> the same as the SQL version.
>
> --
> Fabien.




-- 

*Raúl Marín Rodríguez*carto.com
From be2fedcae277f7eede621dcda66b15b08372ce63 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Fri, 13 Oct 2017 17:42:23 +0200
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 +++
 src/bin/pgbench/exprparse.y  |  3 +
 src/bin/pgbench/pgbench.c| 84 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 13 +
 5 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1f55967e40a..32c94ba0dc1 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1233,6 +1233,13 @@ pgbench  options  d
sqrt(2.0)
1.414213562
   
+  
+   pow(x, y)
+   integer if x and y are integers and y >= 0, else double
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0
+  
  
  

diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 770be981f06..290bca99d12 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -334,6 +334,9 @@ static const struct
 	{
 		"!case_end", -2, PGBENCH_CASE
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index add653bf90c..d565880b6e2 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -739,6 +739,51 @@ getPoissonRand(TState *thread, int64 center)
 }
 
 /*
+ * pow() for integer values
+ */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result;
+
+	if (base == 0)
+	{
+		if (exp > 0)
+			return 0;
+		else if (exp == 0)
+			return 1;
+		return PG_INT64_MAX;
+	}
+
+	/*
+	 * For exp > 0 calculate normally
+	 * For exp == 0 return 1 * sign of base
+	 * For exp < 0 return 0 except when the base is 1 or -1
+	 */
+	if (exp > 0)
+	{
+		result = 1;
+		while (exp)
+		{
+			if (exp & 1)
+result *= base;
+			exp >>= 1;
+			base *= base;
+		}
+	}
+	else if (exp == 0)
+		result = (base > 0) - (base < 0);
+	else
+	{
+		result = 1 / base;
+		if (exp % 2 == 0)
+			result *= result;
+	}
+
+	return result;
+}
+
+/*
  * Initialize the given SimpleStats struct to all zeroes
  */
 static void
@@ -1918,6 +1963,45 @@ evalFunc(TState *thread, CState *st,
 return true;
 			}
 
+		case PGBENCH_POW:
+			{
+PgBenchValue *lval = [0];
+PgBenchValue *rval = [1];
+double		ld,
+			rd;
+
+Assert(nargs == 2);
+
+/*
+ * If both operands are int and exp >= 0 use
+ * the ipow() function, else use pow()
+ */
+if (lval->type == PGBT_INT &&
+	 rval->type == PGBT_INT)
+{
+
+	int64		li,
+ri;
+
+	if (!coerceToInt(lval, ) ||
+		!coerceToInt(rval, ))
+		return false;
+
+	if (ri >= 0)
+	{
+		setIntValue(retval, ipow(li, ri));
+		return true;
+	}
+}
+
+if (!coerceToDouble(lval, ) ||
+	!coerceToDouble(rval, ))
+	return false;
+
+setDoubleValue(retval, pow(ld, rd));
+return true;
+			}
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index e1277a1dde6..9f26af92bf6 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -94,7 +94,8 @@ typedef enum PgBenchFunction
 	PGBENCH_LE,
 	PGBENCH_LT,
 	PGBENCH_IS,
-	PGBENCH_CASE
+	PGBENCH_CASE,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 8e19bbd3f45..2a1bb1216d7 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -237,6 +237,12 @@ sub pgbench
 		qr{command=36.: int 36\b},
 		qr{command=37.: boolean true\b},
 		qr{command=38.: boolean true\b},
+		qr{command=44.: int 

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi Fabien,

Thanks for the review.
I've fixed the documentation and added an ipow function that handles both
positive and negative ints, having 0^0 == 1 and 0^(negative) == PG_INT64_MAX
since that's what my glibc math.h pow() is returning.

On Sat, Nov 4, 2017 at 12:34 PM, Fabien COELHO  wrote:

>
> Hello Raúl,
>
> Sorry about the patch. Attaching it now so it can be considered as
>> submitted.
>>
>
> There is a typo in the XML doc:
>
> 1024.0/
>
> Please check that the documentation compiles.
>
> I'm at odds with having the integer version rely on a double pow(), even
> if it works. I think that there should be a specific integer version which
> does use integer operations. From stack overflow, the following is
> suggested:
>
>  int ipow(int base, int exp)
>  {
> int result = 1;
> while (exp)
> {
> if (exp & 1)
> result *= base;
> exp >>= 1;
> base *= base;
> }
>
> return result;
>  }
>
> The integer version should be when x & y are integers *AND* y >= 0.
>
> if y is a negative integer, the double version should be used.
>
> --
> Fabien.




-- 

*Raúl Marín Rodríguez*carto.com
From 09105f8108e439834510ee5fb53036473f2977d5 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Fri, 13 Oct 2017 17:42:23 +0200
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml|  7 
 src/bin/pgbench/exprparse.y  |  3 ++
 src/bin/pgbench/pgbench.c| 54 
 src/bin/pgbench/pgbench.h|  3 +-
 src/bin/pgbench/t/001_pgbench_with_server.pl |  9 +
 5 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1f55967e40a..4e7f75ddf87 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1233,6 +1233,13 @@ pgbench  options  d
sqrt(2.0)
1.414213562
   
+  
+   pow(x, y)
+   double if x or y are doubles, else integer
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0
+  
  
  

diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 770be981f06..290bca99d12 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -334,6 +334,9 @@ static const struct
 	{
 		"!case_end", -2, PGBENCH_CASE
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index add653bf90c..bdf3e97682a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -739,6 +739,51 @@ getPoissonRand(TState *thread, int64 center)
 }
 
 /*
+ * pow() for integer values
+ */
+static int64
+ipow(int64 base, int64 exp)
+{
+	int64 result;
+
+	if (base == 0)
+	{
+		if (exp > 0)
+			return 0;
+		else if (exp == 0)
+			return 1;
+		return PG_INT64_MAX;
+	}
+
+	/*
+	 * For exp > 0 calculate normally
+	 * For exp == 0 return 1 * sign of base
+	 * For exp < 0 return 0 except when the base is 1 or -1
+	 */
+	if (exp > 0)
+	{
+		result = 1;
+		while (exp)
+		{
+			if (exp & 1)
+result *= base;
+			exp >>= 1;
+			base *= base;
+		}
+	}
+	else if (exp == 0)
+		result = (base > 0) - (base < 0);
+	else
+	{
+		result = 1 / base;
+		if (exp % 2 == 0)
+			result *= result;
+	}
+
+	return result;
+}
+
+/*
  * Initialize the given SimpleStats struct to all zeroes
  */
 static void
@@ -1474,6 +1519,7 @@ evalFunc(TState *thread, CState *st,
 		case PGBENCH_NE:
 		case PGBENCH_LE:
 		case PGBENCH_LT:
+		case PGBENCH_POW:
 			{
 PgBenchValue *lval = [0],
 		   *rval = [1];
@@ -1525,6 +1571,10 @@ evalFunc(TState *thread, CState *st,
 			setBoolValue(retval, ld < rd);
 			return true;
 
+		case PGBENCH_POW:
+			setDoubleValue(retval, pow(ld, rd));
+			return true;
+
 		default:
 			/* cannot get here */
 			Assert(0);
@@ -1602,6 +1652,10 @@ evalFunc(TState *thread, CState *st,
 
 			return true;
 
+		case PGBENCH_POW:
+			setIntValue(retval, ipow(li, ri));
+			return true;
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index e1277a1dde6..9f26af92bf6 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -94,7 +94,8 @@ typedef enum PgBenchFunction
 	PGBENCH_LE,
 	PGBENCH_LT,
 	PGBENCH_IS,
-	PGBENCH_CASE
+	PGBENCH_CASE,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 8e19bbd3f45..c5ecd749c40 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -237,6 +237,10 @@ sub pgbench
 		qr{command=36.: int 36\b},
 		qr{command=37.: boolean true\b},
 		qr{command=38.: boolean true\b},
+		qr{command=44.: int 

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Raúl,


I've fixed the documentation and added an ipow function that handles both
positive and negative ints, having 0^0 == 1 and 0^(negative) == PG_INT64_MAX
since that's what my glibc math.h pow() is returning.



From the comment:


 * For exp < 0 return 0 except when the base is 1 or -1

I think that it should do what POW does in psql, i.e.:

 fabien=# SELECT POW(2, -2); # 0.25

that is if exp < 0 the double version should be used, it should
not return 0.

Basically the idea is that the pgbench client-side version should behave 
the same as the SQL version.


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

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Raúl,


Sorry about the patch. Attaching it now so it can be considered as
submitted.


There is a typo in the XML doc:

1024.0/

Please check that the documentation compiles.

I'm at odds with having the integer version rely on a double pow(), even 
if it works. I think that there should be a specific integer version which 
does use integer operations. From stack overflow, the following is 
suggested:


 int ipow(int base, int exp)
 {
int result = 1;
while (exp)
{
if (exp & 1)
result *= base;
exp >>= 1;
base *= base;
}

return result;
 }

The integer version should be when x & y are integers *AND* y >= 0.

if y is a negative integer, the double version should be used.

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

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 09/18/2017 07:04 PM, Jeff Janes wrote:> You fixed the first issue, 
but I still get the second one:


be-secure-gnutls.c: In function 'get_peer_certificate':
be-secure-gnutls.c:667: error: 'GNUTLS_X509_CRT_LIST_SORT' undeclared 
(first use in this function)
be-secure-gnutls.c:667: error: (Each undeclared identifier is reported 
only once

be-secure-gnutls.c:667: error: for each function it appears in.)


Thanks again for testing the code. I have now rebased the patch and 
fixed the second issue. I tested that it works on CentOS 6.


Work which remains:

- sslinfo
- pgcrypto
- Documentation
- Decide if what I did with the config is a good idea

Andreas
diff --git a/configure b/configure
index 4ecd2e1922..1ba34dfced 100755
--- a/configure
+++ b/configure
@@ -709,6 +709,7 @@ UUID_EXTRA_OBJS
 with_uuid
 with_systemd
 with_selinux
+with_gnutls
 with_openssl
 krb_srvtab
 with_python
@@ -837,6 +838,7 @@ with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
+with_gnutls
 with_selinux
 with_systemd
 with_readline
@@ -1531,6 +1533,7 @@ Optional Packages:
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
+  --with-gnutls   build with GnuTS support
   --with-selinux  build with SELinux support
   --with-systemd  build with systemd support
   --without-readline  do not use GNU Readline nor BSD Libedit for editing
@@ -5997,6 +6000,41 @@ fi
 $as_echo "$with_openssl" >&6; }
 
 
+#
+# GnuTLS
+#
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with GnuTLS support" >&5
+$as_echo_n "checking whether to build with GnuTLS support... " >&6; }
+
+
+
+# Check whether --with-gnutls was given.
+if test "${with_gnutls+set}" = set; then :
+  withval=$with_gnutls;
+  case $withval in
+yes)
+
+$as_echo "#define USE_GNUTLS 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-gnutls option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_gnutls=no
+
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_gnutls" >&5
+$as_echo "$with_gnutls" >&6; }
+
+
 #
 # SELinux
 #
@@ -10164,6 +10202,94 @@ done
 
 fi
 
+if test "$with_gnutls" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing gnutls_init" >&5
+$as_echo_n "checking for library containing gnutls_init... " >&6; }
+if ${ac_cv_search_gnutls_init+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char gnutls_init ();
+int
+main ()
+{
+return gnutls_init ();
+  ;
+  return 0;
+}
+_ACEOF
+for ac_lib in '' gnutls; do
+  if test -z "$ac_lib"; then
+ac_res="none required"
+  else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
+  fi
+  if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_search_gnutls_init=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+  if ${ac_cv_search_gnutls_init+:} false; then :
+  break
+fi
+done
+if ${ac_cv_search_gnutls_init+:} false; then :
+
+else
+  ac_cv_search_gnutls_init=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_gnutls_init" >&5
+$as_echo "$ac_cv_search_gnutls_init" >&6; }
+ac_res=$ac_cv_search_gnutls_init
+if test "$ac_res" != no; then :
+  test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
+
+else
+  as_fn_error $? "library 'gnutls' is required for GnuTLS" "$LINENO" 5
+fi
+
+  # GnuTLS versions before 3.4.0 do not support sorting incorrectly sorted
+  # certificate chains.
+  ac_fn_c_check_decl "$LINENO" "GNUTLS_X509_CRT_LIST_SORT" "ac_cv_have_decl_GNUTLS_X509_CRT_LIST_SORT" "#include 
+#include 
+
+"
+if test "x$ac_cv_have_decl_GNUTLS_X509_CRT_LIST_SORT" = xyes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_GNUTLS_X509_CRT_LIST_SORT $ac_have_decl
+_ACEOF
+
+  for ac_func in gnutls_pkcs11_set_pin_function
+do :
+  ac_fn_c_check_func "$LINENO" "gnutls_pkcs11_set_pin_function" "ac_cv_func_gnutls_pkcs11_set_pin_function"
+if test "x$ac_cv_func_gnutls_pkcs11_set_pin_function" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_GNUTLS_PKCS11_SET_PIN_FUNCTION 1
+_ACEOF
+
+fi
+done
+
+fi
+
 if test "$with_pam" = yes ; then
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for pam_start in -lpam" >&5
 $as_echo_n "checking for pam_start in -lpam... " >&6; }
@@ -10961,6 +11087,17 @@ else
 fi
 
 
+fi
+
+if test "$with_gnutls" = yes ; then
+  ac_fn_c_check_header_mongrel "$LINENO" "gnutls/gnutls.h" "ac_cv_header_gnutls_gnutls_h" "$ac_includes_default"
+if test 

Re: [HACKERS] pow support for pgbench

[Fabien COELHO]

Hello Michaël,

I'm fine with having pow in pgbench.


I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.


It might be even better if https://commitfest.postgresql.org/15/985/, 
which has been around for over one year (!), get through some day...


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

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Sorry about the patch. Attaching it now so it can be considered as
submitted.

-- 

*Raúl Marín Rodríguez*carto.com
From a6eecfe6637bdb0cbb02a9eda7b88d9c4325bb51 Mon Sep 17 00:00:00 2001
From: Raul Marin 
Date: Fri, 13 Oct 2017 17:42:23 +0200
Subject: [PATCH] Add pow() support to pgbench

---
 doc/src/sgml/ref/pgbench.sgml| 7 +++
 src/bin/pgbench/exprparse.y  | 3 +++
 src/bin/pgbench/pgbench.c| 9 +
 src/bin/pgbench/pgbench.h| 3 ++-
 src/bin/pgbench/t/001_pgbench_with_server.pl | 5 +
 5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 1f55967e40a..2e913dfcfd6 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -1233,6 +1233,13 @@ pgbench  options  d
sqrt(2.0)
1.414213562
   
+  
+   pow(x, y)
+   double if x or y are doubles, else integer
+   Numeric exponentiation
+   pow(2.0, 10)
+   1024.0/
+  
  
  

diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index 770be981f06..290bca99d12 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -334,6 +334,9 @@ static const struct
 	{
 		"!case_end", -2, PGBENCH_CASE
 	},
+	{
+		"pow", 2, PGBENCH_POW
+	},
 	/* keep as last array element */
 	{
 		NULL, 0, 0
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index add653bf90c..b2bab9b8239 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1474,6 +1474,7 @@ evalFunc(TState *thread, CState *st,
 		case PGBENCH_NE:
 		case PGBENCH_LE:
 		case PGBENCH_LT:
+		case PGBENCH_POW:
 			{
 PgBenchValue *lval = [0],
 		   *rval = [1];
@@ -1525,6 +1526,10 @@ evalFunc(TState *thread, CState *st,
 			setBoolValue(retval, ld < rd);
 			return true;
 
+		case PGBENCH_POW:
+			setDoubleValue(retval, pow(ld, rd));
+			return true;
+
 		default:
 			/* cannot get here */
 			Assert(0);
@@ -1602,6 +1607,10 @@ evalFunc(TState *thread, CState *st,
 
 			return true;
 
+		case PGBENCH_POW:
+			setIntValue(retval, pow(li, ri));
+			return true;
+
 		default:
 			/* cannot get here */
 			Assert(0);
diff --git a/src/bin/pgbench/pgbench.h b/src/bin/pgbench/pgbench.h
index e1277a1dde6..9f26af92bf6 100644
--- a/src/bin/pgbench/pgbench.h
+++ b/src/bin/pgbench/pgbench.h
@@ -94,7 +94,8 @@ typedef enum PgBenchFunction
 	PGBENCH_LE,
 	PGBENCH_LT,
 	PGBENCH_IS,
-	PGBENCH_CASE
+	PGBENCH_CASE,
+	PGBENCH_POW
 } PgBenchFunction;
 
 typedef struct PgBenchExpr PgBenchExpr;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 8e19bbd3f45..81b7ce4dfcc 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -237,6 +237,8 @@ sub pgbench
 		qr{command=36.: int 36\b},
 		qr{command=37.: boolean true\b},
 		qr{command=38.: boolean true\b},
+		qr{command=44.: int -27\b},
+		qr{command=45.: double 1024\b},
 	],
 	'pgbench expressions',
 	{   '001_pgbench_expressions' => q{-- integer functions
@@ -299,6 +301,9 @@ sub pgbench
 \set v2 5432
 \set v3 -54.21E-2
 SELECT :v0, :v1, :v2, :v3;
+--- pow() operator
+\set poweri debug(pow(-3,3))
+\set powerd debug(pow(2.0,10))
 } });
 
 =head

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

Re: [HACKERS] pow support for pgbench

[Raúl Marín Rodríguez]

Hi,

both patches seem complementary. I've rebased my changes on top of that
patch
(v14) in https://git.io/vFtnT and everything seems to be working fine.

On Mon, Oct 30, 2017 at 12:36 PM, Michael Paquier  wrote:

> On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
>  wrote:
> > Michael Paquier wrote:
> >
> >> Please add this patch to the upcoming commit fest if you would like to
> >> get some feedback:
> >> https://commitfest.postgresql.org/15/
> >>
> >> I am adding as well Fabien in CC who worked in getting the internal
> >> function infrastructure in the shape it is now (waaay better) with
> >> commit 86c43f4.
> >
> > I think Raúl would do well to review this patch by Fabien
> > https://www.postgresql.org/message-id/alpine.DEB.2.20.
> 1710201835390.15170@lancre
> > which adds a few functions and operators.
>
> Good idea. pow() is not added by Fabien's patch, but an operator for
> pow() could be something to add as well.
> --
> Michael
>



-- 

*Raúl Marín Rodríguez*carto.com

Re: [HACKERS] pow support for pgbench

[Alvaro Herrera]

Michael Paquier wrote:

> Attaching patches directly to a thread is a better practice as if
> github goes away, any Postgres developers can still have an access to
> any code you publish using the public archives on postgresql.org.

Also, by posting to pgsql-hackers indicating intention to integrate to
Postgres you're implicitly making a statement about the license of your
contribution; see the second half of the last paragraph at
https://wiki.postgresql.org/wiki/Archives_Policy

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Mon, Oct 30, 2017 at 11:56 AM, Raúl Marín Rodríguez
 wrote:
> both patches seem complementary. I've rebased my changes on top of that
> patch
> (v14) in https://git.io/vFtnT and everything seems to be working fine.

Attaching patches directly to a thread is a better practice as if
github goes away, any Postgres developers can still have an access to
any code you publish using the public archives on postgresql.org.
-- 
Michael


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

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Mon, Oct 30, 2017 at 11:07 AM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>
>> Please add this patch to the upcoming commit fest if you would like to
>> get some feedback:
>> https://commitfest.postgresql.org/15/
>>
>> I am adding as well Fabien in CC who worked in getting the internal
>> function infrastructure in the shape it is now (waaay better) with
>> commit 86c43f4.
>
> I think Raúl would do well to review this patch by Fabien
> https://www.postgresql.org/message-id/alpine.DEB.2.20.1710201835390.15170@lancre
> which adds a few functions and operators.

Good idea. pow() is not added by Fabien's patch, but an operator for
pow() could be something to add as well.
-- 
Michael


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

Re: [HACKERS] pow support for pgbench

[Alvaro Herrera]

Michael Paquier wrote:

> Please add this patch to the upcoming commit fest if you would like to
> get some feedback:
> https://commitfest.postgresql.org/15/
> 
> I am adding as well Fabien in CC who worked in getting the internal
> function infrastructure in the shape it is now (waaay better) with
> commit 86c43f4.

I think Raúl would do well to review this patch by Fabien
https://www.postgresql.org/message-id/alpine.DEB.2.20.1710201835390.15170@lancre
which adds a few functions and operators.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] pow support for pgbench

[Michael Paquier]

On Fri, Oct 27, 2017 at 4:51 PM, Raúl Marín Rodríguez
 wrote:
> I've written a small patch to add support for pow() in pgbench.

Cool.

> The main reason behind it is that I'm currently using a shell call to do it
> which takes between 2-10 ms that can be a big percentage of the time taken
> by the whole transaction. For example (shortened):
>
> latency average = 11.718 ms
>  - statement latencies in milliseconds:
>  2.834  \setshell POWER2  awk 'BEGIN {p=2^ARGV[1]; print p }'
> :ZOOM_CURRENT
>  8.846  SELECT
> ST_AsBinary(ST_Simplify(ST_SnapToGrid("the_geom_webmercator",:SNAP),
> :SIMPLIFY)) AS geom FROM
>
> I've also updated the related docs and added some tests. Please let me know
> if I'm missing anything.

Please add this patch to the upcoming commit fest if you would like to
get some feedback:
https://commitfest.postgresql.org/15/

I am adding as well Fabien in CC who worked in getting the internal
function infrastructure in the shape it is now (waaay better) with
commit 86c43f4.
-- 
Michael


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/10/26 16:40, Etsuro Fujita wrote:
Other changes I made 
to the executor are: (1) currently, we set the RT index for the root 
partitioned table to ri_RangeTableIndex of partitions' ResultRelInfos, 
but the proposed EXPLAIN requires that the partition's 
ri_RangeTableIndex is set to the RT index for that partition's RTE, to 
show that partition info in the output.  So, I made that change.


One thing I forgot to mention is: that would be also required to call 
BeginForeignModify, ExecForeignInsert, and EndForeignModify with the 
partition's ResultRelInfo.


I updated docs in doc/src/sgml/ddl.sgml the same way as [1].  (I used 
only the ddl.sgml change proposed by [1], not all the changes.)  I did 
some cleanup as well.  Please find attached an updated version of the patch.


Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/b19a8e2b-e000-f592-3e0b-3e90ba0fa816%40lab.ntt.co.jp
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 176,182  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', 
delimiter ',');
--- 176,188 
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
  SELECT tableoid::regclass, * FROM p2;
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 315,321  SELECT tableoid::regclass, * FROM p2;
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter 
','); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
--- 315,321 
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter 
','); -- ERROR
! ERROR:  cannot route copied tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
***
*** 341,348  SELECT tableoid::regclass, * FROM p2;
   p2   | 2 | qux
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
--- 341,366 
   p2   | 2 | qux
  (2 rows)
  
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+  Insert on public.pt
+Foreign Insert on public.p1
+Insert on public.p2
+->  Result
+  Output: 1, 'xyzzy'::text
+ 
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
! \t on
! EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
!  Insert on public.pt
!Foreign Insert on public.p1
!Insert on public.p2
!->  Result
!  Output: 2, 'xyzzy'::text
! 
! \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 7029,7034  NOTICE:  drop cascades to foreign table bar2
--- 7029,7236 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing for foreign-table partitions
+ -- ===
+ create table pt (a int, b int) partition by list (a);
+ create table locp partition of pt for values in (1);
+ create table locfoo (a int check (a in (2)), b int);
+ create foreign table remp partition of pt for values in (2) server loopback 
options (table_name 'locfoo');
+ explain (verbose, costs off)
+ insert into pt values (1, 1), (2, 1);
+QUERY PLAN
+ -
+  Insert on public.pt
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2)
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (6 rows)
+ 
+ insert into pt values (1, 1), (2, 1);
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+  remp | 2 | 1
+ (2 rows)
+ 
+ select tableoid::regclass, * FROM locp;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+ (1 row)
+ 
+ select tableoid::regclass, * FROM remp;
+  tableoid | a | b 
+ 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

Hi Maksim,

On 2017/10/02 21:37, Maksim Milyutin wrote:

On 11.09.2017 16:01, Etsuro Fujita wrote:
* Query planning: the patch creates copies of Query/Plan with a 
foreign partition as target from the original Query/Plan for each 
foreign partition and invokes PlanForeignModify with those copies, to 
allow the FDW to do query planning for remote INSERT with the existing 
API.  To make such Queries the similar way inheritance_planner does, I 
modified transformInsertStmt so that the inh flag for the partitioned 
table's RTE is set to true, which allows (1) expand_inherited_rtentry 
to build an RTE and AppendRelInfo for each partition in the 
partitioned table and (2) make_modifytable to build such Queries using 
adjust_appendrel_attrs and those AppendRelInfos.


* explain.c: I modified show_modifytable_info so that we can show 
remote queries for foreign partitions in EXPLAIN for INSERT into a 
partitioned table the same way as for inherited UPDATE/DELETE cases. 
Here is an example:


postgres=# explain verbose insert into pt values (1), (2);
    QUERY PLAN
---
 Insert on public.pt  (cost=0.00..0.03 rows=2 width=4)
   Foreign Insert on public.fp1
 Remote SQL: INSERT INTO public.t1(a) VALUES ($1)
   Foreign Insert on public.fp2
 Remote SQL: INSERT INTO public.t2(a) VALUES ($1)
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
 Output: "*VALUES*".column1
(7 rows)

I think I should add more explanation about the patch, but I don't 
have time today, so I'll write additional explanation in the next 
email. Sorry about that.


Could you update your patch, it isn't applied on the actual state of 
master. Namely conflict in src/backend/executor/execMain.c


Attached is an updated version.

* As mentioned in "Query planning", the patch builds an RTE for each 
partition so that the FDW can make reference to that RTE in eg, 
PlanForeignModify.  set_plan_references also uses such RTEs to record 
plan dependencies for plancache.c to invalidate cached plans.  See an 
example for that added to the regression tests in postgres_fdw.


* As mentioned in "explain.c", the EXPLAIN shows all partitions beneath 
the ModifyTable node.  One merit of that is we can show remote queries 
for foreign partitions in the output as shown above.  Another one I can 
think of is when reporting execution stats for triggers.  Here is an 
example for that:


postgres=# explain analyze insert into list_parted values (1, 'hi 
there'), (2, 'hi there');

  QUERY PLAN
--
 Insert on list_parted  (cost=0.00..0.03 rows=2 width=36) (actual 
time=0.375..0.375 rows=0 loops=1)

   Insert on part1
   Insert on part2
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=36) 
(actual time=0.004..0.007 rows=2 loops=1)

 Planning time: 0.089 ms
 Trigger part1brtrig on part1: time=0.059 calls=1
 Trigger part2brtrig on part2: time=0.021 calls=1
 Execution time: 0.422 ms
(8 rows)

This would allow the user to understand easily that "part1" and "part2" 
in the trigger lines are the partitions of list_parted.  So, I think 
it's useful to show partition info in the ModifyTable node even in the 
case where a partitioned table only contains plain tables.


* The patch modifies make_modifytable and ExecInitModifyTable so that 
the former can allow the FDW to construct private plan data for each 
foreign partition and accumulate it all into a list, and that the latter 
can perform BeginForeignModify for each partition using that plan data 
stored in the list passed from make_modifytable.  Other changes I made 
to the executor are: (1) currently, we set the RT index for the root 
partitioned table to ri_RangeTableIndex of partitions' ResultRelInfos, 
but the proposed EXPLAIN requires that the partition's 
ri_RangeTableIndex is set to the RT index for that partition's RTE, to 
show that partition info in the output.  So, I made that change. 
Because of that, ExecPartitionCheck, ExecConstraints, and 
ExecWithCheckOptions are adjusted accordingly.  (2) I added a new member 
to ResultRelInfo (ie, ri_PartitionIsValid), and modified 
CheckValidResultRel so that it checks a given partition without throwing 
an error and save the result in that flag so that ExecInsert determines 
using that flag whether a partition chosen by ExecFindPartition is valid 
for tuple-routing as proposed before.


* copy.c: I still don't think it's a good idea to implement COPY 
tuple-routing for foreign partitions using PlanForeignModify.  (I plan 
to propose new FDW APIs for bulkload as discussed before, to implement 
this feature.)  So, I kept that as-is.  Two things I changed there are: 
(1) currently, ExecSetupPartitionTupleRouting verifies partitions using 
CheckValidResultRel, but I don't think we need the 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Maksim Milyutin]

Hi Fujita-san!


On 11.09.2017 16:01, Etsuro Fujita wrote:

Here is an updated version of the patch.

* Query planning: the patch creates copies of Query/Plan with a 
foreign partition as target from the original Query/Plan for each 
foreign partition and invokes PlanForeignModify with those copies, to 
allow the FDW to do query planning for remote INSERT with the existing 
API.  To make such Queries the similar way inheritance_planner does, I 
modified transformInsertStmt so that the inh flag for the partitioned 
table's RTE is set to true, which allows (1) expand_inherited_rtentry 
to build an RTE and AppendRelInfo for each partition in the 
partitioned table and (2) make_modifytable to build such Queries using 
adjust_appendrel_attrs and those AppendRelInfos.


* explain.c: I modified show_modifytable_info so that we can show 
remote queries for foreign partitions in EXPLAIN for INSERT into a 
partitioned table the same way as for inherited UPDATE/DELETE cases.  
Here is an example:


postgres=# explain verbose insert into pt values (1), (2);
    QUERY PLAN
---
 Insert on public.pt  (cost=0.00..0.03 rows=2 width=4)
   Foreign Insert on public.fp1
 Remote SQL: INSERT INTO public.t1(a) VALUES ($1)
   Foreign Insert on public.fp2
 Remote SQL: INSERT INTO public.t2(a) VALUES ($1)
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
 Output: "*VALUES*".column1
(7 rows)

I think I should add more explanation about the patch, but I don't 
have time today, so I'll write additional explanation in the next 
email. Sorry about that.


Could you update your patch, it isn't applied on the actual state of 
master. Namely conflict in src/backend/executor/execMain.c


--
Regards,
Maksim Milyutin



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

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Daniel Gustafsson]

> On 11 Apr 2017, at 10:55, Etsuro Fujita  wrote:
> 
> On 2017/04/07 22:54, Arthur Zakirov wrote:
>> Marked the patch as "Ready for Commiter". But the patch should be
>> commited only after the patch [1].
> 
> Thanks for reviewing!  I'll continue to work on this for PG11.

This patch has been marked Ready for Committer in the current commitfest
without being committed or rejected.  Moving to the next commitfest, but since
it has bitrotted I’m moving it to Waiting for author.

cheers ./daniel

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

Re: [HACKERS] GnuTLS support

[Jeff Janes]

On Sun, Sep 17, 2017 at 2:17 PM, Andreas Karlsson  wrote:

> On 09/15/2017 06:55 PM, Jeff Janes wrote:
>
>> I can't build against gnutls-2.12.23-21.el6.x86_64 from CentOS 6.9
>>
>
> Thanks for testing my patch. I have fixed both these issues plus some of
> the other feedback. A new version of my patch is attached which should, at
> least on theory, support all GnuTLS versions >= 2.11.
>

You fixed the first issue, but I still get the second one:

be-secure-gnutls.c: In function 'get_peer_certificate':
be-secure-gnutls.c:667: error: 'GNUTLS_X509_CRT_LIST_SORT' undeclared
(first use in this function)
be-secure-gnutls.c:667: error: (Each undeclared identifier is reported only
once
be-secure-gnutls.c:667: error: for each function it appears in.)

Cheers,

Jeff

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 09/15/2017 06:55 PM, Jeff Janes wrote:

I can't build against gnutls-2.12.23-21.el6.x86_64 from CentOS 6.9


Thanks for testing my patch. I have fixed both these issues plus some of 
the other feedback. A new version of my patch is attached which should, 
at least on theory, support all GnuTLS versions >= 2.11.


I just very quickly fixed the broken SSL tests, as I am no fan of how 
the SSL tests currently are written and think they should be cleaned up.


Andreas
diff --git a/configure b/configure
index 0d76e5ea42..33b1f00bff 100755
--- a/configure
+++ b/configure
@@ -709,6 +709,7 @@ UUID_EXTRA_OBJS
 with_uuid
 with_systemd
 with_selinux
+with_gnutls
 with_openssl
 krb_srvtab
 with_python
@@ -838,6 +839,7 @@ with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
+with_gnutls
 with_selinux
 with_systemd
 with_readline
@@ -1534,6 +1536,7 @@ Optional Packages:
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
+  --with-gnutls   build with GnuTS support
   --with-selinux  build with SELinux support
   --with-systemd  build with systemd support
   --without-readline  do not use GNU Readline nor BSD Libedit for editing
@@ -6051,6 +6054,41 @@ fi
 $as_echo "$with_openssl" >&6; }
 
 
+#
+# GnuTLS
+#
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with GnuTLS support" >&5
+$as_echo_n "checking whether to build with GnuTLS support... " >&6; }
+
+
+
+# Check whether --with-gnutls was given.
+if test "${with_gnutls+set}" = set; then :
+  withval=$with_gnutls;
+  case $withval in
+yes)
+
+$as_echo "#define USE_GNUTLS 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-gnutls option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_gnutls=no
+
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_gnutls" >&5
+$as_echo "$with_gnutls" >&6; }
+
+
 #
 # SELinux
 #
@@ -10218,6 +10256,83 @@ done
 
 fi
 
+if test "$with_gnutls" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing gnutls_init" >&5
+$as_echo_n "checking for library containing gnutls_init... " >&6; }
+if ${ac_cv_search_gnutls_init+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_func_search_save_LIBS=$LIBS
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char gnutls_init ();
+int
+main ()
+{
+return gnutls_init ();
+  ;
+  return 0;
+}
+_ACEOF
+for ac_lib in '' gnutls; do
+  if test -z "$ac_lib"; then
+ac_res="none required"
+  else
+ac_res=-l$ac_lib
+LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
+  fi
+  if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_search_gnutls_init=$ac_res
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext
+  if ${ac_cv_search_gnutls_init+:} false; then :
+  break
+fi
+done
+if ${ac_cv_search_gnutls_init+:} false; then :
+
+else
+  ac_cv_search_gnutls_init=no
+fi
+rm conftest.$ac_ext
+LIBS=$ac_func_search_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_gnutls_init" >&5
+$as_echo "$ac_cv_search_gnutls_init" >&6; }
+ac_res=$ac_cv_search_gnutls_init
+if test "$ac_res" != no; then :
+  test "$ac_res" = "none required" || LIBS="$ac_res $LIBS"
+
+else
+  as_fn_error $? "library 'gnutls' is required for GnuTLS" "$LINENO" 5
+fi
+
+  # GnuTLS versions before 3.4.0 do not support sorting incorrectly sorted
+  # certificate chains.
+  ac_fn_c_check_decl "$LINENO" "GNUTLS_X509_CRT_LIST_SORT" "ac_cv_have_decl_GNUTLS_X509_CRT_LIST_SORT" "#include 
+#include 
+
+"
+if test "x$ac_cv_have_decl_GNUTLS_X509_CRT_LIST_SORT" = xyes; then :
+  ac_have_decl=1
+else
+  ac_have_decl=0
+fi
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_DECL_GNUTLS_X509_CRT_LIST_SORT $ac_have_decl
+_ACEOF
+
+fi
+
 if test "$with_pam" = yes ; then
   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for pam_start in -lpam" >&5
 $as_echo_n "checking for pam_start in -lpam... " >&6; }
@@ -11015,6 +11130,17 @@ else
 fi
 
 
+fi
+
+if test "$with_gnutls" = yes ; then
+  ac_fn_c_check_header_mongrel "$LINENO" "gnutls/gnutls.h" "ac_cv_header_gnutls_gnutls_h" "$ac_includes_default"
+if test "x$ac_cv_header_gnutls_gnutls_h" = xyes; then :
+
+else
+  as_fn_error $? "header file  is required for GnuTLS" "$LINENO" 5
+fi
+
+
 fi
 
 if test "$with_pam" = yes ; then
@@ -15522,9 +15648,11 @@ fi
 # in the template or configure command line.
 
 # If not selected manually, try to select a source automatically.
-if test "$enable_strong_random" = "yes" && test x"$USE_OPENSSL_RANDOM" = x"" && test x"$USE_WIN32_RANDOM" = x"" && test x"$USE_DEV_URANDOM" = x"" ; then
+if test "$enable_strong_random" = "yes" && test 

Re: [HACKERS] GnuTLS support

[Jeff Janes]

On Thu, Aug 31, 2017 at 10:52 AM, Andreas Karlsson 
wrote:

>
>
>
> = Work left to do
>
> - Test code with older versions of GnuTLS
>


I can't build against gnutls-2.12.23-21.el6.x86_64 from CentOS 6.9

be-secure-gnutls.c: In function 'be_tls_init':
be-secure-gnutls.c:168: warning: implicit declaration of function
'gnutls_certificate_set_pin_function'
be-secure-gnutls.c: In function 'get_peer_certificate':
be-secure-gnutls.c:656: error: 'GNUTLS_X509_CRT_LIST_SORT' undeclared
(first use in this function)
be-secure-gnutls.c:656: error: (Each undeclared identifier is reported only
once
be-secure-gnutls.c:656: error: for each function it appears in.)

But I can build on ubuntu 16.04, using whatever baffling salami of package
versions they turned gnutls into on that system.

I can interoperate in both direction gnutls client to openssl server and
the reverse (and gnutls to gnutls).

Cheers,

Jeff

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/17 17:27, Etsuro Fujita wrote:

On 2017/07/11 6:56, Robert Haas wrote:

I have to admit that I'm a little bit fuzzy about why foreign insert
routing requires all of these changes.  I think this patch would
benefit from being accompanied by several paragraphs of explanation
outlining the rationale for each part of the patch.


Will do.


Here is an updated version of the patch.

* Query planning: the patch creates copies of Query/Plan with a foreign 
partition as target from the original Query/Plan for each foreign 
partition and invokes PlanForeignModify with those copies, to allow the 
FDW to do query planning for remote INSERT with the existing API.  To 
make such Queries the similar way inheritance_planner does, I modified 
transformInsertStmt so that the inh flag for the partitioned table's RTE 
is set to true, which allows (1) expand_inherited_rtentry to build an 
RTE and AppendRelInfo for each partition in the partitioned table and 
(2) make_modifytable to build such Queries using adjust_appendrel_attrs 
and those AppendRelInfos.


* explain.c: I modified show_modifytable_info so that we can show remote 
queries for foreign partitions in EXPLAIN for INSERT into a partitioned 
table the same way as for inherited UPDATE/DELETE cases.  Here is an 
example:


postgres=# explain verbose insert into pt values (1), (2);
QUERY PLAN
---
 Insert on public.pt  (cost=0.00..0.03 rows=2 width=4)
   Foreign Insert on public.fp1
 Remote SQL: INSERT INTO public.t1(a) VALUES ($1)
   Foreign Insert on public.fp2
 Remote SQL: INSERT INTO public.t2(a) VALUES ($1)
   ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)
 Output: "*VALUES*".column1
(7 rows)

I think I should add more explanation about the patch, but I don't have 
time today, so I'll write additional explanation in the next email. 
Sorry about that.


Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 315,321  SELECT tableoid::regclass, * FROM p2;
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter 
','); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
--- 315,321 
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter 
','); -- ERROR
! ERROR:  cannot route copied tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
***
*** 342,348  SELECT tableoid::regclass, * FROM p2;
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
--- 342,348 
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 7029,7034  NOTICE:  drop cascades to foreign table bar2
--- 7029,7172 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing to foreign-table partitions
+ -- ===
+ create table pt (a int, b int) partition by list (a);
+ create table locp partition of pt for values in (1);
+ create table locfoo (a int check (a in (2)), b int);
+ create foreign table remp partition of pt for values in (2) server loopback 
options (table_name 'locfoo');
+ explain (verbose, costs off)
+ insert into pt values (1, 1), (2, 1);
+QUERY PLAN
+ -
+  Insert on public.pt
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2)
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (6 rows)
+ 
+ insert into pt values (1, 1), (2, 1);
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+  remp | 2 | 1
+ (2 rows)
+ 
+ select tableoid::regclass, * FROM locp;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+ (1 row)
+ 
+ select tableoid::regclass, * FROM remp;
+  tableoid | a | b 
+ --+---+---
+  remp | 2 | 1
+ (1 row)
+ 
+ explain (verbose, costs off)
+ insert into pt values (1, 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Sat, Sep 9, 2017 at 3:05 AM, Robert Haas  wrote:

> On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
>  wrote:
> > Thanks Robert for taking care of this.
> > My V29 patch series[1] is based on this commit now.
>
> Committed 0001-0003, 0005 with assorted modifications, mostly
> cosmetic, but with some actual changes to describeOneTableDetails and
> ATExecAttachPartition and minor additions to the regression tests.
>
>
Thanks Robert!!

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
 wrote:
> Thanks Robert for taking care of this.
> My V29 patch series[1] is based on this commit now.

Committed 0001-0003, 0005 with assorted modifications, mostly
cosmetic, but with some actual changes to describeOneTableDetails and
ATExecAttachPartition and minor additions to the regression tests.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Sep 8, 2017 at 6:46 AM, Robert Haas  wrote:

> On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
>  wrote:
> > The fix would be much easier if the refactoring patch 0001 by Amul in
> hash
> > partitioning thread[2] is committed.
>
> Done.
>

Thanks Robert for taking care of this.
My V29 patch series[1] is based on this commit now.

[1]
https://www.postgresql.org/message-id/CAOgcT0PCM5mJPCOyj3c0D1mLxwaVz6DJLO=nmz5j-5jgywx...@mail.gmail.com

Regards,
Jeevan Ladhe

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Thu, Sep 7, 2017 at 10:35 PM, Tom Lane  wrote:
> I think we might be best off just playing it straight and providing
> a config file that contains a section along these lines:
>
> # Parameters for OpenSSL.  Leave these commented out if not using OpenSSL.
> #
> #ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> #ssl_prefer_server_ciphers = on
> #ssl_ecdh_curve = 'prime256v1'
> #ssl_dh_params_file = ''
> #ssl_cert_file = 'server.crt'
> #ssl_key_file = 'server.key'
> #ssl_ca_file = ''
> #ssl_crl_file = ''
> #
> # Parameters for GnuTLS.  Leave these commented out if not using GnuTLS.
> #
> #gnufoo=...
> #gnubar=...
> #
> # Parameters for macOS TLS.  ... you get the idea.
>
> As previously noted, it'd be a good idea to rename the existing
> ssl_xxx parameters to openssl_xxx, except maybe ones that we think
> will be universal.  (But even if we do think that, it might be
> simpler in the long run to just have three or four totally independent
> sections of the config file, instead of some common and some library-
> specific parameters.)

+1 to all of that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] GnuTLS support

[Tom Lane]

Andreas Karlsson  writes:
> On 09/07/2017 11:34 PM, Tomas Vondra wrote:
>> Well, people won't be able to set the inactive options, just like you
>> can't set ssl=on when you build without OpenSSL support. But perhaps we
>> could simply not include the inactive options into the config file, no?

> Yeah, I have been thinking about how bad it would be to dynamically 
> generate the config file. I think I will try this.

I'm not exactly convinced that dynamically inserting some parameters
and not others is a great idea.  Remember that people tend to copy
postgresql.conf files forward from one installation to another.  Or
they might decide to rebuild the postmaster for an existing installation
with a different SSL library.  In any scenario like that, you've not
done them any real favor if the config file they have contains no trace
of the SSL parameters they need.

I think we might be best off just playing it straight and providing
a config file that contains a section along these lines:

# Parameters for OpenSSL.  Leave these commented out if not using OpenSSL.
#
#ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
#ssl_prefer_server_ciphers = on
#ssl_ecdh_curve = 'prime256v1'
#ssl_dh_params_file = ''
#ssl_cert_file = 'server.crt'
#ssl_key_file = 'server.key'
#ssl_ca_file = ''
#ssl_crl_file = ''
#
# Parameters for GnuTLS.  Leave these commented out if not using GnuTLS.
#
#gnufoo=...
#gnubar=...
#
# Parameters for macOS TLS.  ... you get the idea.

As previously noted, it'd be a good idea to rename the existing
ssl_xxx parameters to openssl_xxx, except maybe ones that we think
will be universal.  (But even if we do think that, it might be
simpler in the long run to just have three or four totally independent
sections of the config file, instead of some common and some library-
specific parameters.)

regards, tom lane


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

Re: [HACKERS] GnuTLS support

[Andreas Karlsson]

On 09/07/2017 11:34 PM, Tomas Vondra wrote:

I am worried about having 3x version of TLS controls in
postgresql.conf, and only one set being active. Perhaps we need to
break out the TLS config to separate files or something. Anyway, this
needs more thought.


Well, people won't be able to set the inactive options, just like you
can't set ssl=on when you build without OpenSSL support. But perhaps we
could simply not include the inactive options into the config file, no?


Yeah, I have been thinking about how bad it would be to dynamically 
generate the config file. I think I will try this.


Daniel: What options does Secure Transport need for configuring ciphers, 
ECDH, and cipher preference? Does it need any extra options (I think I 
saw something about the keychain)?


Andreas


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
 wrote:
> The fix would be much easier if the refactoring patch 0001 by Amul in hash
> partitioning thread[2] is committed.

Done.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] GnuTLS support

[Magnus Hagander]

On Thu, Sep 7, 2017 at 2:34 PM, Tomas Vondra 
wrote:

> Hi,
>
> On 09/04/2017 04:24 PM, Bruce Momjian wrote:
> > On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
> >> I think that what this shows is that the current set of GUCs is overly
> >> OpenSSL-centric.  We created a set of GUCs that are actually specific
> >> to one particular implementation but named them as if they were
> >> generic.  My idea about this would be to actually rename the existing
> >> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> >> as needed for other SSL implementations.
> >>
> >> Depending on what we think is best, GUCs for an SSL implementation
> >> other than the one against which we compiled can either not exist at
> >> all, or can exist but be limited to a single value (e.g. "none", as we
> >> currently do when the compile has no SSL support at all).  Also, we
> >> could add a read-only GUC with a name like ssl_library that exposes
> >> the name of the underlying SSL implementation - none, openssl, gnutls,
> >> or whatever.
> >>
> >> I think if we go the route of insisting that every SSL implementation
> >> has to use the existing GUCs, we're just trying to shove a square peg
> >> into a round hole, and there's no real benefit for users.  If the
> >> string that has to be stuffed into ssl_ciphers differs based on which
> >> library was chosen at compile time, then you can't have a uniform
> >> default configuration for all libraries anyway.  I think it'll be
> >> easier to explain and document this if there's separate documentation
> >> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
> >> documentation section that tries to explain every implementation
> >> separately.
> >
> > I am worried about having 3x version of TLS controls in
> > postgresql.conf, and only one set being active. Perhaps we need to
> > break out the TLS config to separate files or something. Anyway, this
> > needs more thought.
> >
>
> Well, people won't be able to set the inactive options, just like you
> can't set ssl=on when you build without OpenSSL support. But perhaps we
> could simply not include the inactive options into the config file, no?
>

We build the pg_hba.conf file dynamically depending on if we have ipv6
support, IIRC. Maybe we need to implement that type of support into
postgresql.conf as well?

It will still be a mess though -- documentation, and tutorials around and
whatnot, will be dependent on library. But I'm not sure we can really get
around that.

Do we have some examples of how other products that support multiple
libraries do to handle this?


>
> I don't see how breaking the TLS config into separate files improves the
> situation - instead of dealing with GUCs in a single file you'll be
> dealing with the same GUCs in multiple files. No?
>

+1. I don't think splitting them up into different files makes it in any
way better -- if anything, it makes it worse.


-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: [HACKERS] GnuTLS support

[Tomas Vondra]

Hi,

On 09/04/2017 04:24 PM, Bruce Momjian wrote:
> On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
>> I think that what this shows is that the current set of GUCs is overly
>> OpenSSL-centric.  We created a set of GUCs that are actually specific
>> to one particular implementation but named them as if they were
>> generic.  My idea about this would be to actually rename the existing
>> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
>> as needed for other SSL implementations.
>>
>> Depending on what we think is best, GUCs for an SSL implementation
>> other than the one against which we compiled can either not exist at
>> all, or can exist but be limited to a single value (e.g. "none", as we
>> currently do when the compile has no SSL support at all).  Also, we
>> could add a read-only GUC with a name like ssl_library that exposes
>> the name of the underlying SSL implementation - none, openssl, gnutls,
>> or whatever.
>>
>> I think if we go the route of insisting that every SSL implementation
>> has to use the existing GUCs, we're just trying to shove a square peg
>> into a round hole, and there's no real benefit for users.  If the
>> string that has to be stuffed into ssl_ciphers differs based on which
>> library was chosen at compile time, then you can't have a uniform
>> default configuration for all libraries anyway.  I think it'll be
>> easier to explain and document this if there's separate documentation
>> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
>> documentation section that tries to explain every implementation
>> separately.
> 
> I am worried about having 3x version of TLS controls in
> postgresql.conf, and only one set being active. Perhaps we need to
> break out the TLS config to separate files or something. Anyway, this
> needs more thought.
> 

Well, people won't be able to set the inactive options, just like you
can't set ssl=on when you build without OpenSSL support. But perhaps we
could simply not include the inactive options into the config file, no?

I don't see how breaking the TLS config into separate files improves the
situation - instead of dealing with GUCs in a single file you'll be
dealing with the same GUCs in multiple files. No?

regards

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Sep 6, 2017 at 5:50 PM, Jeevan Ladhe
 wrote:
>
>>
>> I am wondering whether we could avoid call to get_default_partition_oid()
>> in
>> the above block, thus avoiding a sys cache lookup. The sys cache search
>> shouldn't be expensive since the cache should already have that entry, but
>> still if we can avoid it, we save some CPU cycles. The default partition
>> OID is
>> stored in pg_partition_table catalog, which is looked up in
>> RelationGetPartitionKey(), a function which precedes
>> RelationGetPartitionDesc()
>> everywhere. What if that RelationGetPartitionKey() also returns the
>> default
>> partition OID and the common caller passes it to
>> RelationGetPartitionDesc()?.
>
>
> The purpose here is to cross check the relid with partdefid stored in
> catalog
> pg_partitioned_table, though its going to be the same in the parents cache,
> I
> think its better that we retrieve it from the catalog syscache.
> Further, RelationGetPartitionKey() is a macro and not a function, so
> modifying
> the existing simple macro for this reason does not sound a good idea to me.
> Having said this I am open to ideas here.

Sorry, I meant RelationBuildPartitionKey() and
RelationBuildPartitionDesc() instead of RelationGetPartitionKey() and
RelationGetPartitionDesc() resp.

>
>>
>> +/* A partition cannot be attached if there exists a default partition
>> */
>> +defaultPartOid = get_default_partition_oid(RelationGetRelid(rel));
>> +if (OidIsValid(defaultPartOid))
>> +ereport(ERROR,
>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("cannot attach a new partition to table
>> \"%s\" having a default partition",
>> +RelationGetRelationName(rel;
>> get_default_partition_oid() searches the catalogs, which is not needed
>> when we
>> have relation descriptor of the partitioned table (to which a new
>> partition is
>> being attached). You should get the default partition OID from partition
>> descriptor. That will be cheaper.
>
>
> Something like following can be done here:
> /* A partition cannot be attached if there exists a default partition */
> if (partition_bound_has_default(rel->partdesc->boundinfo))
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>  errmsg("cannot attach a new partition to table \"%s\"
> having a default partition",
> RelationGetRelationName(rel;
>
> But, partition_bound_has_default() is defined in partition.c and not in
> partition.h. This is done that way because boundinfo is not available in
> partition.h. Further, this piece of code is removed in next patch where we
> extend default partition support to add/attach partition even when default
> partition exists. So, to me I don’t see much of the correction issue here.

If the code is being removed, I don't think we should sweat too much
about it. Sorry for the noise.

>
> Another way to get around this is, we can define another version of
> get_default_partition_oid() something like
> get_default_partition_oid_from_parent_rel()
> in partition.c which looks around in relcache instead of catalog and returns
> the
> oid of default partition, or get_default_partition_oid() accepts both parent
> OID,
> and parent ‘Relation’ rel, if rel is not null look into relcahce and return,
> else search from catalog using OID.

I think we should define a function to return default partition OID
from partition descriptor and make it extern. Define a wrapper which
accepts Relation and returns calls this function to get default
partition OID from partition descriptor. The wrapper will be called
only on an open Relation, wherever it's available.


>
>> I haven't gone through the full patch yet, so there may be more
>> comments here. There is some duplication of code in
>> check_default_allows_bound() and ValidatePartitionConstraints() to
>> scan the children of partition being validated. The difference is that
>> the first one scans the relations in the same function and the second
>> adds them to work queue. May be we could use
>> ValidatePartitionConstraints() to scan the relation or add to the
>> queue based on some input flag may be wqueue argument itself. But I
>> haven't thought through this completely. Any thoughts?
>
>
> check_default_allows_bound() is called only from DefineRelation(),
> and not for alter command. I am not really sure how can we use
> work queue for create command.


No, we shouldn't use work queue for CREATE command. We should extract
the common code into a function to be called from
check_default_allows_bound() and ValidatePartitionConstraints(). To
that function we pass a flag (or the work queue argument itself),
which decides whether to add a work queue item or scan the relation
directly.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Thu, Sep 7, 2017 at 3:15 PM, Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> On Wed, Sep 6, 2017 at 5:25 PM, Jeevan Ladhe <
> jeevan.la...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> Attached is the rebased set of patches.
>> Robert has committed[1] patch 0001 in V26 series, hence the patch
>> numbering
>> in V27 is now decreased by 1 for each patch as compared to V26.
>>
>
> Hi,
>
> I have applied v27 patches and while testing got below observation.
>
> Observation: in below partition table, d1 constraints not allowing NULL to
> be inserted in b column
> but I am able to insert it.
>
> steps to reproduce:
> create table d0 (a int, b int) partition by range(a,b);
> create table d1 partition of d0 for values from (0,0) to
> (maxvalue,maxvalue);
>
> postgres=# insert into d0 values (0,null);
> INSERT 0 1
> postgres=# \d+ d1
> Table "public.d1"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> +-+---+--+-+
> -+--+-
>  a  | integer |   |  | | plain
> |  |
>  b  | integer |   |  | | plain
> |  |
> Partition of: d0 FOR VALUES FROM (0, 0) TO (MAXVALUE, MAXVALUE)
> Partition constraint: ((a IS NOT NULL) AND *(b IS NOT NULL) *AND ((a > 0)
> OR ((a = 0) AND (b >= 0
>
> postgres=# select tableoid::regclass,* from d0;
>  tableoid | a | b
> --+---+---
>  *d1   | 0 |  *
> (1 row)
>

Good catch. Thanks Rajkumar.
This seems to be happening because of the following changes made in
get_partition_for_tuple() for default range partition support as part of
patch 0002.

@@ -1971,27 +2204,10 @@ get_partition_for_tuple(PartitionDispatch *pd,
  ecxt->ecxt_scantuple = slot;
  FormPartitionKeyDatum(parent, slot, estate, values, isnull);

- if (key->strategy == PARTITION_STRATEGY_RANGE)
- {
- /*
- * Since we cannot route tuples with NULL partition keys through a
- * range-partitioned table, simply return that no partition exists
- */
- for (i = 0; i < key->partnatts; i++)
- {
- if (isnull[i])
- {
- *failed_at = parent;
- *failed_slot = slot;
- result = -1;
- goto error_exit;
- }
- }
- }

Instead of getting rid of this. If there isn't a default partition then
we still do not have any range partition to route a null partition
key and the routing should fail.

I will work on a fix and send a patch shortly.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Rajkumar Raghuwanshi]

On Wed, Sep 6, 2017 at 5:25 PM, Jeevan Ladhe 
wrote:

> Hi,
>
> Attached is the rebased set of patches.
> Robert has committed[1] patch 0001 in V26 series, hence the patch numbering
> in V27 is now decreased by 1 for each patch as compared to V26.
>

Hi,

I have applied v27 patches and while testing got below observation.

Observation: in below partition table, d1 constraints not allowing NULL to
be inserted in b column
but I am able to insert it.

steps to reproduce:
create table d0 (a int, b int) partition by range(a,b);
create table d1 partition of d0 for values from (0,0) to
(maxvalue,maxvalue);

postgres=# insert into d0 values (0,null);
INSERT 0 1
postgres=# \d+ d1
Table "public.d1"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
|
 b  | integer |   |  | | plain   |
|
Partition of: d0 FOR VALUES FROM (0, 0) TO (MAXVALUE, MAXVALUE)
Partition constraint: ((a IS NOT NULL) AND *(b IS NOT NULL) *AND ((a > 0)
OR ((a = 0) AND (b >= 0

postgres=# select tableoid::regclass,* from d0;
 tableoid | a | b
--+---+---
 *d1   | 0 |  *
(1 row)


Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I have tried to address your comments in V27 patch series[1].
Please find my comments inlined.


> >>
> >> The current set of patches contains 6 patches as below:
> >>
> >> 0001:
> >> Refactoring existing ATExecAttachPartition  code so that it can be used
> >> for
> >> default partitioning as well
>

If I understand correctly these comments refer to patch 0001 of V25_rebase
series, which is related to "Fix assumptions that get_qual_from_partbound()"
and not refactoring. This is patch 0001 in V27 now.

  * Returns an expression tree describing the passed-in relation's partition
> - * constraint.
> + * constraint. If there are no partition constraints returns NULL e.g. in
> case
> + * default partition is the only partition.
> The first sentence uses singular constraint. The second uses plural. Given
> that
> partition bounds together form a single constraint we should use singular
> constraint in the second sentence as well.
>

I have changed the wording now.


>
> Do we want to add a similar comment in the prologue of
> generate_partition_qual(). The current wording there seems to cover this
> case,
> but do we want to explicitly mention this case?
>

I have added a comment there.


>
> +if (!and_args)
> +result = NULL;
> While this is correct, I am increasingly seeing (and_args != NIL) usage.
>

Changed this to:
+   if (and_args == NIL)
+   result = NULL;


> get_partition_qual_relid() is called from pg_get_partition_
> constraintdef(),
> constr_expr = get_partition_qual_relid(relationId);
>
> /* Quick exit if not a partition */
> if (constr_expr == NULL)
> PG_RETURN_NULL();
> The comment is now wrong since a default partition may have no
> constraints. May
> be rewrite it as simply, "Quick exit if no partition constraint."
>

Fixed.


>
> generate_partition_qual() has three callers and all of them are capable of
> handling NIL partition constraint for default partition. May be it's
> better to
> mention in the commit message that we have checked that the callers of
> this function
> can handle NIL partition constraint.
>

Added in commit message.


> >>
> >> 0002:
> >> This patch teaches the partitioning code to handle the NIL returned by
> >> get_qual_for_list().
> >> This is needed because a default partition will not have any constraints
> >> in case
> >> it is the only partition of its parent.
>

Comments below refer to patch 0002 in V25_rebase(0003 in V25), which
adds basic support for default partition, which is now 0002 in V27.


> If the partition being dropped is the default partition,
> heap_drop_with_catalog() locks default partition twice, once as the default
> partition and the second time as the partition being dropped. So, it will
> be
> counted as locked twice. There doesn't seem to be any harm in this, since
> the
> lock will be help till the transaction ends, by when all the locks will be
> released.
>

 Fixed.


> Same is the case with cache invalidation message. If we are dropping
> default
> partition, the cache invalidation message on "default partition" is not
> required. Again this might be harmless, but better to avoid it.


Fixed.


> Similar problems exists with ATExecDetachPartition(), default partition
> will be
> locked twice if it's being detached.
>

In ATExecDetachPartition() we do not have OID of the relation being
detached
available at the time we lock default partition. Moreover, here we are
taking an
exclusive lock on default OID and an share lock on partition being detached.
As you correctly said in your earlier comment that it will be counted as
locked
twice, which to me also seems harmless. As these locks are to be held till
commit of the transaction nobody else is supposed to be releasing these
locks in
between. I am not able to visualize a problem here, but still I have tried
to
avoid locking the default partition table twice, please review the changes
and
let me know your thoughts.


> +/*
> + * If this is a default partition, pg_partitioned_table must have
> it's
> + * OID as value of 'partdefid' for it's parent (i.e. rel) entry.
> + */
> +if (castNode(PartitionBoundSpec, boundspec)->is_default)
> +{
> +Oidpartdefid;
> +
> +partdefid = get_default_partition_oid(RelationGetRelid(rel));
> +Assert(partdefid == inhrelid);
> +}
> Since an accidental change or database corruption may change the default
> partition OID in pg_partition_table. An Assert won't help in such a case.
> May
> be we should throw an error or at least report a warning. If we throw an
> error,
> the table will become useless (or even the database will become useless
> RelationBuildPartitionDesc is called from RelationCacheInitializePhase3()
> on
> such a corrupted table). To avoid that we may raise a warning.
>
> I have added a warning here instead of Assert.


> I am wondering whether we could avoid call to 

Re: [HACKERS] GnuTLS support

[David Fetter]

On Fri, Sep 01, 2017 at 10:33:37PM +0200, Alvaro Herrera wrote:
> Tom Lane wrote:
> > Robert Haas  writes:
> > > On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  
> > > wrote:
> 
> > >> There are currently two failing SSL tests which at least to me seems more
> > >> like they test specific OpenSSL behaviors rather than something which 
> > >> need
> > >> to be true for all SSL libraries.
> > 
> > > I don't know what we should do about these issues.
> > 
> > Maybe the SSL test suite needs to be implementation-specific as well.
> 
> If only two tests fail currently, I suggest that the thing to do is to
> split it up in generic vs. library-specific test files.  It should be
> easy to do with the TAP framework -- just move the library-specific
> tests to their own file and mark it as skipped at the start of the file
> when a different library is detected.

This seems like a much smarter and more reliable way to test.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] GnuTLS support

[Bruce Momjian]

On Fri, Sep  1, 2017 at 12:09:35PM -0400, Robert Haas wrote:
> I think that what this shows is that the current set of GUCs is overly
> OpenSSL-centric.  We created a set of GUCs that are actually specific
> to one particular implementation but named them as if they were
> generic.  My idea about this would be to actually rename the existing
> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> as needed for other SSL implementations.
> 
> Depending on what we think is best, GUCs for an SSL implementation
> other than the one against which we compiled can either not exist at
> all, or can exist but be limited to a single value (e.g. "none", as we
> currently do when the compile has no SSL support at all).  Also, we
> could add a read-only GUC with a name like ssl_library that exposes
> the name of the underlying SSL implementation - none, openssl, gnutls,
> or whatever.
> 
> I think if we go the route of insisting that every SSL implementation
> has to use the existing GUCs, we're just trying to shove a square peg
> into a round hole, and there's no real benefit for users.  If the
> string that has to be stuffed into ssl_ciphers differs based on which
> library was chosen at compile time, then you can't have a uniform
> default configuration for all libraries anyway.  I think it'll be
> easier to explain and document this if there's separate documentation
> for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
> documentation section that tries to explain every implementation
> separately.

I am worried about having 3x version of TLS controls in postgresql.conf,
and only one set being active.  Perhaps we need to break out the TLS
config to separate files or something.  Anyway, this needs more thought.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Sat, Sep 2, 2017 at 7:03 AM, Robert Haas  wrote:

> On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas  wrote:
> > On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
> >  wrote:
> >> 0001:
> >> This patch refactors RelationBuildPartitionDesc(), basically this is
> patch
> >> 0001 of default range partition[1].
> >
> > I spent a while studying this; it seems to be simpler and there's no
> > real downside.  So, committed.
>
>
Thanks Robert for taking care of this.


> BTW, the rest of this series seems to need a rebase.  The changes to
> insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3.


Will rebase the patches.

Regards,
Jeevan Ladhe

Re: [HACKERS] GnuTLS support

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:

> >> There are currently two failing SSL tests which at least to me seems more
> >> like they test specific OpenSSL behaviors rather than something which need
> >> to be true for all SSL libraries.
> 
> > I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

If only two tests fail currently, I suggest that the thing to do is to
split it up in generic vs. library-specific test files.  It should be
easy to do with the TAP framework -- just move the library-specific
tests to their own file and mark it as skipped at the start of the file
when a different library is detected.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas  wrote:
> On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
>  wrote:
>> 0001:
>> This patch refactors RelationBuildPartitionDesc(), basically this is patch
>> 0001 of default range partition[1].
>
> I spent a while studying this; it seems to be simpler and there's no
> real downside.  So, committed.

BTW, the rest of this series seems to need a rebase.  The changes to
insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
 wrote:
> 0001:
> This patch refactors RelationBuildPartitionDesc(), basically this is patch
> 0001 of default range partition[1].

I spent a while studying this; it seems to be simpler and there's no
real downside.  So, committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] GnuTLS support

[Daniel Gustafsson]

> On 01 Sep 2017, at 20:00, Robert Haas  wrote:
> 
> On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
 I have seen discussions from time to time about OpenSSL and its licensing
 issues so I decided to see how much work it would be to add support for
 another TLS library, and  I went with GnuTLS since it is the library I know
 best after OpenSSL and it is also a reasonably popular library.
>> 
>>> Thanks for working on this.  I think it's good for PostgreSQL to have
>>> more options in this area.
>> 
>> +1.  We also have a patch in the queue to support macOS' TLS library,
>> and I suppose that's going to be facing similar issues.  It would be
>> a good plan, probably, to try to push both of these to conclusion in
>> the same development cycle.
> 
> The thing which I think would save the most aggravation - at least for
> my employer - is a Windows SSL implementation.

In 53ea546e.6020...@vmware.com, an early version of SChannel support was posted
by Heikki.  If anyone is keen to pick up the effort that would most likely be a
good starting point.

> Relying on OpenSSL
> means that every time OpenSSL puts out a critical security fix, we've
> got to rewrap all the Windows installers to pick up the new version.
> If we were relying on what's built into Windows, it would be
> Microsoft's problem.  Granted, it's not anybody's job to solve
> EnterpriseDB's problems except EnterpriseDB, but users might like it
> too -- and anyone else who is building Windows installers for
> PostgreSQL.
> 
> Depending on macOS TLS instead of OpenSSL has similar advantages, of
> course, just for a somewhat less common platform.

I think providing alternatives to OpenSSL on platforms where OpenSSL can’t be
relied on to be already available (Windows and macOS come to mind) would be a
great thing for many users and app developers.

cheers ./daniel

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

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Fri, Sep 1, 2017 at 1:10 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
>>> I have seen discussions from time to time about OpenSSL and its licensing
>>> issues so I decided to see how much work it would be to add support for
>>> another TLS library, and  I went with GnuTLS since it is the library I know
>>> best after OpenSSL and it is also a reasonably popular library.
>
>> Thanks for working on this.  I think it's good for PostgreSQL to have
>> more options in this area.
>
> +1.  We also have a patch in the queue to support macOS' TLS library,
> and I suppose that's going to be facing similar issues.  It would be
> a good plan, probably, to try to push both of these to conclusion in
> the same development cycle.

The thing which I think would save the most aggravation - at least for
my employer - is a Windows SSL implementation.  Relying on OpenSSL
means that every time OpenSSL puts out a critical security fix, we've
got to rewrap all the Windows installers to pick up the new version.
If we were relying on what's built into Windows, it would be
Microsoft's problem.  Granted, it's not anybody's job to solve
EnterpriseDB's problems except EnterpriseDB, but users might like it
too -- and anyone else who is building Windows installers for
PostgreSQL.

Depending on macOS TLS instead of OpenSSL has similar advantages, of
course, just for a somewhat less common platform.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] GnuTLS support

[Daniel Gustafsson]

> On 01 Sep 2017, at 19:10, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
> 
>>> There are currently two failing SSL tests which at least to me seems more
>>> like they test specific OpenSSL behaviors rather than something which need
>>> to be true for all SSL libraries.
> 
>> I don't know what we should do about these issues.
> 
> Maybe the SSL test suite needs to be implementation-specific as well.

To properly test the macOS Secure Transport support we will need to use
Keychain files on top of plain PEM files, so I think we have to.  That being
said, we should probably define a (as large possible) minimum set which applies
to all to ensure compatability between different frontends and backends.

cheers ./daniel

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

Re: [HACKERS] GnuTLS support

[Tom Lane]

Robert Haas  writes:
> On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
>> I have seen discussions from time to time about OpenSSL and its licensing
>> issues so I decided to see how much work it would be to add support for
>> another TLS library, and  I went with GnuTLS since it is the library I know
>> best after OpenSSL and it is also a reasonably popular library.

> Thanks for working on this.  I think it's good for PostgreSQL to have
> more options in this area.

+1.  We also have a patch in the queue to support macOS' TLS library,
and I suppose that's going to be facing similar issues.  It would be
a good plan, probably, to try to push both of these to conclusion in
the same development cycle.

> I think that what this shows is that the current set of GUCs is overly
> OpenSSL-centric.  We created a set of GUCs that are actually specific
> to one particular implementation but named them as if they were
> generic.  My idea about this would be to actually rename the existing
> GUCs to start with "openssl" rather than "ssl", and then add new GUCs
> as needed for other SSL implementations.

Works for me.

>> There are currently two failing SSL tests which at least to me seems more
>> like they test specific OpenSSL behaviors rather than something which need
>> to be true for all SSL libraries.

> I don't know what we should do about these issues.

Maybe the SSL test suite needs to be implementation-specific as well.

regards, tom lane


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

Re: [HACKERS] GnuTLS support

[Robert Haas]

On Thu, Aug 31, 2017 at 1:52 PM, Andreas Karlsson  wrote:
> I have seen discussions from time to time about OpenSSL and its licensing
> issues so I decided to see how much work it would be to add support for
> another TLS library, and  I went with GnuTLS since it is the library I know
> best after OpenSSL and it is also a reasonably popular library.

Thanks for working on this.  I think it's good for PostgreSQL to have
more options in this area.

> = Design questions
>
> == GnuTLS priority strings vs OpenSSL cipher lists
>
> GnuTLS uses a different format for specifying ciphers. Instead of setting
> ciphers, protocol versions, and ECDH curves separately GnuTLS instead uses a
> single priority string[1].
>
> For example the default settings of PostgreSQL (which include disabling
> SSLv3)
>
> ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> ssl_prefer_server_ciphers = on
> ssl_ecdh_curve = 'prime256v1'
>
> is represented with a string similar to
>
> SECURE128:+3DES-CBC:+GROUP-SECP256R1:%SERVER_PRECEDENCE
>
> So the two libraries have decided on different ways to specify things.

I think that what this shows is that the current set of GUCs is overly
OpenSSL-centric.  We created a set of GUCs that are actually specific
to one particular implementation but named them as if they were
generic.  My idea about this would be to actually rename the existing
GUCs to start with "openssl" rather than "ssl", and then add new GUCs
as needed for other SSL implementations.

Depending on what we think is best, GUCs for an SSL implementation
other than the one against which we compiled can either not exist at
all, or can exist but be limited to a single value (e.g. "none", as we
currently do when the compile has no SSL support at all).  Also, we
could add a read-only GUC with a name like ssl_library that exposes
the name of the underlying SSL implementation - none, openssl, gnutls,
or whatever.

I think if we go the route of insisting that every SSL implementation
has to use the existing GUCs, we're just trying to shove a square peg
into a round hole, and there's no real benefit for users.  If the
string that has to be stuffed into ssl_ciphers differs based on which
library was chosen at compile time, then you can't have a uniform
default configuration for all libraries anyway.  I think it'll be
easier to explain and document this if there's separate documentation
for openssl_ciphers, gnutls_ciphers, etc. rather than one giant
documentation section that tries to explain every implementation
separately.

> There are currently two failing SSL tests which at least to me seems more
> like they test specific OpenSSL behaviors rather than something which need
> to be true for all SSL libraries.
>
> The two tests:
>
> # Try with just the server CA's cert. This fails because the root file
> # must contain the whole chain up to the root CA.
> note "connect with server CA cert, without root CA";
> test_connect_fails("sslrootcert=ssl/server_ca.crt sslmode=verify-ca");
>
> # A CRL belonging to a different CA is not accepted, fails
> test_connect_fails(
> "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca
> sslcrl=ssl/client.crl");
>
> For the missing root CA case GnuTLS seems to be happy enough with just an
> intermediate CA and as far as I can tell this behavior is not easy to
> configure.
>
> And for the CRL belonging to a different CA case GnuTLS can be configured to
> either just store such a non-validating CRL or to ignore it, but not to
> return an error.
>
> Personally I think thee two tests should just be removed but maybe I am
> missing something.

I don't know what we should do about these issues.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/26 1:43, Robert Haas wrote:

On Sun, Aug 20, 2017 at 11:25 PM, Etsuro Fujita
 wrote:

I agree, but I wonder if we ought to make it work first using the
existing APIs and then add these new APIs as an optimization.


I'm not sure that's a good idea because that once we support INSERT
tuple-routing for foreign partitions, we would have a workaround: INSERT
INTO partitioned_table SELECT * from data_table where data_table is a
foreign table defined for an input file using file_fdw.


That's true, but I don't see how it refutes the point I was trying to raise.


My concern is: the existing APIs can really work well for any FDW to do 
COPY tuple-routing?  I know the original version of the patch showed the 
existing APIs would work well for postgres_fdw but nothing beyond.  For 
example: the original version made a dummy Query/Plan only containing a 
leaf partition as its result relation, and passed it to 
PlanForeignModify, IIRC.  That would work well for postgres_fdw because 
it doesn't look at the Query/Plan except the result relation, but might 
not for other FDWs that look at more stuff of the given Query/Plan and 
do something based on that information.  Some FDW might check to see 
whether the Plan is to do INSERT .. VALUES with a single VALUES sublist 
or INSERT .. VALUES with multiple VALUES sublists, for optimizing remote 
INSERT, for example.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Mon, Aug 21, 2017 at 4:47 PM, Jeevan Ladhe
 wrote:
>
> Hi,
>
> On Thu, Aug 17, 2017 at 3:29 PM, Jeevan Ladhe
>  wrote:
>>
>> Hi,
>>
>> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas 
>> wrote:
>>>
>>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>>  wrote:
>>> > I have rebased the patches on the latest commit.
>>>
>>> This needs another rebase.
>>
>>
>> I have rebased the patch and addressed your and Ashutosh comments on last
>> set of patches.

Thanks for the rebased patches.

>>
>> The current set of patches contains 6 patches as below:
>>
>> 0001:
>> Refactoring existing ATExecAttachPartition  code so that it can be used
>> for
>> default partitioning as well

  * Returns an expression tree describing the passed-in relation's partition
- * constraint.
+ * constraint. If there are no partition constraints returns NULL e.g. in case
+ * default partition is the only partition.
The first sentence uses singular constraint. The second uses plural. Given that
partition bounds together form a single constraint we should use singular
constraint in the second sentence as well.

Do we want to add a similar comment in the prologue of
generate_partition_qual(). The current wording there seems to cover this case,
but do we want to explicitly mention this case?

+if (!and_args)
+result = NULL;
While this is correct, I am increasingly seeing (and_args != NIL) usage.

get_partition_qual_relid() is called from pg_get_partition_constraintdef(),
constr_expr = get_partition_qual_relid(relationId);

/* Quick exit if not a partition */
if (constr_expr == NULL)
PG_RETURN_NULL();
The comment is now wrong since a default partition may have no constraints. May
be rewrite it as simply, "Quick exit if no partition constraint."

generate_partition_qual() has three callers and all of them are capable of
handling NIL partition constraint for default partition. May be it's better to
mention in the commit message that we have checked that the callers of
this function
can handle NIL partition constraint.

>>
>> 0002:
>> This patch teaches the partitioning code to handle the NIL returned by
>> get_qual_for_list().
>> This is needed because a default partition will not have any constraints
>> in case
>> it is the only partition of its parent.

If the partition being dropped is the default partition,
heap_drop_with_catalog() locks default partition twice, once as the default
partition and the second time as the partition being dropped. So, it will be
counted as locked twice. There doesn't seem to be any harm in this, since the
lock will be help till the transaction ends, by when all the locks will be
released.

Same is the case with cache invalidation message. If we are dropping default
partition, the cache invalidation message on "default partition" is not
required. Again this might be harmless, but better to avoid it.

Similar problems exists with ATExecDetachPartition(), default partition will be
locked twice if it's being detached.

+/*
+ * If this is a default partition, pg_partitioned_table must have it's
+ * OID as value of 'partdefid' for it's parent (i.e. rel) entry.
+ */
+if (castNode(PartitionBoundSpec, boundspec)->is_default)
+{
+Oidpartdefid;
+
+partdefid = get_default_partition_oid(RelationGetRelid(rel));
+Assert(partdefid == inhrelid);
+}
Since an accidental change or database corruption may change the default
partition OID in pg_partition_table. An Assert won't help in such a case. May
be we should throw an error or at least report a warning. If we throw an error,
the table will become useless (or even the database will become useless
RelationBuildPartitionDesc is called from RelationCacheInitializePhase3() on
such a corrupted table). To avoid that we may raise a warning.

I am wondering whether we could avoid call to get_default_partition_oid() in
the above block, thus avoiding a sys cache lookup. The sys cache search
shouldn't be expensive since the cache should already have that entry, but
still if we can avoid it, we save some CPU cycles. The default partition OID is
stored in pg_partition_table catalog, which is looked up in
RelationGetPartitionKey(), a function which precedes RelationGetPartitionDesc()
everywhere. What if that RelationGetPartitionKey() also returns the default
partition OID and the common caller passes it to RelationGetPartitionDesc()?.

+/* A partition cannot be attached if there exists a default partition */
+defaultPartOid = get_default_partition_oid(RelationGetRelid(rel));
+if (OidIsValid(defaultPartOid))
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot attach a new partition to table
\"%s\" having a default partition",
+

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Sun, Aug 20, 2017 at 11:25 PM, Etsuro Fujita
 wrote:
>> I agree, but I wonder if we ought to make it work first using the
>> existing APIs and then add these new APIs as an optimization.
>
> I'm not sure that's a good idea because that once we support INSERT
> tuple-routing for foreign partitions, we would have a workaround: INSERT
> INTO partitioned_table SELECT * from data_table where data_table is a
> foreign table defined for an input file using file_fdw.

That's true, but I don't see how it refutes the point I was trying to raise.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/19 2:12, Robert Haas wrote:

On Thu, Aug 17, 2017 at 4:27 AM, Etsuro Fujita
 wrote:

I think that would be much more efficient than INSERTing tuples into the
remote server one by one.  What do you think about that?


I agree, but I wonder if we ought to make it work first using the
existing APIs and then add these new APIs as an optimization.


I'm not sure that's a good idea because that once we support INSERT 
tuple-routing for foreign partitions, we would have a workaround: INSERT 
INTO partitioned_table SELECT * from data_table where data_table is a 
foreign table defined for an input file using file_fdw.



postgres_fdw isn't the only FDW in the world, and it's better if
getting a working implementation doesn't require too many interfaces.


Agreed.  My vote would be to leave the COPY part as-is until we have 
these new APIs.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/18 22:41, David Fetter wrote:

On Fri, Aug 18, 2017 at 05:10:29PM +0900, Etsuro Fujita wrote:

On 2017/08/17 23:48, David Fetter wrote:

On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote:

On 2017/07/11 6:56, Robert Haas wrote:

On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
 wrote:

So, I dropped the COPY part.


Ouch.  I think we should try to figure out how the COPY part will be
handled before we commit to a design.


I spent some time on this.  To handle that, I'd like to propose doing
something similar to \copy (frontend copy): submit a COPY query "COPY ...

>FROM STDIN" to the remote server and route data from a file to the remote

server.  For that, I'd like to add new FDW APIs called during CopyFrom that
allow us to copy to foreign tables:

* BeginForeignCopyIn: this would be called after creating a ResultRelInfo
for the target table (or each leaf partition of the target partitioned
table) if it's a foreign table, and perform any initialization needed before
the remote copy can start.  In the postgres_fdw case, I think this function
would be a good place to send "COPY ... FROM STDIN" to the remote server.

* ExecForeignCopyInOneRow: this would be called instead of heap_insert if
the target is a foreign table, and route the tuple read from the file by
NextCopyFrom to the remote server.  In the postgres_fdw case, I think this
function would convert the tuple to text format for portability, and then
send the data to the remote server using PQputCopyData.

* EndForeignCopyIn: this would be called at the bottom of CopyFrom, and
release resources such as connections to the remote server.  In the
postgres_fdw case, this function would do PQputCopyEnd to terminate data
transfer.


These primitives look good.  I know it seems unlikely at first
blush, but do we know of bulk load APIs for non-PostgreSQL data
stores that this would be unable to serve?


Maybe I'm missing something, but I think these would allow the FDW
to do the remote copy the way it would like; in
ExecForeignCopyInOneRow, for example, the FDW could buffer tuples in
a buffer memory and transmit the buffered data to the remote server
if the data size exceeds a threshold.  The naming is not so good?
Suggestions are welcome.


The naming seems reasonable.

I was trying to figure out whether this fits well enough with the bulk
load APIs for databases other than PostgreSQL.  I'm guessing it's
"well enough" based on checking MySQL, Oracle, and MS SQL Server.


Good to know.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Thu, Aug 17, 2017 at 4:27 AM, Etsuro Fujita
 wrote:
> I think that would be much more efficient than INSERTing tuples into the
> remote server one by one.  What do you think about that?

I agree, but I wonder if we ought to make it work first using the
existing APIs and then add these new APIs as an optimization.
postgres_fdw isn't the only FDW in the world, and it's better if
getting a working implementation doesn't require too many interfaces.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Thu, Aug 17, 2017 at 7:58 AM, Etsuro Fujita
 wrote:
>> The description seems to support only COPY to a foreign table from a
>> file, but probably we need the support other way round as well. This
>> looks like a feature (support copy to and from a foreign table) to be
>> handled by itself.
>
> Agreed.  I'll consider how to handle copy-from-a-foreign-table as well.

That's a completely different feature which has nothing to do with
tuple routing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[David Fetter]

On Fri, Aug 18, 2017 at 05:10:29PM +0900, Etsuro Fujita wrote:
> On 2017/08/17 23:48, David Fetter wrote:
> >On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote:
> >>On 2017/07/11 6:56, Robert Haas wrote:
> >>>On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
> >>> wrote:
> So, I dropped the COPY part.
> >>>
> >>>Ouch.  I think we should try to figure out how the COPY part will be
> >>>handled before we commit to a design.
> >>
> >>I spent some time on this.  To handle that, I'd like to propose doing
> >>something similar to \copy (frontend copy): submit a COPY query "COPY ...
> >>FROM STDIN" to the remote server and route data from a file to the remote
> >>server.  For that, I'd like to add new FDW APIs called during CopyFrom that
> >>allow us to copy to foreign tables:
> >>
> >>* BeginForeignCopyIn: this would be called after creating a ResultRelInfo
> >>for the target table (or each leaf partition of the target partitioned
> >>table) if it's a foreign table, and perform any initialization needed before
> >>the remote copy can start.  In the postgres_fdw case, I think this function
> >>would be a good place to send "COPY ... FROM STDIN" to the remote server.
> >>
> >>* ExecForeignCopyInOneRow: this would be called instead of heap_insert if
> >>the target is a foreign table, and route the tuple read from the file by
> >>NextCopyFrom to the remote server.  In the postgres_fdw case, I think this
> >>function would convert the tuple to text format for portability, and then
> >>send the data to the remote server using PQputCopyData.
> >>
> >>* EndForeignCopyIn: this would be called at the bottom of CopyFrom, and
> >>release resources such as connections to the remote server.  In the
> >>postgres_fdw case, this function would do PQputCopyEnd to terminate data
> >>transfer.
> >
> >These primitives look good.  I know it seems unlikely at first
> >blush, but do we know of bulk load APIs for non-PostgreSQL data
> >stores that this would be unable to serve?
> 
> Maybe I'm missing something, but I think these would allow the FDW
> to do the remote copy the way it would like; in
> ExecForeignCopyInOneRow, for example, the FDW could buffer tuples in
> a buffer memory and transmit the buffered data to the remote server
> if the data size exceeds a threshold.  The naming is not so good?
> Suggestions are welcome.

The naming seems reasonable.

I was trying to figure out whether this fits well enough with the bulk
load APIs for databases other than PostgreSQL.  I'm guessing it's
"well enough" based on checking MySQL, Oracle, and MS SQL Server.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/17 23:48, David Fetter wrote:

On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote:

On 2017/07/11 6:56, Robert Haas wrote:

On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
 wrote:

So, I dropped the COPY part.


Ouch.  I think we should try to figure out how the COPY part will be
handled before we commit to a design.


I spent some time on this.  To handle that, I'd like to propose doing
something similar to \copy (frontend copy): submit a COPY query "COPY ...
FROM STDIN" to the remote server and route data from a file to the remote
server.  For that, I'd like to add new FDW APIs called during CopyFrom that
allow us to copy to foreign tables:

* BeginForeignCopyIn: this would be called after creating a ResultRelInfo
for the target table (or each leaf partition of the target partitioned
table) if it's a foreign table, and perform any initialization needed before
the remote copy can start.  In the postgres_fdw case, I think this function
would be a good place to send "COPY ... FROM STDIN" to the remote server.

* ExecForeignCopyInOneRow: this would be called instead of heap_insert if
the target is a foreign table, and route the tuple read from the file by
NextCopyFrom to the remote server.  In the postgres_fdw case, I think this
function would convert the tuple to text format for portability, and then
send the data to the remote server using PQputCopyData.

* EndForeignCopyIn: this would be called at the bottom of CopyFrom, and
release resources such as connections to the remote server.  In the
postgres_fdw case, this function would do PQputCopyEnd to terminate data
transfer.


These primitives look good.  I know it seems unlikely at first blush,
but do we know of bulk load APIs for non-PostgreSQL data stores that
this would be unable to serve?


Maybe I'm missing something, but I think these would allow the FDW to do 
the remote copy the way it would like; in ExecForeignCopyInOneRow, for 
example, the FDW could buffer tuples in a buffer memory and transmit the 
buffered data to the remote server if the data size exceeds a threshold. 
 The naming is not so good?  Suggestions are welcome.


Best regards,
Etsuro Fujita



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Aug 18, 2017 at 12:25 AM, Robert Haas  wrote:

> On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
>  wrote:
> > I have addressed following comments in V25 patch[1].
>
> Committed 0001.  Since that code seems to be changing about every 10
> minutes, it seems best to get this refactoring out of the way before
> it changes again.


Thanks Robert for taking care of this.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
 wrote:
> I have addressed following comments in V25 patch[1].

Committed 0001.  Since that code seems to be changing about every 10
minutes, it seems best to get this refactoring out of the way before
it changes again.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[David Fetter]

On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote:
> On 2017/07/11 6:56, Robert Haas wrote:
> >On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
> > wrote:
> >>So, I dropped the COPY part.
> >
> >Ouch.  I think we should try to figure out how the COPY part will be
> >handled before we commit to a design.
> 
> I spent some time on this.  To handle that, I'd like to propose doing
> something similar to \copy (frontend copy): submit a COPY query "COPY ...
> FROM STDIN" to the remote server and route data from a file to the remote
> server.  For that, I'd like to add new FDW APIs called during CopyFrom that
> allow us to copy to foreign tables:
> 
> * BeginForeignCopyIn: this would be called after creating a ResultRelInfo
> for the target table (or each leaf partition of the target partitioned
> table) if it's a foreign table, and perform any initialization needed before
> the remote copy can start.  In the postgres_fdw case, I think this function
> would be a good place to send "COPY ... FROM STDIN" to the remote server.
> 
> * ExecForeignCopyInOneRow: this would be called instead of heap_insert if
> the target is a foreign table, and route the tuple read from the file by
> NextCopyFrom to the remote server.  In the postgres_fdw case, I think this
> function would convert the tuple to text format for portability, and then
> send the data to the remote server using PQputCopyData.
> 
> * EndForeignCopyIn: this would be called at the bottom of CopyFrom, and
> release resources such as connections to the remote server.  In the
> postgres_fdw case, this function would do PQputCopyEnd to terminate data
> transfer.

These primitives look good.  I know it seems unlikely at first blush,
but do we know of bulk load APIs for non-PostgreSQL data stores that
this would be unable to serve?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Thom Brown]

On 17 August 2017 at 10:59, Jeevan Ladhe  wrote:
> Hi,
>
> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas  wrote:
>>
>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>  wrote:
>> > I have rebased the patches on the latest commit.
>>
>> This needs another rebase.
>
>
> I have rebased the patch and addressed your and Ashutosh comments on last
> set of patches.
>
> The current set of patches contains 6 patches as below:
>
> 0001:
> Refactoring existing ATExecAttachPartition  code so that it can be used for
> default partitioning as well
>
> 0002:
> This patch teaches the partitioning code to handle the NIL returned by
> get_qual_for_list().
> This is needed because a default partition will not have any constraints in
> case
> it is the only partition of its parent.
>
> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition. This is a merge of 0003 and 0004 in
> V24 series.
>
> 0004:
> Extend default partitioning support to allow addition of new partitions
> after
> default partition is created/attached. This patch is a merge of patches
> 0005 and 0006 in V24 series to simplify the review process. The
> commit message has more details regarding what all is included.
>
> 0005:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.
>
> 0006:
> Documentation.
>
>
> PFA, and let me know in case of any comments.

Thanks.  Applies fine, and I've been exercising the patch and it is
doing everything it's supposed to do.

I am, however, curious to know why the planner can't optimise the following:

SELECT * FROM mystuff WHERE mystuff = (1::int,'JP'::text,'blue'::text);

This exhaustively checks all partitions, but if I change it to:

SELECT * FROM mystuff WHERE (id, country, content) =
(1::int,'JP'::text,'blue'::text);

It works fine.

The former filters like so: ((mystuff_default_1.*)::mystuff = ROW(1,
'JP'::text, 'blue'::text))

Shouldn't it instead do:

((mystuff_default_1.id, mystuff_default_1.country,
mystuff_default_1.content)::mystuff = ROW(1, 'JP'::text,
'blue'::text))

So it's not really to do with this patch; it's just something I
noticed while testing.

Thom


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/08/17 20:37, Ashutosh Bapat wrote:

On Thu, Aug 17, 2017 at 1:57 PM, Etsuro Fujita
 wrote:

I spent some time on this.  To handle that, I'd like to propose doing
something similar to \copy (frontend copy): submit a COPY query "COPY ...
FROM STDIN" to the remote server and route data from a file to the remote
server.  For that, I'd like to add new FDW APIs called during CopyFrom that
allow us to copy to foreign tables:


The description seems to support only COPY to a foreign table from a
file, but probably we need the support other way round as well. This
looks like a feature (support copy to and from a foreign table) to be
handled by itself.


Agreed.  I'll consider how to handle copy-from-a-foreign-table as well.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Ashutosh Bapat]

On Thu, Aug 17, 2017 at 1:57 PM, Etsuro Fujita
 wrote:
> On 2017/07/11 6:56, Robert Haas wrote:
>>
>> On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
>>  wrote:
>>>
>>> So, I dropped the COPY part.
>>
>>
>> Ouch.  I think we should try to figure out how the COPY part will be
>> handled before we commit to a design.
>
>
> I spent some time on this.  To handle that, I'd like to propose doing
> something similar to \copy (frontend copy): submit a COPY query "COPY ...
> FROM STDIN" to the remote server and route data from a file to the remote
> server.  For that, I'd like to add new FDW APIs called during CopyFrom that
> allow us to copy to foreign tables:

The description seems to support only COPY to a foreign table from a
file, but probably we need the support other way round as well. This
looks like a feature (support copy to and from a foreign table) to be
handled by itself.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

>> > 0007:
> >> > This patch introduces code to check if the scanning of default
> partition
> >> > child
> >> > can be skipped if it's constraints are proven.
> >>
> >> If I understand correctly, this is actually a completely separate
> >> feature not intrinsically related to default partitioning.
> >
> > I don't see this as a new feature, since scanning the default partition
> > will be introduced by this series of patches only, and rather than a
> > feature this can be classified as a completeness of default skip
> > validation logic. Your thoughts?
>
> Currently, when a partitioned table is attached, we check whether all
> the scans can be checked but not whether scans on some partitions can
> be attached.  So there are two separate things:
>
> 1. When we introduce default partitioning, we need scan the default
> partition either when (a) any partition is attached or (b) any
> partition is created.
>
> 2. In any situation where scans are needed (scanning the partition
> when it's attached, scanning the default partition when some other
> partition is attached, scanning the default when a new partition is
> created), we can run predicate_implied_by for each partition to see
> whether the scan of that partition can be skipped.
>
> Those two changes are independent. We could do (1) without doing (2)
> or (2) without doing (1) or we could do both.  So they are separate
> features.
>
>
In my V25 series(patch 0005) I have only addressed half of (2) above
i.e. code to check whether the scan of a partition of default partition
can be skipped when other partition is being added. Amit Langote
has submitted[1] a separate patch(0003)  to address skipping the scan
of the children of relation when it's being attached as a partition.

[1]
https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3869%40lab.ntt.co.jp

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

Please find my feedback inlined.
I have addressed following comments in V25 patch[1].


> > 0002:
> > This patch teaches the partitioning code to handle the NIL returned by
> > get_qual_for_list().
> > This is needed because a default partition will not have any constraints
> in
> > case
> > it is the only partition of its parent.
>
> Perhaps it would be better to make validatePartConstraint() a no-op
> when the constraint is empty rather than putting the logic in the
> caller.  Otherwise, every place that calls validatePartConstraint()
> has to think about whether or not the constraint-is-NULL case needs to
> be handled.
>
> I have now added a check in ValidatePartConstraint(). This change is made
in 0001 patch.



> > 0003:
> > Support for default partition with the restriction of preventing
> addition of
> > any
> > new partition after default partition.
>
> This looks generally reasonable, but can't really be committed without
> the later patches, because it might break pg_dump, which won't know
> that the DEFAULT partition must be dumped last and might therefore get
> the dump ordering wrong, and of course also because it reverts commit
> c1e0e7e1d790bf18c913e6a452dea811e858b554.
>
>
Thanks Robert for looking into this. The purpose of having different
patches is
just to ease the review process and the basic patch of introducing the
default
partition support and extending support for addition of new partition
should go
together.


> > 0004:
> > Store the default partition OID in pg_partition_table, this will help us
> to
> > retrieve the OID of default relation when we don't have the relation
> cache
> > available. This was also suggested by Amit Langote here[1].
>
> I looked this over and I think this is the right approach.  An
> alternative way to avoid needing a relcache entry in
> heap_drop_with_catalog() would be for get_default_partition_oid() to
> call find_inheritance_children() here and then use a syscache lookup
> to get the partition bound for each one, but that's still going to
> cause some syscache bloat.
>

To safeguard future development from missing this and leaving it out of
sync, I
have added an Assert in RelationBuildPartitionDesc() to cross check the
partdefid.


>
> > 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> +   if (spec->is_default)
> +   {
> +   /* Default partition cannot be added if there already
> exists one. */
> +   if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> +   {
> +   with = boundinfo->default_index;
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> +   relname,
> get_rel_name(partdesc->oids[with])),
> +parser_errposition(pstate,
> spec->location)));
> +   }
> +
> +   return;
> +   }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level.  In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented.  Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.
>

Fixed.

> 0006:
> > Extend default partitioning validation code to reuse the refactored code
> in
> > patch 0001.
>
> I'm having a very hard time understanding what's going on with this
> patch.  It certainly doesn't seem to be just refactoring things to use
> the code from 0001.  For example:
>
> -   if (ExecCheck(partqualstate, econtext))
> +   if (!ExecCheck(partqualstate, econtext))
>
> It seems hard to believe that refactoring things to use the code from
> 0001 would involve inverting the value of this test.
>
> +* derived from the bounds(the partition constraint
> never evaluates to
> +* NULL, so negating it like this is safe).
>
> I don't see it being negated.
>
> I think this patch needs a better explanation of what it's trying to
> do, and better comments.  I gather that at least part of the point
> here is to skip validation scans on default partitions if the default
> partition has been constrained not to contain any values that would
> fall in the new partition, but neither the commit message for 0006 nor
> your description here make that very clear.
>

In V25 series, this is now part of patch 0004, and should avoid any
confusion as above. I have tried to add verbose comment in commit
message as well as I have improved the code comments in this code
area.

> [0008 documentation]
>
> -  attached is marked NO INHERIT, the command will
> fail;
> -  such a constraint must be recreated 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

On Thu, Aug 17, 2017 at 3:41 PM, Jeevan Ladhe  wrote:

> Hi Ashutosh,
>
> Please find my feedback inlined.
>
> On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>>  wrote:
>> > Hi,
>> >
>> > I have rebased the patches on the latest commit.
>> >
>>
>> Thanks for rebasing the patches. The patches apply and compile
>> cleanly. make check passes.
>>
>> Here are some review comments
>> 0001 patch
>> Most of this patch is same as 0002 patch posted in thread [1]. I have
>> extensively reviewed that patch for Amit Langote. Can you please compare
>> these
>> two patches and try to address those comments OR just use patch from that
>> thread? For example, canSkipPartConstraintValidation() is named as
>> PartConstraintImpliedByRelConstraint() in that patch. OR
>> +if (scanRel_constr == NULL)
>> +return false;
>> +
>> is not there in that patch since returning false is wrong when
>> partConstraint
>> is NULL. I think this patch needs those fixes. Also, this patch set would
>> need
>> a rebase when 0001 from that thread gets committed.
>>
>>
> I have renamed the canSkipPartConstraintValidation() to
> PartConstraintImpliedByRelConstraint() and made other changes applicable
> per
> Amit’s patch. This patch also refactors the scanning logic in
> ATExecAttachPartition()
> and adds it into a function ValidatePartitionConstraints(), hence I could
> not use
> Amit’s patch as it is. Please have a look into the new patch and let me
> know if it
> looks fine to you.
>
>
>> 0002 patch
>> +if (!and_args)
>> +result = NULL;
>> Add "NULL, if there are not partition constraints e.g. in case of default
>> partition as the only partition.".
>
>
> Added. Please check.
>
>
>> This patch avoids calling
>> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
>> when
>> partConstraint is NULL, but patches in [1] introduce more callers of
>> canSkipPartConstraintValidation() which may pass NULL. So, it's better
>> that we
>> handle that case.
>>
>
> Following code added in patch 0001 now should take care of this.
> +   num_check = (constr != NULL) ? constr->num_check : 0;
>
>
>> 0003 patch
>> +parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>> +heap_close(parentRel, NoLock);
>>
>
> As clarified earlier this was addressed in 0004 patch of V24 series. In
> current set of patches this is now addressed in patch 0003 itself.
>
>
>>
>> +/*
>> + * The default partition accepts any
>> non-specified
>> + * value, hence it should not get a mapped index
>> while
>> + * assigning those for non-null datums.
>> + */
>> Instead of "any non-specified value", you may want to use "any value not
>> specified in the lists of other partitions" or something like that.
>>
>
> Changed the comment.
>
>
>>
>> + * If this is a NULL, route it to the null-accepting partition.
>> + * Otherwise, route by searching the array of partition bounds.
>> You may want to write it as "If this is a null partition key, ..." to
>> clarify
>> what's NULL.
>>
>
> Changed the comment.
>
>
>>
>> + * cur_index < 0 means we could not find a non-default partition
>> of
>> + * this parent. cur_index >= 0 means we either found the leaf
>> + * partition, or the next parent to find a partition of.
>> + *
>> + * If we couldn't find a non-default partition check if the
>> default
>> + * partition exists, if it does, get its index.
>> In order to avoid repeating "we couldn't find a ..."; you may want to add
>> ",
>> try default partition if one exists." in the first sentence itself.
>>
>
> Sorry, but I am not really sure how this change would make the comment
> more readable than the current one.
>
>
>> get_default_partition_oid() is defined in this patch and then redefined in
>> 0004. Let's define it only once, mostly in or before 0003 patch.
>>
>
> get_default_partition_oid() is now defined only once in patch 0003.
>
>
>>
>> + * partition strategy. Assign the parent strategy to the default
>> s/parent/parent's/
>>
>
> Fixed.
>
>
>>
>> +-- attaching default partition overlaps if the default partition already
>> exists
>> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
>> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
>> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
>> +ERROR:  

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

Please find my feedback inlined.

On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>  wrote:
> > Hi,
> >
> > I have rebased the patches on the latest commit.
> >
>
> Thanks for rebasing the patches. The patches apply and compile
> cleanly. make check passes.
>
> Here are some review comments
> 0001 patch
> Most of this patch is same as 0002 patch posted in thread [1]. I have
> extensively reviewed that patch for Amit Langote. Can you please compare
> these
> two patches and try to address those comments OR just use patch from that
> thread? For example, canSkipPartConstraintValidation() is named as
> PartConstraintImpliedByRelConstraint() in that patch. OR
> +if (scanRel_constr == NULL)
> +return false;
> +
> is not there in that patch since returning false is wrong when
> partConstraint
> is NULL. I think this patch needs those fixes. Also, this patch set would
> need
> a rebase when 0001 from that thread gets committed.
>
>
I have renamed the canSkipPartConstraintValidation() to
PartConstraintImpliedByRelConstraint() and made other changes applicable per
Amit’s patch. This patch also refactors the scanning logic in
ATExecAttachPartition()
and adds it into a function ValidatePartitionConstraints(), hence I could
not use
Amit’s patch as it is. Please have a look into the new patch and let me
know if it
looks fine to you.


> 0002 patch
> +if (!and_args)
> +result = NULL;
> Add "NULL, if there are not partition constraints e.g. in case of default
> partition as the only partition.".


Added. Please check.


> This patch avoids calling
> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
> when
> partConstraint is NULL, but patches in [1] introduce more callers of
> canSkipPartConstraintValidation() which may pass NULL. So, it's better
> that we
> handle that case.
>

Following code added in patch 0001 now should take care of this.
+   num_check = (constr != NULL) ? constr->num_check : 0;


> 0003 patch
> +parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
> +heap_close(parentRel, NoLock);
>

As clarified earlier this was addressed in 0004 patch of V24 series. In
current set of patches this is now addressed in patch 0003 itself.


>
> +/*
> + * The default partition accepts any non-specified
> + * value, hence it should not get a mapped index
> while
> + * assigning those for non-null datums.
> + */
> Instead of "any non-specified value", you may want to use "any value not
> specified in the lists of other partitions" or something like that.
>

Changed the comment.


>
> + * If this is a NULL, route it to the null-accepting partition.
> + * Otherwise, route by searching the array of partition bounds.
> You may want to write it as "If this is a null partition key, ..." to
> clarify
> what's NULL.
>

Changed the comment.


>
> + * cur_index < 0 means we could not find a non-default partition
> of
> + * this parent. cur_index >= 0 means we either found the leaf
> + * partition, or the next parent to find a partition of.
> + *
> + * If we couldn't find a non-default partition check if the
> default
> + * partition exists, if it does, get its index.
> In order to avoid repeating "we couldn't find a ..."; you may want to add
> ",
> try default partition if one exists." in the first sentence itself.
>

Sorry, but I am not really sure how this change would make the comment
more readable than the current one.


> get_default_partition_oid() is defined in this patch and then redefined in
> 0004. Let's define it only once, mostly in or before 0003 patch.
>

get_default_partition_oid() is now defined only once in patch 0003.


>
> + * partition strategy. Assign the parent strategy to the default
> s/parent/parent's/
>

Fixed.


>
> +-- attaching default partition overlaps if the default partition already
> exists
> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
> +ERROR:  cannot attach a new partition to table "list_parted" having a
> default partition
> For 0003 patch this testcase is same as the testcase in the next hunk; no
> new
> partition can be added after default partition. Please add this testcase in
> next set of patches.
>

Though the 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/07/11 6:56, Robert Haas wrote:

On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
 wrote:

So, I dropped the COPY part.


Ouch.  I think we should try to figure out how the COPY part will be
handled before we commit to a design.


I spent some time on this.  To handle that, I'd like to propose doing 
something similar to \copy (frontend copy): submit a COPY query "COPY 
... FROM STDIN" to the remote server and route data from a file to the 
remote server.  For that, I'd like to add new FDW APIs called during 
CopyFrom that allow us to copy to foreign tables:


* BeginForeignCopyIn: this would be called after creating a 
ResultRelInfo for the target table (or each leaf partition of the target 
partitioned table) if it's a foreign table, and perform any 
initialization needed before the remote copy can start.  In the 
postgres_fdw case, I think this function would be a good place to send 
"COPY ... FROM STDIN" to the remote server.


* ExecForeignCopyInOneRow: this would be called instead of heap_insert 
if the target is a foreign table, and route the tuple read from the file 
by NextCopyFrom to the remote server.  In the postgres_fdw case, I think 
this function would convert the tuple to text format for portability, 
and then send the data to the remote server using PQputCopyData.


* EndForeignCopyIn: this would be called at the bottom of CopyFrom, and 
release resources such as connections to the remote server.  In the 
postgres_fdw case, this function would do PQputCopyEnd to terminate data 
transfer.


I think that would be much more efficient than INSERTing tuples into the 
remote server one by one.  What do you think about that?



I have to admit that I'm a little bit fuzzy about why foreign insert
routing requires all of these changes.  I think this patch would
benefit from being accompanied by several paragraphs of explanation
outlining the rationale for each part of the patch.


Will do.

Best regards,
Etsuro Fujita



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
 wrote:
> I have rebased the patches on the latest commit.

This needs another rebase.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Mon, Aug 14, 2017 at 7:51 AM, Jeevan Ladhe
 wrote:
> I think even with this change there will be one level of indentation
> needed for throwing the error, as the error is to be thrown only if
> there exists a default partition.

That's true, but we don't need two levels.

>> > 0007:
>> > This patch introduces code to check if the scanning of default partition
>> > child
>> > can be skipped if it's constraints are proven.
>>
>> If I understand correctly, this is actually a completely separate
>> feature not intrinsically related to default partitioning.
>
> I don't see this as a new feature, since scanning the default partition
> will be introduced by this series of patches only, and rather than a
> feature this can be classified as a completeness of default skip
> validation logic. Your thoughts?

Currently, when a partitioned table is attached, we check whether all
the scans can be checked but not whether scans on some partitions can
be attached.  So there are two separate things:

1. When we introduce default partitioning, we need scan the default
partition either when (a) any partition is attached or (b) any
partition is created.

2. In any situation where scans are needed (scanning the partition
when it's attached, scanning the default partition when some other
partition is attached, scanning the default when a new partition is
created), we can run predicate_implied_by for each partition to see
whether the scan of that partition can be skipped.

Those two changes are independent. We could do (1) without doing (2)
or (2) without doing (1) or we could do both.  So they are separate
features.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,


> 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> +   if (spec->is_default)
> +   {
> +   /* Default partition cannot be added if there already
> exists one. */
> +   if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> +   {
> +   with = boundinfo->default_index;
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> +   relname,
> get_rel_name(partdesc->oids[with])),
> +parser_errposition(pstate,
> spec->location)));
> +   }
> +
> +   return;
> +   }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level.  In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented.  Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.


I think even with this change there will be one level of indentation
needed for throwing the error, as the error is to be thrown only if
there exists a default partition.


>
>
-* We must also lock the default partition, for the same
> reasons explained
> -* in heap_drop_with_catalog().
> +* We must lock the default partition, for the same reasons
> explained in
> +* DefineRelation().
>
> I don't really see the point of this change.  Whichever earlier patch
> adds this code could include or omit the word "also" as appropriate,
> and then this patch wouldn't need to change it.
>
>
Actually the change is made because if the difference in the function name.
I will remove ‘also’ from the first patch itself.


> > 0007:
> > This patch introduces code to check if the scanning of default partition
> > child
> > can be skipped if it's constraints are proven.
>
> If I understand correctly, this is actually a completely separate
> feature not intrinsically related to default partitioning.


I don't see this as a new feature, since scanning the default partition
will be introduced by this series of patches only, and rather than a
feature this can be classified as a completeness of default skip
validation logic. Your thoughts?

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jul 12, 2017 at 3:31 PM, Jeevan Ladhe
 wrote:
> 0001:
> Refactoring existing ATExecAttachPartition  code so that it can be used for
> default partitioning as well

Boring refactoring.  Seems fine.

> 0002:
> This patch teaches the partitioning code to handle the NIL returned by
> get_qual_for_list().
> This is needed because a default partition will not have any constraints in
> case
> it is the only partition of its parent.

Perhaps it would be better to make validatePartConstraint() a no-op
when the constraint is empty rather than putting the logic in the
caller.  Otherwise, every place that calls validatePartConstraint()
has to think about whether or not the constraint-is-NULL case needs to
be handled.

> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition.

This looks generally reasonable, but can't really be committed without
the later patches, because it might break pg_dump, which won't know
that the DEFAULT partition must be dumped last and might therefore get
the dump ordering wrong, and of course also because it reverts commit
c1e0e7e1d790bf18c913e6a452dea811e858b554.

> 0004:
> Store the default partition OID in pg_partition_table, this will help us to
> retrieve the OID of default relation when we don't have the relation cache
> available. This was also suggested by Amit Langote here[1].

I looked this over and I think this is the right approach.  An
alternative way to avoid needing a relcache entry in
heap_drop_with_catalog() would be for get_default_partition_oid() to
call find_inheritance_children() here and then use a syscache lookup
to get the partition bound for each one, but that's still going to
cause some syscache bloat.

> 0005:
> Extend default partitioning support to allow addition of new partitions.

+   if (spec->is_default)
+   {
+   /* Default partition cannot be added if there already
exists one. */
+   if (partdesc->nparts > 0 &&
partition_bound_has_default(boundinfo))
+   {
+   with = boundinfo->default_index;
+   ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+errmsg("partition \"%s\"
conflicts with existing default partition \"%s\"",
+   relname,
get_rel_name(partdesc->oids[with])),
+parser_errposition(pstate,
spec->location)));
+   }
+
+   return;
+   }

I generally think it's good to structure the code so as to minimize
the indentation level.  In this case, if you did if (partdesc->nparts
== 0 || !partition_bound_has_default(boundinfo)) return; first, then
the rest of it could be one level less indented.  Also, perhaps it
would be clearer to test boundinfo == NULL rather than
partdesc->nparts == 0, assuming they are equivalent.

-* We must also lock the default partition, for the same
reasons explained
-* in heap_drop_with_catalog().
+* We must lock the default partition, for the same reasons explained in
+* DefineRelation().

I don't really see the point of this change.  Whichever earlier patch
adds this code could include or omit the word "also" as appropriate,
and then this patch wouldn't need to change it.

> 0006:
> Extend default partitioning validation code to reuse the refactored code in
> patch 0001.

I'm having a very hard time understanding what's going on with this
patch.  It certainly doesn't seem to be just refactoring things to use
the code from 0001.  For example:

-   if (ExecCheck(partqualstate, econtext))
+   if (!ExecCheck(partqualstate, econtext))

It seems hard to believe that refactoring things to use the code from
0001 would involve inverting the value of this test.

+* derived from the bounds(the partition constraint
never evaluates to
+* NULL, so negating it like this is safe).

I don't see it being negated.

I think this patch needs a better explanation of what it's trying to
do, and better comments.  I gather that at least part of the point
here is to skip validation scans on default partitions if the default
partition has been constrained not to contain any values that would
fall in the new partition, but neither the commit message for 0006 nor
your description here make that very clear.

> 0007:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.

If I understand correctly, this is actually a completely separate
feature not intrinsically related to default partitioning.

> [0008 documentation]

-  attached is marked NO INHERIT, the command will fail;
-  such a constraint must be recreated without the NO
INHERIT
-  clause.
+  target table.
+ 

I don't favor inserting a 

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Sun, Jul 30, 2017 at 8:07 AM, Jeevan Ladhe
 wrote:
> Hi Ashutosh,
>
> 0003 patch
>>
>> +parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>
>
> I think the patch 0004 exactly does what you have said here, i.e. it gets
> rid of the heap_open() and heap_close().
> The question might be why I kept the patch 0004 a separate one, and the
> answer is I wanted to make it easier for review, and also keeping it that
> way would make it bit easy to work on a different approach if needed.
>

The reviewer has to review two different set of changes to the same
portion of the code. That just doubles the work. I didn't find that
simplifying review. As I have suggested earlier, let's define
get_default_partition_oid() only once, mostly in or before 0003 patch.
Having it in a separate patch would allow you to change its
implementation if needed.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Sat, Jul 29, 2017 at 2:55 AM, Robert Haas  wrote:
> On Fri, Jul 28, 2017 at 9:30 AM, Ashutosh Bapat
>  wrote:
>> 0004 patch
>> The patch adds another column partdefid to catalog pg_partitioned_table. The
>> column gives OID of the default partition for a given partitioned table. This
>> means that the default partition's OID is stored at two places 1. in the
>> default partition table's pg_class entry and in pg_partitioned_table. There 
>> is
>> no way to detect when these two go out of sync. Keeping those two in sync is
>> also a maintenance burdern. Given that default partition's OID is required 
>> only
>> while adding/dropping a partition, which is a less frequent operation, it 
>> won't
>> hurt to join a few catalogs (pg_inherits and pg_class in this case) to find 
>> out
>> the default partition's OID. That will be occasional performance hit
>> worth the otherwise maintenance burden.
>
> Performance isn't the only consideration here.  We also need to think
> about locking and concurrency.  I think that most operations that
> involve locking the parent will also involve locking the default
> partition.  However, we can't safely build a relcache entry for a
> relation before we've got some kind of lock on it.  We can't assume
> that there is no concurrent DDL going on before we take some lock.  We
> can't assume invalidation messages are processed before we have taken
> some lock.  If we read multiple catalog tuples, they may be from
> different points in time.  If we can figure out everything we need to
> know from one or two syscache lookups, it may be easier to verify that
> the code is bug-free vs. having to do something more complicated.
>

The code takes a lock on the parent relation. While that function
holds that lock nobody else would change partitions of that relation
and hence nobody changes the default partition.
heap_drop_with_catalog() has code to lock the parent. Looking up
pg_inherits catalog for its partitions followed by identifying the
partition which has default partition bounds specification while
holding the lock on the parent should be safe. Any changes to
partition bounds, or partitions would require lock on the parent. In
order to prevent any buggy code changing the default partition without
sufficient locks, we should lock the default partition after it's
found and check the default partition bound specification again. Will
that work?

> Now that having been said, I'm not taking the position that Jeevan's
> patch (based on Amit Langote's idea) has definitely got the right
> idea, just that you should think twice before shooting down the
> approach.
>

If we can avoid the problems specified by Amit Langote, I am fine with
the approach of reading the default partition OID from the Relcache as
well. But I am not able to device a solution to those problems.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

0003 patch

> +parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
>

I think the patch 0004 exactly does what you have said here, i.e. it gets
rid of the heap_open() and heap_close().
The question might be why I kept the patch 0004 a separate one, and the
answer is I wanted to make it easier for review, and also keeping it that
way would make it bit easy to work on a different approach if needed.

About this: *"Also I don't see get_default_partition_oid() using Relation
anywhere."*
The get_default_partition_oid() uses parent relation to
retrieve PartitionDesc
from parent.

Kindly let me know if you think I am still missing anything.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Jul 28, 2017 at 9:30 AM, Ashutosh Bapat
 wrote:
> 0004 patch
> The patch adds another column partdefid to catalog pg_partitioned_table. The
> column gives OID of the default partition for a given partitioned table. This
> means that the default partition's OID is stored at two places 1. in the
> default partition table's pg_class entry and in pg_partitioned_table. There is
> no way to detect when these two go out of sync. Keeping those two in sync is
> also a maintenance burdern. Given that default partition's OID is required 
> only
> while adding/dropping a partition, which is a less frequent operation, it 
> won't
> hurt to join a few catalogs (pg_inherits and pg_class in this case) to find 
> out
> the default partition's OID. That will be occasional performance hit
> worth the otherwise maintenance burden.

Performance isn't the only consideration here.  We also need to think
about locking and concurrency.  I think that most operations that
involve locking the parent will also involve locking the default
partition.  However, we can't safely build a relcache entry for a
relation before we've got some kind of lock on it.  We can't assume
that there is no concurrent DDL going on before we take some lock.  We
can't assume invalidation messages are processed before we have taken
some lock.  If we read multiple catalog tuples, they may be from
different points in time.  If we can figure out everything we need to
know from one or two syscache lookups, it may be easier to verify that
the code is bug-free vs. having to do something more complicated.

Now that having been said, I'm not taking the position that Jeevan's
patch (based on Amit Langote's idea) has definitely got the right
idea, just that you should think twice before shooting down the
approach.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
 wrote:
> Hi,
>
> I have rebased the patches on the latest commit.
>

Thanks for rebasing the patches. The patches apply and compile
cleanly. make check passes.

Here are some review comments
0001 patch
Most of this patch is same as 0002 patch posted in thread [1]. I have
extensively reviewed that patch for Amit Langote. Can you please compare these
two patches and try to address those comments OR just use patch from that
thread? For example, canSkipPartConstraintValidation() is named as
PartConstraintImpliedByRelConstraint() in that patch. OR
+if (scanRel_constr == NULL)
+return false;
+
is not there in that patch since returning false is wrong when partConstraint
is NULL. I think this patch needs those fixes. Also, this patch set would need
a rebase when 0001 from that thread gets committed.

0002 patch
+if (!and_args)
+result = NULL;
Add "NULL, if there are not partition constraints e.g. in case of default
partition as the only partition.". This patch avoids calling
validatePartitionConstraints() and hence canSkipPartConstraintValidation() when
partConstraint is NULL, but patches in [1] introduce more callers of
canSkipPartConstraintValidation() which may pass NULL. So, it's better that we
handle that case.

0003 patch
+parentRel = heap_open(parentOid, AccessExclusiveLock);
In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
should not heap_open() the parent relation. But this patch still calls
heap_open() without giving any counter argument. Also I don't see
get_default_partition_oid() using Relation anywhere. If you remove that
heap_open() please remove following heap_close().
+heap_close(parentRel, NoLock);

+/*
+ * The default partition accepts any non-specified
+ * value, hence it should not get a mapped index while
+ * assigning those for non-null datums.
+ */
Instead of "any non-specified value", you may want to use "any value not
specified in the lists of other partitions" or something like that.

+ * If this is a NULL, route it to the null-accepting partition.
+ * Otherwise, route by searching the array of partition bounds.
You may want to write it as "If this is a null partition key, ..." to clarify
what's NULL.

+ * cur_index < 0 means we could not find a non-default partition of
+ * this parent. cur_index >= 0 means we either found the leaf
+ * partition, or the next parent to find a partition of.
+ *
+ * If we couldn't find a non-default partition check if the default
+ * partition exists, if it does, get its index.
In order to avoid repeating "we couldn't find a ..."; you may want to add ",
try default partition if one exists." in the first sentence itself.

get_default_partition_oid() is defined in this patch and then redefined in
0004. Let's define it only once, mostly in or before 0003 patch.

+ * partition strategy. Assign the parent strategy to the default
s/parent/parent's/

+-- attaching default partition overlaps if the default partition already exists
+CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
+CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
+ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
+ERROR:  cannot attach a new partition to table "list_parted" having a
default partition
For 0003 patch this testcase is same as the testcase in the next hunk; no new
partition can be added after default partition. Please add this testcase in
next set of patches.

+-- fail
+insert into part_default values ('aa', 2);
May be explain why the insert should fail. "A row, which would fit
other partition, does not fit default partition, even when inserted directly"
or something like that. I see that many of the tests in that file do not
explain why something should "fail" or be "ok", but may be it's better to
document the reason for better readability and future reference.

+-- check in case of multi-level default partitioned table
s/in/the/ ?. Or you may want to reword it as "default partitioned partition in
multi-level partitioned table" as there is nothing like "default partitioned
table". May be we need a testcase where every level of a multi-level
partitioned table has a default partition.

+-- drop default, as we need to add some more partitions to test tuple routing
Should be clubbed with the actual DROP statement?

+-- Check that addition or removal of any partition is correctly dealt with by
+-- default partition table when it is being used in cached plan.
Plan of a prepared statement gets cached only after it's executed 5 times.
Before that the statement gets invalidated but there's not cached plan that
gets invalidated. The test is fine here, but in order to test the cached plan
as mentioned in the comment, you 

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,

On Thu, Jul 13, 2017 at 1:22 AM, Jeevan Ladhe
 wrote:
>
>> - Should probably be merged with the patch to add default partitioning
>> for ranges.
>
>
> Beena is already rebasing her patch on my latest patches, so I think getting
> them merged here won't be an issue, mostly will be just like one more patch
> on top my patches.
>

I have posted the updated patch which can be applied over the v22
patches submitted here.
https://www.postgresql.org/message-id/CAOG9ApGEZxSQD-ZD3icj_CwTmprSGG7sZ_r3k9m4rmcc6ozr%3Dg%40mail.gmail.com

Thank you,

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, I'd like to review this but it doesn't fit the master, as
> Robert said. Especially the interface of predicate_implied_by is
> changed by the suggested commit.
>
> Anyway I have some comment on this patch with fresh eyes.  I
> believe the basic design so my comment below are from a rather
> micro viewpoint.
>
> - Considering on how canSkipPartConstraintValidation is called, I
>   *think* that RelationProvenValid() would be better.  (But this
>   would be disappear by rebasing..)
>
> I think RelationProvenValid() is bit confusing in the sense that, it does
not
imply the meaning that some constraint is being checke


> - 0002 changes the interface of get_qual_for_list, but left
>   get_qual_for_range alone.  Anyway get_qual_for_range will have
>   to do the similar thing soon.
>
> Yes, this will be taken care with default partition for range.


> - In check_new_partition_bound, "overlap" and "with" is
>   completely correlated with each other. "with > -1" means
>   "overlap = true". So overlap is not useless. ("with" would be
>   better to be "overlap_with" or somehting if we remove
>   "overlap")
>
> Agree, probably this can be taken as a separate refactoring patch.
Currently,
for in case of default I have got rid of "overlap", and now use of "with"
and
that is also used just for code simplification.


> - The error message of check_default_allows_bound is below.
>
>   "updated partition constraint for default partition \"%s\"
>would be violated by some row"
>
>   This looks an analog of validateCheckConstraint but as my
>   understanding this function is called only when new partition
>   is added. This would be difficult to recognize in the
>   situation.
>
>   "the default partition contains rows that should be in
>the new partition: \"%s\""
>
>   or something?
>
> I think the current error message is more clearer. Agree that there might
be
sort of confusion if it's due to addition or attach partition, but we have
already stretched the message longer. I am open to suggestions here.


> - In check_default_allows_bound, the iteration over partitions is
>   quite similar to what validateCheckConstraint does. Can we
>   somehow share validateCheckConstraint with this function?
>
> May be we can, but I think again this can also be categorized as
refactoring
patch and done later maybe. Your thoughts?


> - In the same function, skipping RELKIND_PARTITIONED_TABLE is
>   okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
>   good. I think at least some warning should be emitted.
>
>   "Skipping foreign tables in the defalut partition. It might
>contain rows that should be in the new partition."  (Needs
>preventing multple warnings in single call, maybe)
>
> Currently we do not emit any warning when attaching a foreign table as a
non-default partition having rows that do not match its partition
constraints
and we still let attach the partition.
But, I agree that we should emit such a warning, I added a code to do this.


> - In the same function, the following condition seems somewhat
>   strange in comparison to validateCheckConstraint.
>
> > if (partqualstate && ExecCheck(partqualstate, econtext))
>
>   partqualstate won't be null as long as partition_constraint is
>   valid. Anyway (I'm believing that) an invalid constraint
>   results in error by ExecPrepareExpr. Therefore 'if
>   (partqualstate' is useless.
>
> Removed the check for partqualstate.


> - In gram.y, the nonterminal for list spec clause is still
>   "ForValues". It seems somewhat strange. partition_spec or
>   something would be better.
>
> Done.
Thanks for catching this, I agree with you.
I have changed the name to PartitionBoundSpec.


> - This is not a part of this patch, but in ruleutils.c, the error
>   for unknown paritioning strategy is emitted as following.
>
> >   elog(ERROR, "unrecognized partition strategy: %d",
> >(int) strategy);
>
>   The cast is added because the strategy is a char. I suppose
>   this is because strategy can be an unprintable. I'd like to see
>   a comment if it is correct.
>
> I think this should be taken separately.

Thanks,
Jeevan Ladhe

Refs:
[1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%
2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Thu, Jun 15, 2017 at 10:24 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Some more comments on the latest set of patches.
>
> In heap_drop_with_catalog(), we heap_open() the parent table to get the
> default partition OID, if any. If the relcache doesn't have an entry for
> the
> parent, this means that the entry will be created, only to be invalidated
> at
> the end of the function. If there is no default partition, this all is
> completely unnecessary. We should avoid heap_open() in this case. This also
> means that get_default_partition_oid() should not rely on the relcache
> entry,
> but should growl through pg_inherit to find the default partition.
>

Instead of reading the defaultOid from cache, as suggested by Amit here[2],
now
I have stored this in pg_partition_table, and reading it from there.


> In get_qual_for_list(), if the table has only default partition, it won't
> have
> any boundinfo. In such a case the default partition's constraint would
> come out
> as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty
> array
> looks odd and may be we spend a few CPU cycles executing ANY on an empty
> array.
> We have the same problem with a partition containing only NULL value. So,
> may
> be this one is not that bad.
>
> Fixed.


> Please add a testcase to test addition of default partition as the first
> partition.
>
> Added this in insert.sql rather than create_table.sql, as the purpose here
is to test if default being a first partition accepts any values for the key
including null.


> get_qual_for_list() allocates the constant expressions corresponding to the
> datums in CacheMemoryContext while constructing constraints for a default
> partition. We do not do this for other partitions. We may not be
> constructing
> the constraints for saving in the cache. For example, ATExecAttachPartition
> constructs the constraints for validation. In such a case, this code will
> unnecessarily clobber the cache memory. generate_partition_qual() copies
> the
> partition constraint in the CacheMemoryContext.
>

Removed CacheMemoryContext.
I thought once the partition qual is generated, it should be in remain in
the memory context, but when it is needed, it is indirectly taken care by
generate_partition_qual() in following code:

/* Save a copy in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
rel->rd_partcheck = copyObject(result);
MemoryContextSwitchTo(oldcxt);


>
> +   if (spec->is_default)
> +   {
> +   result = list_make1(make_ands_explicit(result));
> +   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
> +   }
>
> If the "result" is an OR expression, calling make_ands_explicit() on it
> would
> create AND(OR(...)) expression, with an unnecessary AND. We want to avoid
> that?
>
>
Actually the OR expression here is generated using a call to makeBoolExpr(),
which returns a single expression node, and if this is passed to
make_ands_explicit(), it checks if the list length is node, returns the
initial
node itself, and hence AND(OR(...)) kind of expression is not generated
here.


> +   if (cur_index < 0 && (partition_bound_has_default(
> partdesc->boundinfo)))
> +   cur_index = partdesc->boundinfo->default_index;
> +
> The partition_bound_has_default() check is unnecessary since we check for
> cur_index < 0 next anyway.
>
> Done.


> + *
> + * Given the parent relation checks if it has default partition, and if it
> + * does exist returns its oid, otherwise returns InvalidOid.
> + */
> May be reworded as "If the given relation has a default partition, this
> function returns the OID of the default partition. Otherwise it returns
> InvalidOid."
>
> I have reworded this to:
"If the given relation has a default partition return the OID of the default
partition, otherwise return InvalidOid."


> +Oid
> +get_default_partition_oid(Relation parent)
> +{
> +   PartitionDesc partdesc = RelationGetPartitionDesc(parent);
> +
> +   if (partdesc->boundinfo && partition_bound_has_default(
> partdesc->boundinfo))
> +   return partdesc->oids[partdesc->boundinfo->default_index];
> +
> +   return InvalidOid;
> +}
> An unpartitioned table would not have partdesc set set. So, this function
> will
> segfault if we pass an unpartitioned table. Either Assert that partdesc
> should
> exist or check for its NULL-ness.
>

Fixed.


>
>
> +defaultPartOid = get_default_partition_oid(rel);
> +if (OidIsValid(defaultPartOid))
> +ereport(ERROR,
> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("there exists a default partition for table
> \"%s\", cannot attach a new partition",
> +RelationGetRelationName(rel;
> +
> Should be done before heap_open on the table being attached. If we are not
> going to attach the partition, there's no 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.


On Thu, Jun 15, 2017 at 1:21 AM, Robert Haas  wrote:
>
> - Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
>

Rebased on master latest commit: ca793c59a51e94cedf8cbea5c29668bf8fa298f3


> - Still no documentation.
>
Yes, this is long pending, and I will make this is a priority to get it
included
in next set of my patches.

- Should probably be merged with the patch to add default partitioning
> for ranges.


Beena is already rebasing her patch on my latest patches, so I think getting
them merged here won't be an issue, mostly will be just like one more patch
on top my patches.


> Other stuff I noticed:
>
> - The regression tests don't seem to check that the scan-skipping
> logic works as expected.  We have regression tests for that case for
> attaching regular partitions, and it seems like it would be worth
> testing the default-partition case as well.
>

Added a test case for default in alter_table.sql.

- check_default_allows_bound() assumes that if
> canSkipPartConstraintValidation() fails for the default partition, it
> will also fail for every subpartition of the default partition.  That
> is, once we commit to scanning one child partition, we're committed to
> scanning them all.  In practice, that's probably not a huge
> limitation, but if it's not too much code, we could keep the top-level
> check but also check each partitioning individually as we reach it,
> and skip the scan for any individual partitions for which the
> constraint can be proven.  For example, suppose the top-level table is
> list-partitioned with a partition for each of the most common values,
> and then we range-partition the default partition.
>

I have tried to address this in patch 0007, please let me know your views on
that patch.

- The changes to the regression test results in 0004 make the error
> messages slightly worse.  The old message names the default partition,
> whereas the new one does not.  Maybe that's worth avoiding.


The only way for this, I can think of to achieve this is to store the name
of
the default relation in AlteredTableInfo, currently I am using a flag for
realizing if the scanned table is a default partition to throw specific
error.
But, IMO storing a string in AlteredTableInfo just for error purpose might
be
overkill. Your suggestions?


> Specific comments:
>
> + * Also, invalidate the parent's and a sibling default partition's
> relcache,
> + * so that the next rebuild will load the new partition's info into
> parent's
> + * partition descriptor and default partition constraints(which are
> dependent
> + * on other partition bounds) are built anew.
>
> I find this a bit unclear, and it also repeats the comment further
> down.  Maybe something like: Also, invalidate the parent's relcache
> entry, so that the next rebuild will load he new partition's info into
> its partition descriptor.  If there is a default partition, we must
> invalidate its relcache entry as well.
>
> Done.


> +/*
> + * The default partition constraints depend upon the partition bounds
> of
> + * other partitions. Adding a new(or even removing existing) partition
> + * would invalidate the default partition constraints. Invalidate the
> + * default partition's relcache so that the constraints are built
> anew and
> + * any plans dependent on those constraints are invalidated as well.
> + */
>
> Here, I'd write: The partition constraint for the default partition
> depends on the partition bounds of every other partition, so we must
> invalidate the relcache entry for that partition every time a
> partition is added or removed.
>
> Done.


> +/*
> + * Default partition cannot be added if there already
> + * exists one.
> + */
> +if (spec->is_default)
> +{
> +overlap = partition_bound_has_default(boundinfo);
> +with = boundinfo->default_index;
> +break;
> +}
>
> To support default partitioning for range, this is going to have to be
> moved above the switch rather than done inside of it.  And there's
> really no downside to putting it there.
>
> Done.


> + * constraint, by *proving* that the existing constraints of the table
> + * *imply* the given constraints.  We include the table's check
> constraints and
>
> Both the comma and the asterisks are unnecessary.
>
> Done.


> + * Check whether all rows in the given table (scanRel) obey given
> partition
>
> obey the given
>
> I think the larger comment block could be tightened up a bit, like
> this:  Check whether all rows in the given table obey the given
> partition constraint; if so, it can be attached as a partition.  We do
> this by scanning the table (or all of its 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita
 wrote:
> So, I dropped the COPY part.

Ouch.  I think we should try to figure out how the COPY part will be
handled before we commit to a design.

I have to admit that I'm a little bit fuzzy about why foreign insert
routing requires all of these changes.  I think this patch would
benefit from being accompanied by several paragraphs of explanation
outlining the rationale for each part of the patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

On 2017/07/05 9:13, Amit Langote wrote:

On 2017/06/29 20:20, Etsuro Fujita wrote:



In relation to this, I also allowed expand_inherited_rtentry() to
build an RTE and AppendRelInfo for each partition of a partitioned table
that is an INSERT target, as mentioned by Amit in [1], by modifying
transformInsertStmt() so that we set the inh flag for the target table's
RTE to true when the target table is a partitioned table.


About this part, Robert sounded a little unsure back then [1]; his words:

"Doing more inheritance expansion early is probably not a good idea
because it likely sucks for performance, and that's especially unfortunate
if it's only needed for foreign tables."

But...


The partition
RTEs are not only referenced by FDWs, but used in selecting relation
aliases for EXPLAIN (see below) as well as extracting plan dependencies in
setref.c so that we force rebuilding of the plan on any change to
partitions.  (We do replanning on FDW table-option changes to foreign
partitions, currently.  See regression tests in the attached patch.)


...this part means we cannot really avoid locking at least the foreign
partitions during the planning stage, which we currently don't, as we
don't look at partitions at all before ExecSetupPartitionTupleRouting() is
called by ExecInitModifyTable().


Another case where we need inheritance expansion during the planning 
stage would probably be INSERT .. ON CONFLICT into a partitioned table, 
I guess.  We don't support that yet, but if implementing that, I guess 
we would probably need to look at each partition and do something like 
infer_arbiter_indexes() for each partition during the planner stage. 
Not sure.



Since we are locking *all* the partitions anyway, it may be better to
shift the cost of locking to the planner by doing the inheritance
expansion even in the insert case (for partitioned tables) and avoid
calling the lock manager again in the executor when setting up
tuple-routing data structures (that is, in ExecSetupPartitionTupleRouting).


We need the execution-time lock, anyway.  See the comments from Robert 
in [3].



An idea that Robert recently mentioned on the nearby "UPDATE partition
key" thread [2] seems relevant in this regard.  By that idea,
expand_inherited_rtentry(), instead of reading in the partition OIDs in
the old catalog order, will instead call
RelationBuildPartitionDispatchInfo(), which locks the partitions and
returns their OIDs in the canonical order.  If we store the foreign
partition RT indexes in that order in ModifyTable.partition_rels
introduced by this patch, then the following code added by the patch could
be made more efficient (as also explained in aforementioned Robert's email):

+foreach(l, node->partition_rels)
+{
+Index   rti = lfirst_int(l);
+Oid relid = getrelid(rti, estate->es_range_table);
+boolfound;
+int j;
+
+/* First, find the ResultRelInfo for the partition */
+found = false;
+for (j = 0; j < mtstate->mt_num_partitions; j++)
+{
+resultRelInfo = partitions + j;
+
+if (RelationGetRelid(resultRelInfo->ri_RelationDesc) ==
relid)
+{
+Assert(mtstate->mt_partition_indexes[i] == 0);
+mtstate->mt_partition_indexes[i] = j;
+found = true;
+break;
+}
+}
+if (!found)
+elog(ERROR, "failed to find ResultRelInfo for rangetable
index %u", rti);


Agreed.  I really want to improve this based on that idea.

Thank you for the comments!

Best regards,
Etsuro Fujita

[3] 
https://www.postgresql.org/message-id/ca+tgmoyiwvicdri3zk+quxj1r7umu9t_kdnq+17pcwgzrbz...@mail.gmail.com




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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

On 2017/06/29 20:20, Etsuro Fujita wrote:
> Hi,
> 
> Here is a patch for $subject.  This is based on Amit's original patch
> (patch '0007-Tuple-routing-for-partitioned-tables-15.patch' in [1]).

Thanks a lot for taking this up.

> Main changes are:
> 
> * I like Amit's idea that for each partition that is a foreign table, the
> FDW performs query planning in the same way as in non-partition cases, but
> the changes to make_modifytable() in the original patch that creates a
> working-copy of Query to pass to PlanForeignModify() seem unacceptable. 
> So, I modified the changes so that we create more-valid-looking copies of
> Query and ModifyTable with the foreign partition as target, as I said
> before.

This sounds good.

> In relation to this, I also allowed expand_inherited_rtentry() to
> build an RTE and AppendRelInfo for each partition of a partitioned table
> that is an INSERT target, as mentioned by Amit in [1], by modifying
> transformInsertStmt() so that we set the inh flag for the target table's
> RTE to true when the target table is a partitioned table.

About this part, Robert sounded a little unsure back then [1]; his words:

"Doing more inheritance expansion early is probably not a good idea
because it likely sucks for performance, and that's especially unfortunate
if it's only needed for foreign tables."

But...

> The partition
> RTEs are not only referenced by FDWs, but used in selecting relation
> aliases for EXPLAIN (see below) as well as extracting plan dependencies in
> setref.c so that we force rebuilding of the plan on any change to
> partitions.  (We do replanning on FDW table-option changes to foreign
> partitions, currently.  See regression tests in the attached patch.)

...this part means we cannot really avoid locking at least the foreign
partitions during the planning stage, which we currently don't, as we
don't look at partitions at all before ExecSetupPartitionTupleRouting() is
called by ExecInitModifyTable().

Since we are locking *all* the partitions anyway, it may be better to
shift the cost of locking to the planner by doing the inheritance
expansion even in the insert case (for partitioned tables) and avoid
calling the lock manager again in the executor when setting up
tuple-routing data structures (that is, in ExecSetupPartitionTupleRouting).

An idea that Robert recently mentioned on the nearby "UPDATE partition
key" thread [2] seems relevant in this regard.  By that idea,
expand_inherited_rtentry(), instead of reading in the partition OIDs in
the old catalog order, will instead call
RelationBuildPartitionDispatchInfo(), which locks the partitions and
returns their OIDs in the canonical order.  If we store the foreign
partition RT indexes in that order in ModifyTable.partition_rels
introduced by this patch, then the following code added by the patch could
be made more efficient (as also explained in aforementioned Robert's email):

+foreach(l, node->partition_rels)
+{
+Index   rti = lfirst_int(l);
+Oid relid = getrelid(rti, estate->es_range_table);
+boolfound;
+int j;
+
+/* First, find the ResultRelInfo for the partition */
+found = false;
+for (j = 0; j < mtstate->mt_num_partitions; j++)
+{
+resultRelInfo = partitions + j;
+
+if (RelationGetRelid(resultRelInfo->ri_RelationDesc) ==
relid)
+{
+Assert(mtstate->mt_partition_indexes[i] == 0);
+mtstate->mt_partition_indexes[i] = j;
+found = true;
+break;
+}
+}
+if (!found)
+elog(ERROR, "failed to find ResultRelInfo for rangetable
index %u", rti);


> * The original patch added tuple routing to foreign partitions in COPY
> FROM, but I'm not sure the changes to BeginCopy() in the patch are the
> right way to go.  For example, the patch passes a dummy empty Query to
> PlanForeignModify() to make FDWs perform query planning, but I think FDWs
> would be surprised by the Query.  Currently, we don't support COPY to
> foreign tables, so ISTM that it's better to revisit this issue when adding
> support for COPY to foreign tables.  So, I dropped the COPY part.

I agree.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoYvp6DD1jpwb9sNe08E7jSFEFky32TJU%2BsJ8OStHYW3nA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CA%2BTgmoajC0J50%3D2FqnZLvB10roY%2B68HgFWhso%3DV_StkC6PWujQ%40mail.gmail.com



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

On Mon, Jun 19, 2017 at 12:34 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> On 2017/06/16 14:16, Ashutosh Bapat wrote:
> > On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas 
> wrote:
> >> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
> >>  wrote:
> >>> Some more comments on the latest set of patches.
> >> or looking up the OID in the
> >> relcache multiple times.
> >
> > I am not able to understand this in the context of default partition.
> > After that nobody else is going to change its partitions and their
> > bounds (since both of those require heap_open on parent which would be
> > stuck on the lock we hold.). So, we have to check only once if the
> > table has a default partition. If it doesn't, it's not going to
> > acquire one unless we release the lock on the parent i.e at the end of
> > transaction. If it has one, it's not going to get dropped till the end
> > of the transaction for the same reason. I don't see where we are
> > looking up OIDs multiple times.
>
> Without heap_opening the parent, the only way is to look up parentOid's
> children in pg_inherits and for each child looking up its pg_class tuple
> in the syscache to see if its relpartbound indicates that it's a default
> partition.  That seems like it won't be inexpensive either.
>
> It would be nice if could get that information (that is - is a given
> relation being heap_drop_with_catalog'd a partition of the parent that
> happens to have default partition) in less number of steps than that.
> Having that information in relcache is one way, but as mentioned, that
> turns out be expensive.
>
> Has anyone considered the idea of putting the default partition OID in the
> pg_partitioned_table catalog?  Looking the above information up would
> amount to one syscache lookup.  Default partition seems to be special
> enough object to receive a place in the pg_partitioned_table tuple of the
> parent.  Thoughts?
>

I liked this suggestion. Having an entry in pg_partitioned_table would avoid
both expensive methods, i.e. 1. opening the parent or 2. lookup for
each of the children first in pg_inherits and then its corresponding entry
in
pg_class.
Unless anybody has any other suggestions/comments here, I am going to
implement this suggestion.

Thanks,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 21, 2017 at 8:47 PM, Amit Langote
 wrote:
> It's the comma inside the error message that suggests to me that it's a
> style that I haven't seen elsewhere in the backend code.

Exactly.  Avoid that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/21 21:37, Jeevan Ladhe wrote:
> Hi Amit,
> 
> On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp> wrote:
> 
>> Oops, I meant to send one more comment.
>>
>> On 2017/06/15 15:48, Amit Langote wrote:
>>> BTW, I noticed the following in 0002
>> +errmsg("there exists a default
>> partition for table \"%s\", cannot
>> add a new partition",
>>
>> This error message style seems novel to me.  I'm not sure about the best
>> message text here, but maybe: "cannot add new partition to table \"%s\"
>> with default partition"
>>
> 
> This sounds confusing to me, what about something like:
> "\"%s\" has a default partition, cannot add a new partition."

It's the comma inside the error message that suggests to me that it's a
style that I haven't seen elsewhere in the backend code.  The primary
error message here is that the new partition cannot be created.  "%s has
default partition" seems to me to belong in errdetail() (see "What Goes
Where" in [1].)

Or write the sentence such that the comma is not required.  Anyway, we can
leave this for the committer to decide.

> Note that this comment belongs to patch 0002, and it will go away
> in case we are going to have extended functionality i.e. patch 0003,
> as in that patch we allow user to create a new partition even in the
> cases when there exists a default partition.

Oh, that'd be great.  It's always better to get rid of the error
conditions that are hard to communicate to users. :)  (Although, this
one's not that ambiguous.)

Thanks,
Amit

[1] https://www.postgresql.org/docs/devel/static/error-style-guide.html



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Thanks Ashutosh and Kyotaro for reviewing further.
I shall address your comments in next version of my patch.

Regards,
Jeevan Ladhe

On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, I'd like to review this but it doesn't fit the master, as
> Robert said. Especially the interface of predicate_implied_by is
> changed by the suggested commit.
>
> Anyway I have some comment on this patch with fresh eyes.  I
> believe the basic design so my comment below are from a rather
> micro viewpoint.
>
> At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote <
> langote_amit...@lab.ntt.co.jp> wrote in  34ff801bf...@lab.ntt.co.jp>
> > Oops, I meant to send one more comment.
> >
> > On 2017/06/15 15:48, Amit Langote wrote:
> > > BTW, I noticed the following in 0002
> > +  errmsg("there exists a default
> partition for table \"%s\", cannot
> > add a new partition",
> >
> > This error message style seems novel to me.  I'm not sure about the best
> > message text here, but maybe: "cannot add new partition to table \"%s\"
> > with default partition"
> >
> > Note that the comment applies to both DefineRelation and
> > ATExecAttachPartition.
>
> - Considering on how canSkipPartConstraintValidation is called, I
>   *think* that RelationProvenValid() would be better.  (But this
>   would be disappear by rebasing..)
>
> - 0002 changes the interface of get_qual_for_list, but left
>   get_qual_for_range alone.  Anyway get_qual_for_range will have
>   to do the similar thing soon.
>
> - In check_new_partition_bound, "overlap" and "with" is
>   completely correlated with each other. "with > -1" means
>   "overlap = true". So overlap is not useless. ("with" would be
>   better to be "overlap_with" or somehting if we remove
>   "overlap")
>
> - The error message of check_default_allows_bound is below.
>
>   "updated partition constraint for default partition \"%s\"
>would be violated by some row"
>
>   This looks an analog of validateCheckConstraint but as my
>   understanding this function is called only when new partition
>   is added. This would be difficult to recognize in the
>   situation.
>
>   "the default partition contains rows that should be in
>the new partition: \"%s\""
>
>   or something?
>
> - In check_default_allows_bound, the iteration over partitions is
>   quite similar to what validateCheckConstraint does. Can we
>   somehow share validateCheckConstraint with this function?
>
> - In the same function, skipping RELKIND_PARTITIONED_TABLE is
>   okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
>   good. I think at least some warning should be emitted.
>
>   "Skipping foreign tables in the defalut partition. It might
>contain rows that should be in the new partition."  (Needs
>preventing multple warnings in single call, maybe)
>
> - In the same function, the following condition seems somewhat
>   strange in comparison to validateCheckConstraint.
>
> > if (partqualstate && ExecCheck(partqualstate, econtext))
>
>   partqualstate won't be null as long as partition_constraint is
>   valid. Anyway (I'm believing that) an invalid constraint
>   results in error by ExecPrepareExpr. Therefore 'if
>   (partqualstate' is useless.
>
> - In gram.y, the nonterminal for list spec clause is still
>   "ForValues". It seems somewhat strange. partition_spec or
>   something would be better.
>
> - This is not a part of this patch, but in ruleutils.c, the error
>   for unknown paritioning strategy is emitted as following.
>
> >   elog(ERROR, "unrecognized partition strategy: %d",
> >(int) strategy);
>
>   The cast is added because the strategy is a char. I suppose
>   this is because strategy can be an unprintable. I'd like to see
>   a comment if it is correct.
>
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Amit,

On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> Oops, I meant to send one more comment.
>
> On 2017/06/15 15:48, Amit Langote wrote:
> > BTW, I noticed the following in 0002
> +errmsg("there exists a default
> partition for table \"%s\", cannot
> add a new partition",
>
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
>

This sounds confusing to me, what about something like:
"\"%s\" has a default partition, cannot add a new partition."

Note that this comment belongs to patch 0002, and it will go away
in case we are going to have extended functionality i.e. patch 0003,
as in that patch we allow user to create a new partition even in the
cases when there exists a default partition.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

Sorry for being away from here.
I had some issues with my laptop, and I have resumed working on this.

On Thu, Jun 15, 2017 at 1:21 AM, Robert Haas  wrote:

> On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
>  wrote:
> > Here are the details of the patches in attached zip.
> > 0001. refactoring existing ATExecAttachPartition  code so that it can be
> > used for
> > default partitioning as well
> > 0002. support for default partition with the restriction of preventing
> > addition
> > of any new partition after default partition.
> > 0003. extend default partitioning support to allow addition of new
> > partitions.
> > 0004. extend default partitioning validation code to reuse the refactored
> > code
> > in patch 0001.
>
> I think the core ideas of this patch are pretty solid now.  It's come
> a long way in the last month.  High-level comments:
>

Thanks Robert for looking into this.


> - Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
>

Will rebase.


> - Still no documentation.
> - Should probably be merged with the patch to add default partitioning
> for ranges.
>
Will try to get this soon.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/16 14:16, Ashutosh Bapat wrote:
> On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas  wrote:
>> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
>>  wrote:
>>> Some more comments on the latest set of patches.
>>>
>>> In heap_drop_with_catalog(), we heap_open() the parent table to get the
>>> default partition OID, if any. If the relcache doesn't have an entry for the
>>> parent, this means that the entry will be created, only to be invalidated at
>>> the end of the function. If there is no default partition, this all is
>>> completely unnecessary. We should avoid heap_open() in this case. This also
>>> means that get_default_partition_oid() should not rely on the relcache 
>>> entry,
>>> but should growl through pg_inherit to find the default partition.
>>
>> I am *entirely* unconvinced by this line of argument.  I think we want
>> to open the relation the first time we touch it and pass the Relation
>> around thereafter.
> 
> If this would be correct, why heap_drop_with_catalog() without this
> patch just locks the parent and doesn't call a heap_open(). I am
> missing something.

As of commit c1e0e7e1d790bf, we avoid creating relcache entry for the
parent.  Before that commit, drop table
partitioned_table_with_many_partitions used to take too long and consumed
quite some memory as result of relcache invalidation requested at the end
on the parent table for every partition.

If this patch reintroduces the heap_open() on the parent table, that's
going to bring back the problem fixed by that commit.

>> Anything else is prone to accidentally failing to
>> have the relation locked early enough,
> 
> We are locking the parent relation even without this patch, so this
> isn't an issue.

Yes.

>> or looking up the OID in the
>> relcache multiple times.
> 
> I am not able to understand this in the context of default partition.
> After that nobody else is going to change its partitions and their
> bounds (since both of those require heap_open on parent which would be
> stuck on the lock we hold.). So, we have to check only once if the
> table has a default partition. If it doesn't, it's not going to
> acquire one unless we release the lock on the parent i.e at the end of
> transaction. If it has one, it's not going to get dropped till the end
> of the transaction for the same reason. I don't see where we are
> looking up OIDs multiple times.

Without heap_opening the parent, the only way is to look up parentOid's
children in pg_inherits and for each child looking up its pg_class tuple
in the syscache to see if its relpartbound indicates that it's a default
partition.  That seems like it won't be inexpensive either.

It would be nice if could get that information (that is - is a given
relation being heap_drop_with_catalog'd a partition of the parent that
happens to have default partition) in less number of steps than that.
Having that information in relcache is one way, but as mentioned, that
turns out be expensive.

Has anyone considered the idea of putting the default partition OID in the
pg_partitioned_table catalog?  Looking the above information up would
amount to one syscache lookup.  Default partition seems to be special
enough object to receive a place in the pg_partitioned_table tuple of the
parent.  Thoughts?

>>> +defaultPartOid = get_default_partition_oid(rel);
>>> +if (OidIsValid(defaultPartOid))
>>> +ereport(ERROR,
>>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>>> + errmsg("there exists a default partition for table
>>> \"%s\", cannot attach a new partition",
>>> +RelationGetRelationName(rel;
>>> +
>>> Should be done before heap_open on the table being attached. If we are not
>>> going to attach the partition, there's no point in instantiating its 
>>> relcache.
>>
>> No, because we should take the lock before examining any properties of
>> the table.
> 
> There are three tables involved here. "rel" which is the partitioned
> table. "attachrel" is the table being attached as a partition to "rel"
> and defaultrel, which is the default partition table. If there exists
> a default partition in "rel" we are not allowing "attachrel" to be
> attached to "rel". If that's the case, we don't need to examine any
> properties of "attachrel" and hence we don't need to instantiate
> relcache of "attachrel". That's what the comment is about.
> ATExecAttachPartition() receives "rel" as an argument which has been
> already locked and opened. So, we can check the existence of default
> partition right at the beginning of the function.

It seems that we are examining the properties of the parent table here
(whether it has default partition), which as Ashutosh mentions, is already
locked before we got to ATExecAttachPartition().  Another place where we
are ereporting before locking the table to be attached (actually even
before looking it up by name), based just on the properties 

Re: [HACKERS] Adding support for Default partition in partitioning

[Kyotaro HORIGUCHI]

Hello, I'd like to review this but it doesn't fit the master, as
Robert said. Especially the interface of predicate_implied_by is
changed by the suggested commit.

Anyway I have some comment on this patch with fresh eyes.  I
believe the basic design so my comment below are from a rather
micro viewpoint.

At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote 
 wrote in 

> Oops, I meant to send one more comment.
> 
> On 2017/06/15 15:48, Amit Langote wrote:
> > BTW, I noticed the following in 0002
> +  errmsg("there exists a default 
> partition for table \"%s\", cannot
> add a new partition",
> 
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
> 
> Note that the comment applies to both DefineRelation and
> ATExecAttachPartition.

- Considering on how canSkipPartConstraintValidation is called, I
  *think* that RelationProvenValid() would be better.  (But this
  would be disappear by rebasing..)

- 0002 changes the interface of get_qual_for_list, but left
  get_qual_for_range alone.  Anyway get_qual_for_range will have
  to do the similar thing soon.

- In check_new_partition_bound, "overlap" and "with" is
  completely correlated with each other. "with > -1" means
  "overlap = true". So overlap is not useless. ("with" would be
  better to be "overlap_with" or somehting if we remove
  "overlap")

- The error message of check_default_allows_bound is below.

  "updated partition constraint for default partition \"%s\"
   would be violated by some row"

  This looks an analog of validateCheckConstraint but as my
  understanding this function is called only when new partition
  is added. This would be difficult to recognize in the
  situation.

  "the default partition contains rows that should be in
   the new partition: \"%s\""

  or something?

- In check_default_allows_bound, the iteration over partitions is
  quite similar to what validateCheckConstraint does. Can we
  somehow share validateCheckConstraint with this function?

- In the same function, skipping RELKIND_PARTITIONED_TABLE is
  okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
  good. I think at least some warning should be emitted.

  "Skipping foreign tables in the defalut partition. It might
   contain rows that should be in the new partition."  (Needs
   preventing multple warnings in single call, maybe)

- In the same function, the following condition seems somewhat
  strange in comparison to validateCheckConstraint.

> if (partqualstate && ExecCheck(partqualstate, econtext))

  partqualstate won't be null as long as partition_constraint is
  valid. Anyway (I'm believing that) an invalid constraint
  results in error by ExecPrepareExpr. Therefore 'if
  (partqualstate' is useless.

- In gram.y, the nonterminal for list spec clause is still
  "ForValues". It seems somewhat strange. partition_spec or
  something would be better.

- This is not a part of this patch, but in ruleutils.c, the error
  for unknown paritioning strategy is emitted as following.

>   elog(ERROR, "unrecognized partition strategy: %d",
>(int) strategy);

  The cast is added because the strategy is a char. I suppose
  this is because strategy can be an unprintable. I'd like to see
  a comment if it is correct.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas  wrote:
> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
>  wrote:
>> Some more comments on the latest set of patches.
>>
>> In heap_drop_with_catalog(), we heap_open() the parent table to get the
>> default partition OID, if any. If the relcache doesn't have an entry for the
>> parent, this means that the entry will be created, only to be invalidated at
>> the end of the function. If there is no default partition, this all is
>> completely unnecessary. We should avoid heap_open() in this case. This also
>> means that get_default_partition_oid() should not rely on the relcache entry,
>> but should growl through pg_inherit to find the default partition.
>
> I am *entirely* unconvinced by this line of argument.  I think we want
> to open the relation the first time we touch it and pass the Relation
> around thereafter.

If this would be correct, why heap_drop_with_catalog() without this
patch just locks the parent and doesn't call a heap_open(). I am
missing something.

> Anything else is prone to accidentally failing to
> have the relation locked early enough,

We are locking the parent relation even without this patch, so this
isn't an issue.

> or looking up the OID in the
> relcache multiple times.

I am not able to understand this in the context of default partition.
After that nobody else is going to change its partitions and their
bounds (since both of those require heap_open on parent which would be
stuck on the lock we hold.). So, we have to check only once if the
table has a default partition. If it doesn't, it's not going to
acquire one unless we release the lock on the parent i.e at the end of
transaction. If it has one, it's not going to get dropped till the end
of the transaction for the same reason. I don't see where we are
looking up OIDs multiple times.


>
>> +defaultPartOid = get_default_partition_oid(rel);
>> +if (OidIsValid(defaultPartOid))
>> +ereport(ERROR,
>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("there exists a default partition for table
>> \"%s\", cannot attach a new partition",
>> +RelationGetRelationName(rel;
>> +
>> Should be done before heap_open on the table being attached. If we are not
>> going to attach the partition, there's no point in instantiating its 
>> relcache.
>
> No, because we should take the lock before examining any properties of
> the table.

There are three tables involved here. "rel" which is the partitioned
table. "attachrel" is the table being attached as a partition to "rel"
and defaultrel, which is the default partition table. If there exists
a default partition in "rel" we are not allowing "attachrel" to be
attached to "rel". If that's the case, we don't need to examine any
properties of "attachrel" and hence we don't need to instantiate
relcache of "attachrel". That's what the comment is about.
ATExecAttachPartition() receives "rel" as an argument which has been
already locked and opened. So, we can check the existence of default
partition right at the beginning of the function.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
 wrote:
> Some more comments on the latest set of patches.
>
> In heap_drop_with_catalog(), we heap_open() the parent table to get the
> default partition OID, if any. If the relcache doesn't have an entry for the
> parent, this means that the entry will be created, only to be invalidated at
> the end of the function. If there is no default partition, this all is
> completely unnecessary. We should avoid heap_open() in this case. This also
> means that get_default_partition_oid() should not rely on the relcache entry,
> but should growl through pg_inherit to find the default partition.

I am *entirely* unconvinced by this line of argument.  I think we want
to open the relation the first time we touch it and pass the Relation
around thereafter.  Anything else is prone to accidentally failing to
have the relation locked early enough, or looking up the OID in the
relcache multiple times.

> In get_qual_for_list(), if the table has only default partition, it won't have
> any boundinfo. In such a case the default partition's constraint would come 
> out
> as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty array
> looks odd and may be we spend a few CPU cycles executing ANY on an empty 
> array.
> We have the same problem with a partition containing only NULL value. So, may
> be this one is not that bad.

I think that one is probably worth fixing.

> Please add a testcase to test addition of default partition as the first
> partition.

That seems like a good idea, too.

> get_qual_for_list() allocates the constant expressions corresponding to the
> datums in CacheMemoryContext while constructing constraints for a default
> partition. We do not do this for other partitions. We may not be constructing
> the constraints for saving in the cache. For example, ATExecAttachPartition
> constructs the constraints for validation. In such a case, this code will
> unnecessarily clobber the cache memory. generate_partition_qual() copies the
> partition constraint in the CacheMemoryContext.
>
> +   if (spec->is_default)
> +   {
> +   result = list_make1(make_ands_explicit(result));
> +   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
> +   }

Clearly we do not want things to end up across multiple contexts.  We
should ensure that anything linked from the relcache entry ends up in
CacheMemoryContext, but we must be careful not to allocate anything
else in there, because CacheMemoryContext is never reset.

> If the "result" is an OR expression, calling make_ands_explicit() on it would
> create AND(OR(...)) expression, with an unnecessary AND. We want to avoid 
> that?

I'm not sure it's worth the trouble.

> +defaultPartOid = get_default_partition_oid(rel);
> +if (OidIsValid(defaultPartOid))
> +ereport(ERROR,
> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("there exists a default partition for table
> \"%s\", cannot attach a new partition",
> +RelationGetRelationName(rel;
> +
> Should be done before heap_open on the table being attached. If we are not
> going to attach the partition, there's no point in instantiating its relcache.

No, because we should take the lock before examining any properties of
the table.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

Some more comments on the latest set of patches.

In heap_drop_with_catalog(), we heap_open() the parent table to get the
default partition OID, if any. If the relcache doesn't have an entry for the
parent, this means that the entry will be created, only to be invalidated at
the end of the function. If there is no default partition, this all is
completely unnecessary. We should avoid heap_open() in this case. This also
means that get_default_partition_oid() should not rely on the relcache entry,
but should growl through pg_inherit to find the default partition.

In get_qual_for_list(), if the table has only default partition, it won't have
any boundinfo. In such a case the default partition's constraint would come out
as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty array
looks odd and may be we spend a few CPU cycles executing ANY on an empty array.
We have the same problem with a partition containing only NULL value. So, may
be this one is not that bad.

Please add a testcase to test addition of default partition as the first
partition.

get_qual_for_list() allocates the constant expressions corresponding to the
datums in CacheMemoryContext while constructing constraints for a default
partition. We do not do this for other partitions. We may not be constructing
the constraints for saving in the cache. For example, ATExecAttachPartition
constructs the constraints for validation. In such a case, this code will
unnecessarily clobber the cache memory. generate_partition_qual() copies the
partition constraint in the CacheMemoryContext.

+   if (spec->is_default)
+   {
+   result = list_make1(make_ands_explicit(result));
+   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
+   }

If the "result" is an OR expression, calling make_ands_explicit() on it would
create AND(OR(...)) expression, with an unnecessary AND. We want to avoid that?

+   if (cur_index < 0 && (partition_bound_has_default(partdesc->boundinfo)))
+   cur_index = partdesc->boundinfo->default_index;
+
The partition_bound_has_default() check is unnecessary since we check for
cur_index < 0 next anyway.

+ *
+ * Given the parent relation checks if it has default partition, and if it
+ * does exist returns its oid, otherwise returns InvalidOid.
+ */
May be reworded as "If the given relation has a default partition, this
function returns the OID of the default partition. Otherwise it returns
InvalidOid."

+Oid
+get_default_partition_oid(Relation parent)
+{
+   PartitionDesc partdesc = RelationGetPartitionDesc(parent);
+
+   if (partdesc->boundinfo && partition_bound_has_default(partdesc->boundinfo))
+   return partdesc->oids[partdesc->boundinfo->default_index];
+
+   return InvalidOid;
+}
An unpartitioned table would not have partdesc set set. So, this function will
segfault if we pass an unpartitioned table. Either Assert that partdesc should
exist or check for its NULL-ness.


+defaultPartOid = get_default_partition_oid(rel);
+if (OidIsValid(defaultPartOid))
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("there exists a default partition for table
\"%s\", cannot attach a new partition",
+RelationGetRelationName(rel;
+
Should be done before heap_open on the table being attached. If we are not
going to attach the partition, there's no point in instantiating its relcache.

The comment in heap_drop_with_catalog() should mention why we lock the default
partition before locking the table being dropped.

 extern List *preprune_pg_partitions(PlannerInfo *root, RangeTblEntry *rte,
Index rti, Node *quals, LOCKMODE lockmode);
-
 #endif   /* PARTITION_H */
Unnecessary hunk.

On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote
 wrote:
> Oops, I meant to send one more comment.
>
> On 2017/06/15 15:48, Amit Langote wrote:
>> BTW, I noticed the following in 0002
> +errmsg("there exists a default 
> partition for table \"%s\", cannot
> add a new partition",
>
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
>
> Note that the comment applies to both DefineRelation and
> ATExecAttachPartition.
>
> Thanks,
> Amit
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

Oops, I meant to send one more comment.

On 2017/06/15 15:48, Amit Langote wrote:
> BTW, I noticed the following in 0002
+errmsg("there exists a default 
partition for table \"%s\", cannot
add a new partition",

This error message style seems novel to me.  I'm not sure about the best
message text here, but maybe: "cannot add new partition to table \"%s\"
with default partition"

Note that the comment applies to both DefineRelation and
ATExecAttachPartition.

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/15 4:51, Robert Haas wrote:
> On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
>  wrote:
>> Here are the details of the patches in attached zip.
>> 0001. refactoring existing ATExecAttachPartition  code so that it can be
>> used for
>> default partitioning as well
>> 0002. support for default partition with the restriction of preventing
>> addition
>> of any new partition after default partition.
>> 0003. extend default partitioning support to allow addition of new
>> partitions.
>> 0004. extend default partitioning validation code to reuse the refactored
>> code
>> in patch 0001.
> 
> I think the core ideas of this patch are pretty solid now.  It's come
> a long way in the last month.

+1


BTW, I noticed the following in 0002:

@@ -1322,15 +1357,59 @@ get_qual_for_list(PartitionKey key,
PartitionBoundSpec *spec)

[ ... ]

+   oldcxt = MemoryContextSwitchTo(CacheMemoryContext);

I'm not sure if we need to do that.  Can you explain?

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
 wrote:
> Here are the details of the patches in attached zip.
> 0001. refactoring existing ATExecAttachPartition  code so that it can be
> used for
> default partitioning as well
> 0002. support for default partition with the restriction of preventing
> addition
> of any new partition after default partition.
> 0003. extend default partitioning support to allow addition of new
> partitions.
> 0004. extend default partitioning validation code to reuse the refactored
> code
> in patch 0001.

I think the core ideas of this patch are pretty solid now.  It's come
a long way in the last month.  High-level comments:

- Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
- Still no documentation.
- Should probably be merged with the patch to add default partitioning
for ranges.

Other stuff I noticed:

- The regression tests don't seem to check that the scan-skipping
logic works as expected.  We have regression tests for that case for
attaching regular partitions, and it seems like it would be worth
testing the default-partition case as well.

- check_default_allows_bound() assumes that if
canSkipPartConstraintValidation() fails for the default partition, it
will also fail for every subpartition of the default partition.  That
is, once we commit to scanning one child partition, we're committed to
scanning them all.  In practice, that's probably not a huge
limitation, but if it's not too much code, we could keep the top-level
check but also check each partitioning individually as we reach it,
and skip the scan for any individual partitions for which the
constraint can be proven.  For example, suppose the top-level table is
list-partitioned with a partition for each of the most common values,
and then we range-partition the default partition.

- The changes to the regression test results in 0004 make the error
messages slightly worse.  The old message names the default partition,
whereas the new one does not.  Maybe that's worth avoiding.

Specific comments:

+ * Also, invalidate the parent's and a sibling default partition's relcache,
+ * so that the next rebuild will load the new partition's info into parent's
+ * partition descriptor and default partition constraints(which are dependent
+ * on other partition bounds) are built anew.

I find this a bit unclear, and it also repeats the comment further
down.  Maybe something like: Also, invalidate the parent's relcache
entry, so that the next rebuild will load he new partition's info into
its partition descriptor.  If there is a default partition, we must
invalidate its relcache entry as well.

+/*
+ * The default partition constraints depend upon the partition bounds of
+ * other partitions. Adding a new(or even removing existing) partition
+ * would invalidate the default partition constraints. Invalidate the
+ * default partition's relcache so that the constraints are built anew and
+ * any plans dependent on those constraints are invalidated as well.
+ */

Here, I'd write: The partition constraint for the default partition
depends on the partition bounds of every other partition, so we must
invalidate the relcache entry for that partition every time a
partition is added or removed.

+/*
+ * Default partition cannot be added if there already
+ * exists one.
+ */
+if (spec->is_default)
+{
+overlap = partition_bound_has_default(boundinfo);
+with = boundinfo->default_index;
+break;
+}

To support default partitioning for range, this is going to have to be
moved above the switch rather than done inside of it.  And there's
really no downside to putting it there.

+ * constraint, by *proving* that the existing constraints of the table
+ * *imply* the given constraints.  We include the table's check constraints and

Both the comma and the asterisks are unnecessary.

+ * Check whether all rows in the given table (scanRel) obey given partition

obey the given

I think the larger comment block could be tightened up a bit, like
this:  Check whether all rows in the given table obey the given
partition constraint; if so, it can be attached as a partition.  We do
this by scanning the table (or all of its leaf partitions) row by row,
except when the existing constraints are sufficient to prove that the
new partitioning constraint must already hold.

+/* Check if we can do away with having to scan the table being attached. */

If possible, skip the validation scan.

+ * Set up to have the table be scanned to validate the partition
+ * constraint If it's a partitioned table, we instead schedule its leaf
+ * partitions to be scanned.

I suggest: Prepare to scan the default partition (or, if it is itself
partitioned, all of its leaf 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

While rebasing the current set of patches to the latest source, I realized
that it might be a good idea to split the default partitioning support
patch further
into two incremental patches, where the first patch for default partition
support would prevent addition of any new partition if there exists a
default
partition, and then an incremental patch which allows to create/attach a
new partition even if there exists a default partition provided the default
partition does not have any rows satisfying the bounds of the new partition
being added. This would be easier for review.

Here are the details of the patches in attached zip.
0001. refactoring existing ATExecAttachPartition  code so that it can be
used for
default partitioning as well
0002. support for default partition with the restriction of preventing
addition
of any new partition after default partition.
0003. extend default partitioning support to allow addition of new
partitions.
0004. extend default partitioning validation code to reuse the refactored
code
in patch 0001.

PFA

Regards,
Jeevan Ladhe

On Mon, Jun 12, 2017 at 11:49 AM, Jeevan Ladhe <
jeevan.la...@enterprisedb.com> wrote:

>
> On Mon, Jun 12, 2017 at 9:39 AM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> While the refactoring seems a reasonable way to re-use existing code,
>> that may change based on the discussion in [1]. Till then please keep
>> the refactoring patches separate from the main patch. In the final
>> version, I think the refactoring changes to ATAttachPartition and the
>> default partition support should be committed separately. So, please
>> provide three different patches. That also makes review easy.
>>
>
> Sure Ashutosh,
>
> PFA.
>


default_partition_splits_v21.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] ICU support on Windows

[Ashutosh Sharma]

On Tue, Jun 13, 2017 at 6:45 PM, Peter Eisentraut
 wrote:
> On 6/12/17 14:03, Ashutosh Sharma wrote:
>>> I noticed that this only works if you use the "Win32" download of ICU,
>>> because the "Win64" download uses "lib64" paths.  I'm not sure what the
>>> impact of this is in practice.
>>
>> Yes, that's right, Win64 download uses lib64 path and in my case i had
>> renamed lib64-> lib and bin64-> bin which i guess is not a right thing
>> to do. I think, we should allow Solution.pm to detect the platform and
>> make a decision on the library path accordingly. Attached patch does
>> that. Please have a look let me know your thoughts on this. Thanks.
>
> committed

Thank you :)

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


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