Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-03 Thread Robert Haas
On Wed, Feb 3, 2016 at 11:17 AM, Ashutosh Bapat wrote: > There seems to be double successive assignment to fdw_private in recent > commit. Here's patch to remove the first one. Committed along with a fix for another problem I noted along the way. -- Robert Haas EnterpriseDB: http://www.enterpri

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-03 Thread Ashutosh Bapat
There seems to be double successive assignment to fdw_private in recent commit. Here's patch to remove the first one. On Wed, Feb 3, 2016 at 7:40 PM, Robert Haas wrote: > On Tue, Feb 2, 2016 at 10:42 PM, Corey Huinker > wrote: > >> I don't see how. There really is no declaration in there for a

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-03 Thread Robert Haas
On Tue, Feb 2, 2016 at 10:42 PM, Corey Huinker wrote: >> I don't see how. There really is no declaration in there for a >> variable called server. > > Absolutely correct. My only guess is that it was from failing to make clean > after a checkout/re-checkout. A good reason to have even boring regr

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-02 Thread Corey Huinker
> > > I don't see how. There really is no declaration in there for a > variable called server. > Absolutely correct. My only guess is that it was from failing to make clean after a checkout/re-checkout. A good reason to have even boring regression tests. Regression tests added. diff --git a/cont

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-02 Thread Robert Haas
On Tue, Feb 2, 2016 at 5:57 PM, Corey Huinker wrote: >> postgres_fdw.c:2642:16: error: use of undeclared identifier 'server' >> foreach(lc, server->options) > > > Odd. Compiled for me. Maybe I messed up the diff. Will get back to you on > that with the tests suggested. I d

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-02 Thread Corey Huinker
> > > postgres_fdw.c:2642:16: error: use of undeclared identifier 'server' > foreach(lc, server->options) > Odd. Compiled for me. Maybe I messed up the diff. Will get back to you on that with the tests suggested. > ^ > ../../src/includ

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-02 Thread Robert Haas
On Wed, Jan 27, 2016 at 11:19 PM, Corey Huinker wrote: > The possible tests might be: > - creating a server/table with fetch_size set > - altering an existing server/table to have fetch_size set > - verifying that the value exists in pg_foreign_server.srvoptions and > pg_foreign_table.ftoptions. >

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-02-01 Thread Alvaro Herrera
Corey Huinker wrote: > Enjoy. Didn't actually look into the patch but looks like there's progress here and this might be heading for commit. Moving to next one. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-27 Thread Corey Huinker
> > > Looks pretty good. You seem to have added a blank line at the end of > postgres_fdw.c, which should be stripped back out. > Done. > > > I'm not too keen on having *no* new regression tests, but defer to your > > judgement. > > So... I'm not *opposed* to regression tests. I just don't see

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-27 Thread Robert Haas
On Mon, Jan 25, 2016 at 4:18 PM, Corey Huinker wrote: > Revised in patch v3: > * get_option() and get_fetch_size() removed, fetch_size searches added to > existing loops. > * Move fetch_size <= 0 tests into postgres_fdw_validator() routine in > option.c > * DEBUG1 message removed, never intended t

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-25 Thread Corey Huinker
On Mon, Jan 25, 2016 at 3:22 PM, Robert Haas wrote: > On Mon, Jan 25, 2016 at 1:21 PM, Corey Huinker > wrote: > >> - We could consider folding fetch_size into "Remote Execution > >> Options", but maybe that's too clever. > > > > If you care to explain, I'm listening. Otherwise I'm going forward

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-25 Thread Robert Haas
On Mon, Jan 25, 2016 at 1:21 PM, Corey Huinker wrote: >> - We could consider folding fetch_size into "Remote Execution >> Options", but maybe that's too clever. > > If you care to explain, I'm listening. Otherwise I'm going forward with the > other suggestions you've made. It's just a little unfo

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-25 Thread Corey Huinker
> > > Review: > > - There is an established idiom in postgres_fdw for how to extract > data from lists of DefElems, and this patch does something else > instead. Please make it look like postgresIsForeignRelUpdatable, > postgresGetForeignRelSize, and postgresImportForeignSchema instead of > invent

Re: [HACKERS] [POC] FETCH limited by bytes.

