[HACKERS] Proposal to add work_mem option to postgres_fdw module

Started by Shinoda, Noriyoshi (PN Japan GCS Delivery)over 7 years ago17 messages
1 attachment(s)

Hello hackers,

The attached patch adds a new option work_mem to postgres_fdw contrib module.

Previously, it was impossible to change the setting of work_mem for remote session with connection by postgres_fdw.

By adding this option to the CREATE SERVER statement, you can also change the work_mem setting for remote sessions with postgres_fdw.

Usage is the same as other options, and specify work_mem as an option of the CREATE SERVER statement. The string you specify is the same as the format of work_mem in GUC.

Example

----------

postgres=# CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', work_mem '16MB');

CREATE SERVER

Not only CREATE SERVER statement but ALTER SERVER statement are also available.

postgres=# ALTER SERVER remsvr1 OPTIONS ( SET work_mem '32MB');

ALTER SERVER

postgres=# SELECT * FROM pg_foreign_server;

-[ RECORD 1 ]----------------------------------------------------------

srvname | remsvr1

srvowner | 10

srvfdw | 16389

srvtype |

srvversion |

srvacl |

srvoptions | {host=remhost1,port=5433,dbname=demodb,work_mem=32MB}

Within this patch, I implemented a change in GUC work_mem by specifying "options -c work_mem=value" for the PQconnectdbParams API function.

Best Regards,

Noriyoshi Shinoda

Attachments:

postgres_fdw_work_mem_v1.patchapplication/octet-stream; name=postgres_fdw_work_mem_v1.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d912bd9..c529a00 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8607,3 +8607,28 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+ count 
+-------
+     1
+(1 row)
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+ count 
+-------
+     1
+(1 row)
+
+ROLLBACK;
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 6854f1b..aa28af4 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -22,7 +22,7 @@
 #include "commands/extension.h"
 #include "utils/builtins.h"
 #include "utils/varlena.h"
-
+#include "utils/guc.h"
 
 /*
  * Describes the valid options for objects that this wrapper uses.
@@ -143,6 +143,19 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("%s requires a non-negative integer value",
 								def->defname)));
 		}
+		else if (strcmp(def->defname, "work_mem") == 0)
+		{
+			int work_mem = 0;
+			bool result;
+			result = parse_int(defGetString(def), &work_mem, GUC_UNIT_KB, NULL);
+                        if ((!result) || ((work_mem < 64) || (work_mem > MAX_KILOBYTES)))
+			{
+				ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("invalid value for option '%s'",
+							def->defname)));
+			} 
+		}
 	}
 
 	PG_RETURN_VOID();
@@ -177,6 +190,8 @@ InitPgFdwOptions(void)
 		/* fetch_size is available on both server and table */
 		{"fetch_size", ForeignServerRelationId, false},
 		{"fetch_size", ForeignTableRelationId, false},
+		/* work_mem */
+		{"work_mem", ForeignServerRelationId, true},
 		{NULL, InvalidOid, false}
 	};
 
@@ -299,6 +314,7 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
 {
 	ListCell   *lc;
 	int			i;
+	StringInfoData buf;
 
 	/* Build our options lists if we didn't yet. */
 	InitPgFdwOptions();
@@ -310,8 +326,16 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
 
 		if (is_libpq_option(d->defname))
 		{
-			keywords[i] = d->defname;
-			values[i] = defGetString(d);
+			if (strcmp(d->defname, "work_mem") == 0)
+			{
+				keywords[i] = pstrdup("options");
+				initStringInfo(&buf);
+				appendStringInfo(&buf, "-c work_mem=%s", defGetString(d));
+				values[i] = buf.data;
+			} else {
+				keywords[i] = d->defname;
+				values[i] = defGetString(d);
+			}
 			i++;
 		}
 	}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index c0b0dd9..c450eca 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2348,3 +2348,24 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+
+ROLLBACK;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 54b5e98..50a508e 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -313,6 +313,15 @@
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>work_mem</literal></term>
+     <listitem>
+      <para>
+       This option specifies the size of working memory using <xref linkend="guc-work-mem"/>.
+       The default value is depend on th remote instance setting.
+      </para>
+     </listitem>
+    </varlistentry>  
    </variablelist>
 
   </sect3>
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#1)
1 attachment(s)
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Hi, Hackers.

I updated the patch that I attached the other day.
Added release of dynamically allocated memory and modified source according to coding rule.

