RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module

From: "Shinoda, Noriyoshi (PN Japan GCS Delivery)" <noriyoshi(dot)shinoda(at)hpe(dot)com>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, "fabriziomello(at)gmail(dot)com" <fabriziomello(at)gmail(dot)com>, "[pgdg] Robert Haas" <robertmhaas(at)gmail(dot)com>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz>
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: RE: [HACKERS] Proposal to add work_mem option to postgres_fdw module
Date: 2018-09-01 04:33:18
Message-ID: TU4PR8401MB043012F3C5D6410E64611383EE0E0@TU4PR8401MB0430.NAMPRD84.PROD.OUTLOOK.COM
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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(dot)eisentraut(at)2ndquadrant(dot)com]
Sent: Friday, August 31, 2018 2:45 AM
To: Shinoda, Noriyoshi (PN Japan GCS Delivery) <noriyoshi(dot)shinoda(at)hpe(dot)com>; fabriziomello(at)gmail(dot)com; [pgdg] Robert Haas <robertmhaas(at)gmail(dot)com>; michael(at)paquier(dot)xyz
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)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

Attachment Content-Type Size
postgres_fdw_work_mem_v4.patch application/octet-stream 3.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2018-09-01 04:51:31 Re: remove ancient pre-dlopen dynloader code
Previous Message Robert Haas 2018-09-01 02:52:09 Re: pg_verify_checksums failure with hash indexes