2016-01-25 Thread Robert Haas
On Thu, Dec 31, 2015 at 1:10 AM, Corey Huinker wrote: > Here's a re-based patch. Notable changes since the last patch are: > > - some changes had to move to postgres_fdw.h > - the regression tests are in their own script fetch_size.sql / > fetch_size.out. that should make things easier for the rev

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-30 Thread Corey Huinker
> > >> I believe it won't be needed as a regression test but DEBUGn >> messages could be usable to see "what was sent to the remote >> side". It can be shown to the console so easily done in >> regression test. See the deocumentation for client_min_messages >> for details. (It could receive far man

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-26 Thread Vladimir Sitnikov
>and fetch a number of rows that, by its estimation, would fit in the memory >available What's wrong with having size limit in the first place? It seems to make much more sense. Vladimir -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-26 Thread Corey Huinker
On Sat, Dec 26, 2015 at 6:16 AM, Vladimir Sitnikov < sitnikov.vladi...@gmail.com> wrote: > >Have you got numbers showing any actual performance win for postgres_fdw? > > For JDBC purposes, it would be nice to have an ability of asking > backend "to stop fetching if produced more than X MiB of resp

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-26 Thread Vladimir Sitnikov
>Have you got numbers showing any actual performance win for postgres_fdw? For JDBC purposes, it would be nice to have an ability of asking backend "to stop fetching if produced more than X MiB of response data". For small table (4 int4 fields), having decent fetchSize (~1000) makes result process

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-26 Thread Corey Huinker
On Fri, Dec 25, 2015 at 12:31 AM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Hello, > > At Thu, 24 Dec 2015 18:31:42 -0500, Corey Huinker > wrote in fh+zueykccdu8p0pproyywlep5obrjkce5o7swqdf...@mail.gmail.com> > > On Wed, Dec 23, 2015 at 3:08 PM, Jim Nasby > wrote: > > > > >

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-24 Thread Kyotaro HORIGUCHI
Hello, At Thu, 24 Dec 2015 18:31:42 -0500, Corey Huinker wrote in > On Wed, Dec 23, 2015 at 3:08 PM, Jim Nasby wrote: > > > On 12/23/15 12:15 PM, Corey Huinker wrote: > > > >> That's fair. I'm still at a loss for how to show that the fetch size was > >> respected by the remote server, suggest

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-24 Thread Corey Huinker
On Wed, Dec 23, 2015 at 3:08 PM, Jim Nasby wrote: > On 12/23/15 12:15 PM, Corey Huinker wrote: > >> That's fair. I'm still at a loss for how to show that the fetch size was >> respected by the remote server, suggestions welcome! >> > > A combination of repeat() and generate_series()? > > I'm gues

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-23 Thread Jim Nasby
On 12/23/15 12:15 PM, Corey Huinker wrote: That's fair. I'm still at a loss for how to show that the fetch size was respected by the remote server, suggestions welcome! A combination of repeat() and generate_series()? I'm guessing it's not that obvious and that I'm missing something; can you

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-23 Thread Corey Huinker
On Wed, Dec 23, 2015 at 8:43 AM, Michael Paquier wrote: > On Mon, Sep 21, 2015 at 1:52 PM, Corey Huinker > wrote: > > Attached is the patch / diff against current master. > > I have moved this patch to next CF, there is a new patch, but no reviews. > -- > Michael > That's fair. I'm still at a

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-12-23 Thread Michael Paquier
On Mon, Sep 21, 2015 at 1:52 PM, Corey Huinker wrote: > Attached is the patch / diff against current master. I have moved this patch to next CF, there is a new patch, but no reviews. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subsc

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-09-20 Thread Corey Huinker
On Fri, Sep 4, 2015 at 2:45 AM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > Hello, > > > > > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node, > > > int eflags) > ... > > > > + def = get_option(table->options, "fetch_size"); > > > > > I don't think it's a

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-09-03 Thread Kyotaro HORIGUCHI
Hello, > > > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node, > > int eflags) ... > > > + def = get_option(table->options, "fetch_size"); > > > I don't think it's a good idea to make such checks at runtime - and > > either way it's somethign that should be reported back u

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-09-03 Thread Corey Huinker
On Wed, Sep 2, 2015 at 9:08 AM, Andres Freund wrote: > On 2015-02-27 13:50:22 -0500, Corey Huinker wrote: > > +static DefElem* > > +get_option(List *options, char *optname) > > +{ > > + ListCell *lc; > > + > > + foreach(lc, options) > > + { > > + DefElem *def = (DefElem *)

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-09-02 Thread Andres Freund
On 2015-02-27 13:50:22 -0500, Corey Huinker wrote: > +static DefElem* > +get_option(List *options, char *optname) > +{ > + ListCell *lc; > + > + foreach(lc, options) > + { > + DefElem *def = (DefElem *) lfirst(lc); > + > + if (strcmp(def->defname, optname) == 0)

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-02-27 Thread Corey Huinker
Attached is a diff containing the original (working) patch from my (incomplete) patch, plus regression test changes and documentation changes. While it's easy to regression-test the persistence of the fetch_size options, I am confounded as to how we would show that the fetch_size setting was respe

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-02-06 Thread Kyotaro HORIGUCHI
Hello, > Redshift has a table, stv_inflight, which serves about the same purpose as > pg_stat_activity. Redshift seems to perform better with very high fetch > sizes (10,000 is a good start), so the default foreign data wrapper didn't > perform so well. I agree with you. > I was able to confirm

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-02-04 Thread Corey Huinker
I applied this patch to REL9_4_STABLE, and I was able to connect to a foreign database (redshift, actually). the basic outline of the test is below, names changed to protect my employment. create extension if not exists postgres_fdw; create server redshift_server foreign data wrapper postgres_fd

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-02-01 Thread Kyotaro HORIGUCHI
Hmm, somehow I removed some recipients, especially the list. Sorry for the duplicate. - Sorry, I've been back. Thank you for the comment. > Do you have any insight into where I would pass the custom row fetches from > the table struct to the scan struct? Yeah it's one simple way to tune it,

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-01-27 Thread Corey Huinker
Last year I was working on a patch to postgres_fdw where the fetch_size could be set at the table level and the server level. I was able to get the settings parsed and they would show up in pg_foreign_table and pg_foreign_servers. Unfortunately, I'm not very familiar with how foreign data wrappers

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-01-27 Thread Kyotaro HORIGUCHI
Thank you for the comment. The automatic way to determin the fetch_size looks become too much for the purpose. An example of non-automatic way is a new foreign table option like 'fetch_size' but this exposes the inside too much... Which do you think is preferable? Thu, 22 Jan 2015 11:17:52 -0500,

Re: [HACKERS] [POC] FETCH limited by bytes.

2015-01-22 Thread Tom Lane
Kyotaro HORIGUCHI writes: > Hello, as the discuttion on async fetching on postgres_fdw, FETCH > with data-size limitation would be useful to get memory usage > stability of postgres_fdw. > Is such a feature and syntax could be allowed to be added? This seems like a lot of work, and frankly an in