Regards,
Noriyoshi Shinoda
--
From: Shinoda, Noriyoshi (PN Japan GCS Delivery)
Sent: Friday, August 17, 2018 3:07 PM
To: pgsql-hackers@postgresql.org
Subject: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Hello hackers,

The attached patch adds a new option work_mem to postgres_fdw contrib module.
Previously, it was impossible to change the setting of work_mem for remote session with connection by postgres_fdw.
By adding this option to the CREATE SERVER statement, you can also change the work_mem setting for remote sessions with postgres_fdw.

Usage is the same as other options, and specify work_mem as an option of the CREATE SERVER statement. The string you specify is the same as the format of work_mem in GUC.

Example
----------
postgres=# CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', work_mem '16MB');
CREATE SERVER

Not only CREATE SERVER statement but ALTER SERVER statement are also available.

postgres=# ALTER SERVER remsvr1 OPTIONS ( SET work_mem '32MB');
ALTER SERVER
postgres=# SELECT * FROM pg_foreign_server;
-[ RECORD 1 ]----------------------------------------------------------
srvname    | remsvr1
srvowner   | 10
srvfdw     | 16389
srvtype    |
srvversion |
srvacl     |
srvoptions | {host=remhost1,port=5433,dbname=demodb,work_mem=32MB}

Within this patch, I implemented a change in GUC work_mem by specifying "options -c work_mem=value" for the PQconnectdbParams API function.

Best Regards,
Noriyoshi Shinoda

Attachments:

postgres_fdw_work_mem_v2.patchapplication/octet-stream; name=postgres_fdw_work_mem_v2.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index fe4893a..885345e 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -286,6 +286,16 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 		/* Prepare new session for use */
 		configure_remote_session(conn);
 
+		/* Free memory for "work_mem" parameter */
+		for (int i = 0; i < n; i++)
+		{
+			if (strcmp(keywords[i], "options") == 0)
+			{
+				pfree((void *)(values[i]));
+				break;
+			}
+		}	
+
 		pfree(keywords);
 		pfree(values);
 	}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d912bd9..c529a00 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8607,3 +8607,28 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+ count 
+-------
+     1
+(1 row)
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+ count 
+-------
+     1
+(1 row)
+
+ROLLBACK;
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 6854f1b..f45a50a 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -22,7 +22,7 @@
 #include "commands/extension.h"
 #include "utils/builtins.h"
 #include "utils/varlena.h"
-
+#include "utils/guc.h"
 
 /*
  * Describes the valid options for objects that this wrapper uses.
@@ -143,6 +143,19 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("%s requires a non-negative integer value",
 								def->defname)));
 		}
+		else if (strcmp(def->defname, "work_mem") == 0)
+		{
+			int work_mem = 0;
+			bool result;
+			result = parse_int(defGetString(def), &work_mem, GUC_UNIT_KB, NULL);
+			if ((!result) || ((work_mem < 64) || (work_mem > MAX_KILOBYTES)))
+			{
+				ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("invalid value for option '%s'",
+							def->defname)));
+			}
+		}
 	}
 
 	PG_RETURN_VOID();
@@ -177,6 +190,8 @@ InitPgFdwOptions(void)
 		/* fetch_size is available on both server and table */
 		{"fetch_size", ForeignServerRelationId, false},
 		{"fetch_size", ForeignTableRelationId, false},
+		/* work_mem */
+		{"work_mem", ForeignServerRelationId, true},
 		{NULL, InvalidOid, false}
 	};
 
@@ -299,6 +314,8 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
 {
 	ListCell   *lc;
 	int			i;
+	static const char options[] = "options";
+	char *buf;
 
 	/* Build our options lists if we didn't yet. */
 	InitPgFdwOptions();
@@ -310,8 +327,18 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
 
 		if (is_libpq_option(d->defname))
 		{
-			keywords[i] = d->defname;
-			values[i] = defGetString(d);
+			if (strcmp(d->defname, "work_mem") == 0)
+			{
+				keywords[i] = options;
+				buf = palloc(128);
+				sprintf(buf, "-c work_mem=%s", defGetString(d));
+				values[i] = buf;
+			}
+			else
+			{
+				keywords[i] = d->defname;
+				values[i] = defGetString(d);
+			}
 			i++;
 		}
 	}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index c0b0dd9..c450eca 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2348,3 +2348,24 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+
+ROLLBACK;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 54b5e98..50a508e 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -313,6 +313,15 @@
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>work_mem</literal></term>
+     <listitem>
+      <para>
+       This option specifies the size of working memory using <xref linkend="guc-work-mem"/>.
+       The default value is depend on th remote instance setting.
+      </para>
+     </listitem>
+    </varlistentry>  
    </variablelist>
 
   </sect3>
