Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
On Fri, Sep 7, 2018 at 9:17 AM Peter Eisentraut < peter.eisentr...@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
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.eisentr...@2ndquadrant.com] Sent: Friday, September 7, 2018 10:17 PM To: Shinoda, Noriyoshi (PN Japan GCS Delivery) ; fabriziome...@gmail.com; [pgdg] Robert Haas ; mich...@paquier.xyz Cc: Pgsql Hackers 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
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
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.eisentr...@2ndquadrant.com]
Sent: Friday, August 31, 2018 2:45 AM
To: Shinoda, Noriyoshi (PN Japan GCS Delivery) ;
fabriziome...@gmail.com; [pgdg] Robert Haas ;
mich...@paquier.xyz
Cc: Pgsql Hackers
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
postgres_fdw_work_mem_v4.patch
Description: postgres_fdw_work_mem_v4.patch
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) 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
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
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
At Mon, 27 Aug 2018 19:38:19 -0700, Michael Paquier wrote
in <20180828023819.ga29...@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
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:fabriziome...@gmail.com] Sent: Tuesday, August 28, 2018 3:53 AM To: [pgdg] Robert Haas Cc: mich...@paquier.xyz; Shinoda, Noriyoshi (PN Japan GCS Delivery) ; Pgsql Hackers Subject: Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module On Mon, Aug 27, 2018 at 3:35 PM Robert Haas wrote: > > On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier 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 postgres_fdw_work_mem_v3.patch Description: postgres_fdw_work_mem_v3.patch
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 signature.asc Description: PGP signature
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
On Mon, Aug 27, 2018 at 3:35 PM Robert Haas wrote: > > On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier 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
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
On Mon, Aug 27, 2018 at 1:29 AM Michael Paquier 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
Re: [HACKERS] Proposal to add work_mem option to postgres_fdw module
Hi Shinoda-san, On Sun, Aug 26, 2018 at 02:21:55AM +, 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 signature.asc Description: PGP signature
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:robertmh...@gmail.com] Sent: Saturday, August 25, 2018 4:49 AM To: Shinoda, Noriyoshi (PN Japan GCS Delivery) Cc: pgsql-hack...@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) 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
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.shin...@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
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-hack...@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
postgres_fdw_work_mem_v2.patch
Description: postgres_fdw_work_mem_v2.patch
[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
postgres_fdw_work_mem_v1.patch
Description: postgres_fdw_work_mem_v1.patch
