Обсуждение: [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
specifyis 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
PQconnectdbParamsAPI function. 

Best Regards,
Noriyoshi Shinoda

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

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

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

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

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
allinput 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

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

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
allinput 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
InitPgFdwOptionsfunction,
 
> 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

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
allinput 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
InitPgFdwOptionsfunction,
 
> 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

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"
keywordis 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
allinput 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_validatorfunction.
 

We already have a method for libpq applications to pass GUC settings via connection parameters.  And postgres_fdw
supportspassing 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
ismarked as such.  I think this is wrong.  Just remove that designation and then this will work.  (Perhaps filtering
outthe 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

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

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

>I have committed just the libpq change.  The documentation change was redundant, because the documentation already
statedthat all libpq options are accepted.  (Arguably, the documentation was wrong before.) Also, the proposed test
changedidn'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
goesto 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
statedthat all libpq options are accepted.  (Arguably, the documentation was wrong before.) Also, the proposed test
changedidn'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
goesto libpq, we don't need to test it further.
 

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