#3Robert Haas
robertmhaas@gmail.com
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#1)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Fri, Aug 17, 2018 at 2:07 AM Shinoda, Noriyoshi (PN Japan GCS Delivery) <
noriyoshi.shinoda@hpe.com> wrote:

The attached patch adds a new option work_mem to postgres_fdw contrib
module.

Previously, it was impossible to change the setting of work_mem for remote
session with connection by postgres_fdw.

It would be nicer to have a generic way to set any GUC, vs. something that
only works for work_mem.

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

In reply to: Robert Haas (#3)
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Robert-san,

Thank you very much for your comment.
I will try to modify it so that GUC can be added more generically.
When specifying multiple GUC settings for PQconnectdbParams, is it correct to describe as follows?

--
keywords [n] = "options";
values [n] = "-c work_mem=8MB -c maintenance_work_mem=16MB";

conn = PQconnectdbParams (keywords, values, false);
--

There is no explanation about multiple guc settings in the manual for "options".

Best regards,
Noriyoshi Shinoda.

-----Original Message-----
From: Robert Haas [mailto:robertmhaas@gmail.com]
Sent: Saturday, August 25, 2018 4:49 AM
To: Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi.shinoda@hpe.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Fri, Aug 17, 2018 at 2:07 AM Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi.shinoda@hpe.com> wrote:
The attached patch adds a new option work_mem to postgres_fdw contrib module.
Previously, it was impossible to change the setting of work_mem for remote session with connection by postgres_fdw.

It would be nicer to have a generic way to set any GUC, vs. something that only works for work_mem.

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

#5Michael Paquier
michael@paquier.xyz
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#4)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Hi Shinoda-san,

On Sun, Aug 26, 2018 at 02:21:55AM +0000, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

Thank you very much for your comment.
I will try to modify it so that GUC can be added more generically.

It seems to me that you would pass down just a string which gets
allocated for "options", and injection risks are something to be careful
about. Another possibility would be an array with comma-separated
arguments, say:
options = 'option1=foo,option2=bar'
There is already some work done with comma-separated arguments for the
parameter "extensions", now that's more work.

When specifying multiple GUC settings for PQconnectdbParams, is it
correct to describe as follows?

Yep, it seems to me that you have that right.

https://www.postgresql.org/docs/devel/static/libpq-connect.html#LIBPQ-CONNSTRING

options
Specifies command-line options to send to the server at connection
start. For example, setting this to -c geqo=off sets the session's
value of the geqo parameter to off. Spaces within this string are
considered to separate command-line arguments, unless escaped with a
backslash (\); write \\ to represent a literal backslash. For a
detailed discussion of the available options, consult Chapter 19.

--
Michael

#6Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#5)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that you would pass down just a string which gets
allocated for "options", and injection risks are something to be careful
about. Another possibility would be an array with comma-separated
arguments, say:
options = 'option1=foo,option2=bar'
There is already some work done with comma-separated arguments for the
parameter "extensions", now that's more work.

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

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

#7Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Robert Haas (#6)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Mon, Aug 27, 2018 at 3:35 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier <michael@paquier.xyz>

wrote:

It seems to me that you would pass down just a string which gets
allocated for "options", and injection risks are something to be careful
about. Another possibility would be an array with comma-separated
arguments, say:
options = 'option1=foo,option2=bar'
There is already some work done with comma-separated arguments for the
parameter "extensions", now that's more work.

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

Maybe we can use multiple "options". Something like:

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb',
option='option1=foo', option='option2=bar' );

Regards,

--
Fabrízio de Royes Mello Timbira - http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

#8Michael Paquier
michael@paquier.xyz
In reply to: Robert Haas (#6)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Mon, Aug 27, 2018 at 02:34:31PM -0400, Robert Haas wrote:

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

proconfig already handles such cases, that's what I was coming at. We
could reuse most of what it does, no?
--
Michael

In reply to: Fabrízio de Royes Mello (#7)
1 attachment(s)
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Hi Everyone, thank you for your comment.

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', option='option1=foo', option='option2=bar' );

I do not want to allow postgres_fdw to set "all" GUCs. Because I think it is very difficult to check the validity of all input values.
So, I attached modified the patch so that we can easily add GUC that we can set to postgres_fdw module.
If you wish to add more GUC options to the module, add values to the non_libpq_options[] array of the InitPgFdwOptions function,
And add the validator code for the GUC in the postgres_fdw_validator function.

Best Regards,
Noriyoshi Shinoda

-------------
From: Fabrízio de Royes Mello [mailto:fabriziomello@gmail.com]
Sent: Tuesday, August 28, 2018 3:53 AM
To: [pgdg] Robert Haas <robertmhaas@gmail.com>
Cc: michael@paquier.xyz; Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi.shinoda@hpe.com>; Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Mon, Aug 27, 2018 at 3:35 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that you would pass down just a string which gets
allocated for "options", and injection risks are something to be careful
about.  Another possibility would be an array with comma-separated
arguments, say:
options = 'option1=foo,option2=bar'
There is already some work done with comma-separated arguments for the
parameter "extensions", now that's more work.

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

Maybe we can use multiple "options". Something like:

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', option='option1=foo', option='option2=bar' );

Regards,

--
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Attachments:

postgres_fdw_work_mem_v3.patchapplication/octet-stream; name=postgres_fdw_work_mem_v3.patchDownload
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index fe4893a..65d90fb 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -235,10 +235,10 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 		/*
 		 * Construct connection params from generic options of ForeignServer
 		 * and UserMapping.  (Some of them might not be libpq options, in
-		 * which case we'll just waste a few array slots.)  Add 3 extra slots
-		 * for fallback_application_name, client_encoding, end marker.
+		 * which case we'll just waste a few array slots.)  Add 4 extra slots
+		 * for fallback_application_name, client_encoding, guc, end marker.
 		 */
-		n = list_length(server->options) + list_length(user->options) + 3;
+		n = list_length(server->options) + list_length(user->options) + 4;
 		keywords = (const char **) palloc(n * sizeof(char *));
 		values = (const char **) palloc(n * sizeof(char *));
 
@@ -247,6 +247,8 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 									  keywords + n, values + n);
 		n += ExtractConnectionOptions(user->options,
 									  keywords + n, values + n);
+		n += ExtractGucOptions(server->options,
+									  keywords + n, values + n);
 
 		/* Use "postgres_fdw" as fallback_application_name. */
 		keywords[n] = "fallback_application_name";
@@ -286,6 +288,16 @@ connect_pg_server(ForeignServer *server, UserMapping *user)
 		/* Prepare new session for use */
 		configure_remote_session(conn);
 
+		/* Free memory for "options" parameter */
+		for (int i = 0; i < n; i++)
+		{
+			if (strcmp(keywords[i], "options") == 0)
+			{
+				pfree((void *)(values[i]));
+				break;
+			}
+		}
+
 		pfree(keywords);
 		pfree(values);
 	}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d912bd9..c529a00 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8607,3 +8607,28 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+ count 
+-------
+     1
+(1 row)
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+ count 
+-------
+     1
+(1 row)
+
+ROLLBACK;
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 6854f1b..67adb79 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -22,7 +22,7 @@
 #include "commands/extension.h"
 #include "utils/builtins.h"
 #include "utils/varlena.h"
-
+#include "utils/guc.h"
 
 /*
  * Describes the valid options for objects that this wrapper uses.
@@ -32,6 +32,7 @@ typedef struct PgFdwOption
 	const char *keyword;
 	Oid			optcontext;		/* OID of catalog in which option may appear */
 	bool		is_libpq_opt;	/* true if it's used in libpq */
+	bool		is_guc_opt;	/* true if it's used for GUC setting */
 } PgFdwOption;
 
 /*
@@ -52,7 +53,7 @@ static PQconninfoOption *libpq_options;
 static void InitPgFdwOptions(void);
 static bool is_valid_option(const char *keyword, Oid context);
 static bool is_libpq_option(const char *keyword);
-
+static bool is_guc_option(const char *keyword);
 
 /*
  * Validate the generic options given to a FOREIGN DATA WRAPPER, SERVER,
@@ -143,6 +144,19 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 						 errmsg("%s requires a non-negative integer value",
 								def->defname)));
 		}
+		else if (strcmp(def->defname, "work_mem") == 0)
+		{
+			int work_mem = 0;
+			bool result;
+			result = parse_int(defGetString(def), &work_mem, GUC_UNIT_KB, NULL);
+			if ((!result) || ((work_mem < 64) || (work_mem > MAX_KILOBYTES)))
+			{
+				ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("invalid value for option '%s'",
+							def->defname)));
+			}
+		}
 	}
 
 	PG_RETURN_VOID();
@@ -160,24 +174,27 @@ InitPgFdwOptions(void)
 
 	/* non-libpq FDW-specific FDW options */
 	static const PgFdwOption non_libpq_options[] = {
-		{"schema_name", ForeignTableRelationId, false},
-		{"table_name", ForeignTableRelationId, false},
-		{"column_name", AttributeRelationId, false},
+		{"schema_name", ForeignTableRelationId, false, false},
+		{"table_name", ForeignTableRelationId, false, false},
+		{"column_name", AttributeRelationId, false, false},
 		/* use_remote_estimate is available on both server and table */
-		{"use_remote_estimate", ForeignServerRelationId, false},
-		{"use_remote_estimate", ForeignTableRelationId, false},
+		{"use_remote_estimate", ForeignServerRelationId, false, false},
+		{"use_remote_estimate", ForeignTableRelationId, false, false},
 		/* cost factors */
-		{"fdw_startup_cost", ForeignServerRelationId, false},
-		{"fdw_tuple_cost", ForeignServerRelationId, false},
+		{"fdw_startup_cost", ForeignServerRelationId, false, false},
+		{"fdw_tuple_cost", ForeignServerRelationId, false, false},
 		/* shippable extensions */
-		{"extensions", ForeignServerRelationId, false},
+		{"extensions", ForeignServerRelationId, false, false},
 		/* updatable is available on both server and table */
-		{"updatable", ForeignServerRelationId, false},
-		{"updatable", ForeignTableRelationId, false},
+		{"updatable", ForeignServerRelationId, false, false},
+		{"updatable", ForeignTableRelationId, false, false},
 		/* fetch_size is available on both server and table */
-		{"fetch_size", ForeignServerRelationId, false},
-		{"fetch_size", ForeignTableRelationId, false},
-		{NULL, InvalidOid, false}
+		{"fetch_size", ForeignServerRelationId, false, false},
+		{"fetch_size", ForeignTableRelationId, false, false},
+		/* for GUC settings */
+		/* work_mem */
+		{"work_mem", ForeignServerRelationId, false, true},
+		{NULL, InvalidOid, false, false}
 	};
 
 	/* Prevent redundant initialization. */
@@ -241,6 +258,7 @@ InitPgFdwOptions(void)
 		else
 			popt->optcontext = ForeignServerRelationId;
 		popt->is_libpq_opt = true;
+		popt->is_guc_opt = false;
 
 		popt++;
 	}
@@ -289,6 +307,25 @@ is_libpq_option(const char *keyword)
 }
 
 /*
+ * Check whether the given option is one of the valid GUC options.
+ */
+static bool
+is_guc_option(const char *keyword)
+{
+	PgFdwOption *opt;
+
+	Assert(postgres_fdw_options);	/* must be initialized already */
+
+	for (opt = postgres_fdw_options; opt->keyword; opt++)
+	{
+		if (opt->is_guc_opt && strcmp(opt->keyword, keyword) == 0)
+			return true;
+	}
+
+	return false;
+}
+
+/*
  * Generate key-value arrays which include only libpq options from the
  * given list (which can contain any kind of options).  Caller must have
  * allocated large-enough arrays.  Returns number of options found.
@@ -319,6 +356,42 @@ ExtractConnectionOptions(List *defelems, const char **keywords,
 }
 
 /*
+ * Generate key-value arrays which include for GUC options from the
+ * given list (which can contain any kind of options).  Caller must have
+ * allocated large-enough arrays.  Returns number of options found.
+ */
+int
+ExtractGucOptions(List *defelems, const char **keywords,
+						 const char **values)
+{
+	ListCell   *lc;
+	bool is_guc = false;
+	StringInfoData buf;
+	static const char options[] = "options";
+
+	initStringInfo(&buf);
+
+	foreach(lc, defelems)
+	{
+		DefElem    *d = (DefElem *) lfirst(lc);
+
+		if (is_guc_option(d->defname))
+		{
+			is_guc = true;
+			appendStringInfo(&buf, " -c %s=%s ", d->defname, defGetString(d));
+		}
+	}
+	
+	if (is_guc)
+	{
+		keywords[0] = options;
+		values[0] = buf.data;
+	}
+
+	return (is_guc ? 1 : 0);
+}
+
+/*
  * Parse a comma-separated string and return a List of the OIDs of the
  * extensions named in the string.  If any names in the list cannot be
  * found, report a warning if warnOnMissing is true, else just silently
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 70b538e..01e94a1 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -130,6 +130,9 @@ extern int ExtractConnectionOptions(List *defelems,
 						 const char **values);
 extern List *ExtractExtensionList(const char *extensionsString,
 					 bool warnOnMissing);
+extern int ExtractGucOptions(List *defelems,
+						 const char **keywords,
+						 const char **values);
 
 /* in deparse.c */
 extern void classifyConditions(PlannerInfo *root,
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index c0b0dd9..c450eca 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2348,3 +2348,24 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+
+-- ===================================================================
+-- test for work_mem option
+-- ===================================================================
+BEGIN;
+
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( work_mem '64kB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=64kB'];
+
+ALTER SERVER workmem1 OPTIONS( SET work_mem '8MB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['work_mem=8MB'];
+
+ROLLBACK;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 54b5e98..50a508e 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -313,6 +313,15 @@
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>work_mem</literal></term>
+     <listitem>
+      <para>
+       This option specifies the size of working memory using <xref linkend="guc-work-mem"/>.
+       The default value is depend on th remote instance setting.
+      </para>
+     </listitem>
+    </varlistentry>  
    </variablelist>
 
   </sect3>
#10Kyotaro HORIGUCHI
horiguchi.kyotaro@lab.ntt.co.jp
In reply to: Michael Paquier (#8)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

At Mon, 27 Aug 2018 19:38:19 -0700, Michael Paquier <michael@paquier.xyz> wrote in <20180828023819.GA29157@paquier.xyz>

On Mon, Aug 27, 2018 at 02:34:31PM -0400, Robert Haas wrote:

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

proconfig already handles such cases, that's what I was coming at. We
could reuse most of what it does, no?

IIUC, proconfig is a text array, options is a text, and the
options psql-option is in the format of '-cvar=val -cvar=val...'.
Thus I think we still need something to convert them.

As an experiment, just with the following mod:

contrib/postgres_fdw/option.c
       /* Hide debug options, as well as settings we override internally. */
-      if (strchr(lopt->dispchar, 'D') ||
+      if ((strchr(lopt->dispchar, 'D') &&
+           strcmp(lopt->keyword, "options") != 0) ||
           strcmp(lopt->keyword, "fallback_application_name") == 0 ||

and the following setup:

=# alter server sv1
options (set options '-csearch_path=\\"hoge,hage\\"\\ -cwork_mem=16MB');

actually works. Converting array representation into the
commandline options format shown above internally might make it
look better a bit..

=# alter server sv1
options (set options '{"search_path=\"hoge,hage\"",work_mem=16MB}');

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#11Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#9)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On 28/08/2018 05:55, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', option='option1=foo', option='option2=bar' );

I do not want to allow postgres_fdw to set "all" GUCs. Because I think it is very difficult to check the validity of all input values.
So, I attached modified the patch so that we can easily add GUC that we can set to postgres_fdw module.
If you wish to add more GUC options to the module, add values to the non_libpq_options[] array of the InitPgFdwOptions function,
And add the validator code for the GUC in the postgres_fdw_validator function.

We already have a method for libpq applications to pass GUC settings via
connection parameters. And postgres_fdw supports passing libpq
connection parameters as server options. So why doesn't this work here?

The reason is that postgres_fdw filters out connection settings that are
marked debug ("D"), and the "options" keyword is marked as such. I
think this is wrong. Just remove that designation and then this will
work. (Perhaps filtering out the debug options is also wrong, but I can
see an argument for it.)

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

#12Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#9)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Tue, Aug 28, 2018 at 12:55 PM, Shinoda, Noriyoshi (PN Japan GCS
Delivery) <noriyoshi.shinoda@hpe.com> wrote:

Hi Everyone, thank you for your comment.

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb', option='option1=foo', option='option2=bar' );

I do not want to allow postgres_fdw to set "all" GUCs. Because I think it is very difficult to check the validity of all input values.
So, I attached modified the patch so that we can easily add GUC that we can set to postgres_fdw module.
If you wish to add more GUC options to the module, add values to the non_libpq_options[] array of the InitPgFdwOptions function,
And add the validator code for the GUC in the postgres_fdw_validator function.

FWIW, in term of usability I'm not sure that setting GUCs per foreign
servers would be useful for users. The setting parameters per foreign
servers affects all statements that touches it, which is different
behavior from the local some parameters, especially for query tuning
parameters. Is it worth to consider another ways to specify GUC
parameter per statements or transactions?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

In reply to: Peter Eisentraut (#11)
1 attachment(s)
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

Eisentraut-san
Thank you for becoming a reviewer.

The reason is that postgres_fdw filters out connection settings that are marked debug ("D"), and the "options" keyword is marked as such.
I think this is wrong. Just remove that designation and then this will work.
(Perhaps filtering out the debug options is also wrong, but I can see an argument for it.)

Certainly the PQconndefaults function specifies Debug flag for the "options" option.
I think that eliminating the Debug flag is the simplest solution.
For attached patches, GUC can be specified with the following syntax.

CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'host 1', ..., options '-c work_mem=64MB -c geqo=off');
ALTER SERVER remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off');

However, I am afraid of the effect that this patch will change the behavior of official API PQconndefaults.

Best Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Peter Eisentraut [mailto:peter.eisentraut@2ndquadrant.com]
Sent: Friday, August 31, 2018 2:45 AM
To: Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi.shinoda@hpe.com>; fabriziomello@gmail.com; [pgdg] Robert Haas <robertmhaas@gmail.com>; michael@paquier.xyz
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On 28/08/2018 05:55, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

I like the direction of your thinking, but it seems to me that this
would cause a problem if you want to set search_path=foo,bar.

... OPTIONS ( host 'remhost1', port '5433', dbname 'demodb',
option='option1=foo', option='option2=bar' );

I do not want to allow postgres_fdw to set "all" GUCs. Because I think it is very difficult to check the validity of all input values.
So, I attached modified the patch so that we can easily add GUC that we can set to postgres_fdw module.
If you wish to add more GUC options to the module, add values to the
non_libpq_options[] array of the InitPgFdwOptions function, And add the validator code for the GUC in the postgres_fdw_validator function.

We already have a method for libpq applications to pass GUC settings via connection parameters. And postgres_fdw supports passing libpq connection parameters as server options. So why doesn't this work here?

The reason is that postgres_fdw filters out connection settings that are marked debug ("D"), and the "options" keyword is marked as such. I think this is wrong. Just remove that designation and then this will work. (Perhaps filtering out the debug options is also wrong, but I can see an argument for it.)

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

Attachments:

postgres_fdw_work_mem_v4.patchapplication/octet-stream; name=postgres_fdw_work_mem_v4.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 21a2ef5..01ea77d 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8650,3 +8650,28 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+-- ===================================================================
+-- test for options option
+-- ===================================================================
+BEGIN;
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( options '-c work_mem=64kB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['options=-c work_mem=64kB'];
+ count 
+-------
+     1
+(1 row)
+
+ALTER SERVER workmem1 OPTIONS( SET options '-c work_mem=8MB' );
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['options=-c work_mem=8MB'];
+ count 
+-------
+     1
+(1 row)
+
+ROLLBACK;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 88c4cb4..c37c001 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2354,3 +2354,24 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700
 
 -- Clean-up
 RESET enable_partitionwise_aggregate;
+
+-- ===================================================================
+-- test for options option
+-- ===================================================================
+BEGIN;
+
+CREATE SERVER workmem1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS( options '-c work_mem=64kB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['options=-c work_mem=64kB'];
+
+ALTER SERVER workmem1 OPTIONS( SET options '-c work_mem=8MB' );
+
+SELECT count(*)
+FROM pg_foreign_server
+WHERE srvname = 'workmem1'
+AND srvoptions @> array['options=-c work_mem=8MB'];
+
+ROLLBACK;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 54b5e98..310f5ec 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -313,6 +313,17 @@
      </listitem>
     </varlistentry>
 
+    <varlistentry>
+     <term><literal>options</literal></term>
+     <listitem>
+      <para>
+       This option specifies the command-line options to send to the server at connection start. 
+       For example, setting this to <literal>-c geqo=off</literal> sets the session's value of the geqo parameter to off.
+       To set more than one option, specify <literal>-c param=value</literal> repeatedly.
+      </para>
+     </listitem>
+    </varlistentry>
+
    </variablelist>
 
   </sect3>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a8048ff..e85e67e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -236,7 +236,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 	offsetof(struct pg_conn, pgtty)},
 
 	{"options", "PGOPTIONS", DefaultOption, NULL,
-		"Backend-Debug-Options", "D", 40,
+		"Backend-Debug-Options", "", 40,
 	offsetof(struct pg_conn, pgoptions)},
 
 	{"application_name", "PGAPPNAME", NULL, NULL,
#14Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Shinoda, Noriyoshi (PN Japan GCS Delivery) (#13)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

Certainly the PQconndefaults function specifies Debug flag for the "options" option.
I think that eliminating the Debug flag is the simplest solution.
For attached patches, GUC can be specified with the following syntax.

CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'host 1', ..., options '-c work_mem=64MB -c geqo=off');
ALTER SERVER remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off');

However, I am afraid of the effect that this patch will change the behavior of official API PQconndefaults.

It doesn't change the behavior of the API, it just changes the result of
the API function, which is legitimate and the reason we have the API
function in the first place.

I think this patch is fine. I'll work on committing it.

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

#15Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Peter Eisentraut (#14)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On 05/09/2018 18:46, Peter Eisentraut wrote:

On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

Certainly the PQconndefaults function specifies Debug flag for the "options" option.
I think that eliminating the Debug flag is the simplest solution.
For attached patches, GUC can be specified with the following syntax.

CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'host 1', ..., options '-c work_mem=64MB -c geqo=off');
ALTER SERVER remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off');

However, I am afraid of the effect that this patch will change the behavior of official API PQconndefaults.

It doesn't change the behavior of the API, it just changes the result of
the API function, which is legitimate and the reason we have the API
function in the first place.

I think this patch is fine. I'll work on committing it.

I have committed just the libpq change. The documentation change was
redundant, because the documentation already stated that all libpq
options are accepted. (Arguably, the documentation was wrong before.)
Also, the proposed test change didn't seem to add much. It just checked
that the foreign server option is accepted, but not whether it does
anything. If you want to develop a more substantive test, we could
consider it, but I feel that since this all just goes to libpq, we don't
need to test it further.

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

In reply to: Peter Eisentraut (#15)
RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

I have committed just the libpq change. The documentation change was redundant, because the documentation already stated that all libpq options are accepted. (Arguably, the documentation was wrong before.) Also, the proposed test change didn't >seem to add much. It just checked that the foreign server option is accepted, but not whether it does anything. If you want to develop a more substantive test, we could consider it, but I feel that since this all just goes to libpq, we don't need to test it >further.

Thanks !

Regards.

Noriyoshi Shinoda

-----Original Message-----
From: Peter Eisentraut [mailto:peter.eisentraut@2ndquadrant.com]
Sent: Friday, September 7, 2018 10:17 PM
To: Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi.shinoda@hpe.com>; fabriziomello@gmail.com; [pgdg] Robert Haas <robertmhaas@gmail.com>; michael@paquier.xyz
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On 05/09/2018 18:46, Peter Eisentraut wrote:

On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

Certainly the PQconndefaults function specifies Debug flag for the "options" option.
I think that eliminating the Debug flag is the simplest solution.
For attached patches, GUC can be specified with the following syntax.

CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host
'host 1', ..., options '-c work_mem=64MB -c geqo=off'); ALTER SERVER
remsv11 OPTIONS (SET options '-c work_mem=64MB -c geqo=off');

However, I am afraid of the effect that this patch will change the behavior of official API PQconndefaults.

It doesn't change the behavior of the API, it just changes the result
of the API function, which is legitimate and the reason we have the
API function in the first place.

I think this patch is fine. I'll work on committing it.

I have committed just the libpq change. The documentation change was redundant, because the documentation already stated that all libpq options are accepted. (Arguably, the documentation was wrong before.) Also, the proposed test change didn't seem to add much. It just checked that the foreign server option is accepted, but not whether it does anything. If you want to develop a more substantive test, we could consider it, but I feel that since this all just goes to libpq, we don't need to test it further.

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

#17Jeff Janes
jeff.janes@gmail.com
In reply to: Peter Eisentraut (#15)
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module

On Fri, Sep 7, 2018 at 9:17 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 05/09/2018 18:46, Peter Eisentraut wrote:

On 01/09/2018 06:33, Shinoda, Noriyoshi (PN Japan GCS Delivery) wrote:

Certainly the PQconndefaults function specifies Debug flag for the

"options" option.

I think that eliminating the Debug flag is the simplest solution.
For attached patches, GUC can be specified with the following syntax.

CREATE SERVER remsvr1 FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host

'host 1', ..., options '-c work_mem=64MB -c geqo=off');

ALTER SERVER remsv11 OPTIONS (SET options '-c work_mem=64MB -c

geqo=off');

However, I am afraid of the effect that this patch will change the

behavior of official API PQconndefaults.

It doesn't change the behavior of the API, it just changes the result of
the API function, which is legitimate and the reason we have the API
function in the first place.

I think this patch is fine. I'll work on committing it.

I have committed just the libpq change. The documentation change was
redundant, because the documentation already stated that all libpq
options are accepted. (Arguably, the documentation was wrong before.)

Since the effect of this commit was to make postgres_fdw actually comply
with its documentation,
should this have been back-patched? Is there a danger in backpatching this
change to libpq to older versions?

This seems like more of a bug fix than an added feature.

Cheers,

Jeff