Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 9/19/15 10:46 AM, Tom Lane wrote: > Peter Eisentraut writes: >> On 7/23/15 6:39 PM, Tom Lane wrote: >>> + 2202HEERRCODE_INVALID_TABLESAMPLE_ARGUMENT >>>invalid_tablesample_argument >>> + 2202GEERRCODE_INVALID_TABLESAMPLE_REPEAT

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Peter Eisentraut writes: > On 7/23/15 6:39 PM, Tom Lane wrote: >> + 2202HEERRCODE_INVALID_TABLESAMPLE_ARGUMENT >> invalid_tablesample_argument >> + 2202GEERRCODE_INVALID_TABLESAMPLE_REPEAT >> invalid_tablesample_repeat > Wher

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 7/23/15 6:39 PM, Tom Lane wrote: > + 2202HEERRCODE_INVALID_TABLESAMPLE_ARGUMENT > invalid_tablesample_argument > + 2202GEERRCODE_INVALID_TABLESAMPLE_REPEAT > invalid_tablesample_repeat Where did you get these error codes fr

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > I was wondering if we should perhaps cache the output of GetTsmRoutine > as we call it up to 4 times in the planner now but it's relatively cheap > call (basically just makeNode) so it's probably not worth it. Yeah, I was wondering about that too. The way to do it would

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-25 00:36, Tom Lane wrote: I wrote: Petr Jelinek writes: The only major difference that I see so far and I'd like you to incorporate that into your patch is that I renamed the SampleScanCost to SampleScanGetRelSize because that reflects much better the use of it, it isn't really used

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > Ok, attached are couple of cosmetic changes - what I wrote above, plus > comment about why we do float8 + hashing for REPEATABLE (it's not > obvious IMHO) and one additional test query. OK, thanks. > Do you want to do the contrib changes yourself as well or should I take

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > The only major difference that I see so far and I'd like you to > incorporate that into your patch is that I renamed the SampleScanCost to > SampleScanGetRelSize because that reflects much better the use of it, it > isn't really used for costing, but for getting the pages

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-24 01:26, Petr Jelinek wrote: On 2015-07-24 00:39, Tom Lane wrote: I wrote: OK, so "InitSampleScan" for a function called at ExecInitSampleScan time (which we might as well make optional), and then we'll use BeginSampleScan for the function that gets the parameters. The restart/ReSc

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-24 00:39, Tom Lane wrote: I wrote: OK, so "InitSampleScan" for a function called at ExecInitSampleScan time (which we might as well make optional), and then we'll use BeginSampleScan for the function that gets the parameters. The restart/ReScan function goes away since BeginSampleSca

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > On 2015-07-23 02:01, Tom Lane wrote: >> This needs to work more like LIMIT, which doesn't try to compute the >> limit parameters until the first fetch. So what we need is an Init >> function that does very darn little indeed (maybe we don't even need >> it at all), and then

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-23 02:01, Tom Lane wrote: This needs to work more like LIMIT, which doesn't try to compute the limit parameters until the first fetch. So what we need is an Init function that does very darn little indeed (maybe we don't even need it at all), and then a ParamInspect function that is c

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

I wrote: > We could alternatively provide two scan-initialization callbacks, > one that analyzes the parameters before we do heap_beginscan, > and another that can do additional setup afterwards. Actually, > that second call would really not be meaningfully different from > the ReScan call, so ano

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > Another thing that's not clear to me after playing with this is how do > we want to detect if to do pagemode scan or not. I understand that it's > neat optimization to say pagemode = true in bernoulli when percentage is > high and false when it's low but then this would h

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-20 17:23, Tom Lane wrote: Doesn't look like it to me: heap_beginscan_sampling always passes allow_sync = false to heap_beginscan_internal. Oh, right. More to the point, the existing coding of all these methods is such that they would fail to be reproducible if syncscan were enabl

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

>> to handle DROP dependency behaviors properly. (On reflection, maybe >> better if it's "bernoulli(internal) returns tablesample_handler", >> so as to guarantee that such a function doesn't create a conflict with >> any user-defined function of the same name.) > > The probability of conflict seem

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > On 2015-07-19 22:56, Tom Lane wrote: >> * I specified that omitting NextSampleBlock is allowed and causes the >> core code to do a standard seqscan, including syncscan support, which >> is a behavior that's impossible with the current API. If we fix >> the bernoulli code to

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-20 12:18, Petr Jelinek wrote: On 2015-07-19 22:56, Tom Lane wrote: * You might have expected me to move the tsmseqscan and tsmpagemode flags into the TsmRoutine struct, but instead this API puts equivalent flags into the SampleScanState struct. The reason for that is that it lets th

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-19 22:56, Tom Lane wrote: Since I'm not observing any movement on the key question of redesigning the tablesample method API, and I think that's something that's absolutely necessary to fix for 9.5, attached is an attempt to respecify the API. Sorry, I got something similar to what

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 19 July 2015 at 21:56, Tom Lane wrote: > Since I'm not observing any movement Apologies if nothing visible was occurring. Petr and I had arranged to meet and discuss Mon/Tues. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 2

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Since I'm not observing any movement on the key question of redesigning the tablesample method API, and I think that's something that's absolutely necessary to fix for 9.5, attached is an attempt to respecify the API. I haven't actually written any code, but I've written a tsmapi.h file modeled on

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-16 17:08, Tom Lane wrote: Petr Jelinek writes: On 2015-07-16 15:59, Tom Lane wrote: I'm not clear on whether sequence AMs would need explicit catalog representation, or could be folded down to just a single SQL function with special signature as I suggested for tablesample handlers.

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek writes: > On 2015-07-16 15:59, Tom Lane wrote: >> I'm not clear on whether sequence AMs would need explicit catalog >> representation, or could be folded down to just a single SQL function >> with special signature as I suggested for tablesample handlers. >> Is there any need for a se

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-16 15:59, Tom Lane wrote: Alvaro Herrera writes: Petr Jelinek wrote: On 2015-07-13 00:36, Tom Lane wrote: PS: now that I've written this rant, I wonder why we don't redesign the index AM API along the same lines. It probably doesn't matter much at the moment, but if we ever get se

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Amit Langote writes: > On Thu, Jul 16, 2015 at 10:33 PM, Alvaro Herrera > wrote: >> Hmm, how would this work? Would we have index AM implementation run >> some function that register their support methods somehow at startup? > I recall a proposal by Alexander Korotkov about extensible access >

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On Thu, Jul 16, 2015 at 10:33 PM, Alvaro Herrera wrote: > Petr Jelinek wrote: >> On 2015-07-13 00:36, Tom Lane wrote: > >> >PS: now that I've written this rant, I wonder why we don't redesign the >> >index AM API along the same lines. It probably doesn't matter much at >> >the moment, but if we e

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Alvaro Herrera writes: > Petr Jelinek wrote: >> On 2015-07-13 00:36, Tom Lane wrote: >>> PS: now that I've written this rant, I wonder why we don't redesign the >>> index AM API along the same lines. It probably doesn't matter much at >>> the moment, but if we ever get serious about supporting in

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek wrote: > On 2015-07-13 00:36, Tom Lane wrote: > >PS: now that I've written this rant, I wonder why we don't redesign the > >index AM API along the same lines. It probably doesn't matter much at > >the moment, but if we ever get serious about supporting index AM > >extensions, I think

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Petr Jelinek wrote: > On 2015-07-13 15:39, Tom Lane wrote: > >I don't find this to be good error message style. The secondary comment > >is not a "hint", it's an ironclad statement of what you did wrong, so if > >we wanted to phrase it like this it should be an errdetail not errhint. > >But the w

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-13 18:00, Tom Lane wrote: And here's that. I do *not* claim that this is a complete list of design issues with the patch, it's just things I happened to notice in the amount of time I've spent so far (which is already way more than I wanted to spend on TABLESAMPLE right now). I'm n

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-13 15:39, Tom Lane wrote: Datum tsm_system_rows_init(PG_FUNCTION_ARGS) { TableSampleDesc *tsdesc = (TableSampleDesc *) PG_GETARG_POINTER(0); uint32 seed = PG_GETARG_UINT32(1); int32 ntuples = PG_ARGISNULL(2) ? -1 : PG_GETARG_INT32(2); This is rather curious

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 2015-07-13 00:36, Tom Lane wrote: We have a far better model to follow already, namely the foreign data wrapper API. In that, there's a single SQL-visible function that returns a dummy datatype indicating that it's an FDW handler, and when called, it hands back a struct containing pointers t

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 15 July 2015 at 05:58, Noah Misch wrote: > > > If it's > > > to stay, it *must* get a line-by-line review from some committer-level > > > person; and I think there are other more important things for us to be > > > doing for 9.5. > > > > > > > Honestly, I am very surprised by this. > > Tom's

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On Tue, Jul 14, 2015 at 11:14:55AM +0100, Simon Riggs wrote: > On 13 July 2015 at 14:39, Tom Lane wrote: > > TBH, I think the right thing to do at this point is to revert the entire > > patch and send it back for ground-up rework. I think the high-level > > design is wrong in many ways and I have

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 14 July 2015 at 15:32, Tom Lane wrote: > Simon Riggs writes: > > On 13 July 2015 at 14:39, Tom Lane wrote: > >> TBH, I think the right thing to do at this point is to revert the entire > >> patch and send it back for ground-up rework. I think the high-level > >> design is wrong in many ways

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Simon Riggs writes: > On 13 July 2015 at 14:39, Tom Lane wrote: >> TBH, I think the right thing to do at this point is to revert the entire >> patch and send it back for ground-up rework. I think the high-level >> design is wrong in many ways and I have about zero confidence in most >> of the co

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 13 July 2015 at 14:39, Tom Lane wrote: > Michael Paquier writes: > > Regarding the fact that those two contrib modules can be part of a > > -contrib package and could be installed, nuking those two extensions > > from the tree and preventing the creating of custom tablesample > > methods look

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 13 July 2015 at 17:00, Tom Lane wrote: > I wrote: > > TBH, I think the right thing to do at this point is to revert the entire > > patch and send it back for ground-up rework. I think the high-level > > design is wrong in many ways and I have about zero confidence in most > > of the code deta

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On 11 July 2015 at 21:28, Tom Lane wrote: > What are we going to do about this? > I will address the points you raise, one by one. -- Simon Riggshttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

I wrote: > TBH, I think the right thing to do at this point is to revert the entire > patch and send it back for ground-up rework. I think the high-level > design is wrong in many ways and I have about zero confidence in most > of the code details as well. > I'll send a separate message about hig

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

Michael Paquier writes: > Regarding the fact that those two contrib modules can be part of a > -contrib package and could be installed, nuking those two extensions > from the tree and preventing the creating of custom tablesample > methods looks like a correct course of things to do for 9.5. TBH,

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

On Mon, Jul 13, 2015 at 7:36 AM, Tom Lane wrote: > I wrote: >> The two contrib modules this patch added are nowhere near fit for public >> consumption. They cannot clean up after themselves when dropped: >> ... >> Raw inserts into system catalogs just >> aren't a sane thing to do in extensions. >

Re: [HACKERS] TABLESAMPLE patch is really in pretty sad shape

I wrote: > The two contrib modules this patch added are nowhere near fit for public > consumption. They cannot clean up after themselves when dropped: > ... > Raw inserts into system catalogs just > aren't a sane thing to do in extensions. I had some thoughts about how we might fix that, without

[HACKERS] TABLESAMPLE patch is really in pretty sad shape

The two contrib modules this patch added are nowhere near fit for public consumption. They cannot clean up after themselves when dropped: regression=# create extension tsm_system_rows; CREATE EXTENSION regression=# create table big as select i, random() as x from generate_series(1,100) i; SE

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 23, 2015 at 4:31 AM, Petr Jelinek wrote: > On 19/04/15 01:24, Michael Paquier wrote: >> >> On Sat, Apr 18, 2015 at 8:38 PM, Michael Paquier >> wrote: >>> >>> On Fri, Apr 17, 2015 at 10:54 PM, Petr Jelinek wrote: On 10/04/15 06:46, Michael Paquier wrote: > > 13) Some

Re: [HACKERS] TABLESAMPLE patch

On 19/04/15 01:24, Michael Paquier wrote: On Sat, Apr 18, 2015 at 8:38 PM, Michael Paquier wrote: On Fri, Apr 17, 2015 at 10:54 PM, Petr Jelinek wrote: On 10/04/15 06:46, Michael Paquier wrote: 13) Some regression tests with pg_tablesample_method would be welcome. Not sure what you mean by

Re: [HACKERS] TABLESAMPLE patch

On Sat, Apr 18, 2015 at 8:38 PM, Michael Paquier wrote: > On Fri, Apr 17, 2015 at 10:54 PM, Petr Jelinek wrote: >> On 10/04/15 06:46, Michael Paquier wrote: >>> 13) Some regression tests with pg_tablesample_method would be welcome. >> >> Not sure what you mean by that. > > I meant a sanity check o

Re: [HACKERS] TABLESAMPLE patch

On Fri, Apr 17, 2015 at 10:54 PM, Petr Jelinek wrote: > On 10/04/15 06:46, Michael Paquier wrote: >> 13) Some regression tests with pg_tablesample_method would be welcome. > > Not sure what you mean by that. I meant a sanity check on pg_tablesample_method to be sure that tsminit, tsmnextblock and

Re: [HACKERS] TABLESAMPLE patch

On 17 April 2015 at 14:54, Petr Jelinek wrote: > I agree that DDL patch is not that important to get in (and I made it last > patch in the series now), which does not mean somebody can't write the > extension with new tablesample method. > > > In any case attached another version. > > Changes: >

Re: [HACKERS] TABLESAMPLE patch

On Sat, Apr 11, 2015 at 12:56 AM, Peter Eisentraut wrote: > > On 4/9/15 8:58 PM, Petr Jelinek wrote: > > Well, you can have two approaches to this, either allow some specific > > set of keywords that can be used to specify limit, or you let sampling > > methods interpret parameters, I believe the

Re: [HACKERS] TABLESAMPLE patch

On 10 April 2015 at 15:26, Peter Eisentraut wrote: > What is your intended use case for this feature? Likely use cases are: * Limits on numbers of rows in sample. Some research colleagues have published a new mathematical analysis that will allow a lower limit than previously considered. * Time

Re: [HACKERS] TABLESAMPLE patch

On 10/04/15 22:16, Tomas Vondra wrote: On 04/10/15 21:57, Petr Jelinek wrote: On 10/04/15 21:26, Peter Eisentraut wrote: But this was not really my point, the BERNOULLI just does not work well with row-limit by definition, it applies probability on each individual row and while you can get pr

Re: [HACKERS] TABLESAMPLE patch

On 04/10/15 21:57, Petr Jelinek wrote: On 10/04/15 21:26, Peter Eisentraut wrote: But this was not really my point, the BERNOULLI just does not work well with row-limit by definition, it applies probability on each individual row and while you can get probability from percentage very easily (j

Re: [HACKERS] TABLESAMPLE patch

On 10/04/15 21:26, Peter Eisentraut wrote: On 4/9/15 8:58 PM, Petr Jelinek wrote: Well, you can have two approaches to this, either allow some specific set of keywords that can be used to specify limit, or you let sampling methods interpret parameters, I believe the latter is more flexible. Ther

Re: [HACKERS] TABLESAMPLE patch

On 4/9/15 5:02 AM, Michael Paquier wrote: > Just to be clear, the example above being misleading... Doing table > sampling using SYSTEM at physical level makes sense. In this case I > think that we should properly error out when trying to use this method > on something not present at physical level

Re: [HACKERS] TABLESAMPLE patch

On 09/04/15 11:37, Simon Riggs wrote: On 9 April 2015 at 04:52, Simon Riggs wrote: TABLESAMPLE BERNOULLI could work in this case, or any other non-block based sampling mechanism. Whether it does work yet is another matter. This query should be part of the test suite and should generate a usef

Re: [HACKERS] TABLESAMPLE patch

On 9 April 2015 at 04:52, Simon Riggs wrote: > TABLESAMPLE BERNOULLI could work in this case, or any other non-block > based sampling mechanism. Whether it does work yet is another matter. > > This query should be part of the test suite and should generate a > useful message or work correctly. T

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 5:52 PM, Simon Riggs wrote: > On 9 April 2015 at 04:12, Michael Paquier wrote: > >> Also, I am wondering if the sampling logic based on block analysis is >> actually correct, for example for now this fails and I think that we >> should support it: >> =# with query_select as

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 5:12 PM, Michael Paquier wrote: > On Thu, Apr 9, 2015 at 4:30 AM, Jeff Janes wrote: >> On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek wrote: >>> >>> On 06/04/15 14:30, Petr Jelinek wrote: On 06/04/15 11:02, Simon Riggs wrote: > Are we ready for a final de

Re: [HACKERS] TABLESAMPLE patch

On 9 April 2015 at 04:12, Michael Paquier wrote: > Also, I am wondering if the sampling logic based on block analysis is > actually correct, for example for now this fails and I think that we > should support it: > =# with query_select as (select generate_series(1, 10) as a) select > query_select

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 4:30 AM, Jeff Janes wrote: > On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek wrote: >> >> On 06/04/15 14:30, Petr Jelinek wrote: >>> >>> On 06/04/15 11:02, Simon Riggs wrote: >>> Are we ready for a final detailed review and commit? >>> >>> I plan to send v12 in the

Re: [HACKERS] TABLESAMPLE patch

On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek wrote: > On 06/04/15 14:30, Petr Jelinek wrote: > >> On 06/04/15 11:02, Simon Riggs wrote: >> >> Are we ready for a final detailed review and commit? >>> >>> >> I plan to send v12 in the evening with some additional changes that came >> up from Amit'

Re: [HACKERS] TABLESAMPLE patch

On Mon, Apr 6, 2015 at 11:32 PM, Petr Jelinek wrote: > > On 06/04/15 14:30, Petr Jelinek wrote: >> >> On 06/04/15 11:02, Simon Riggs wrote: >> >>> Are we ready for a final detailed review and commit? >>> >> >> I plan to send v12 in the evening with some additional changes that came >> up from Amit

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 15:07, Amit Kapila wrote: On Mon, Apr 6, 2015 at 5:56 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > On 06/04/15 12:33, Amit Kapila wrote: >> >> >> But I think the Update on target table with sample scan is >> supported via views which doesn't seem to be the right thi

Re: [HACKERS] TABLESAMPLE patch

On Mon, Apr 6, 2015 at 5:56 PM, Petr Jelinek wrote: > > On 06/04/15 12:33, Amit Kapila wrote: >> >> >> But I think the Update on target table with sample scan is >> supported via views which doesn't seem to be the right thing >> in case you just want to support it via FROM/USING, example >> >> pos

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 11:02, Simon Riggs wrote: On 2 April 2015 at 17:36, Petr Jelinek wrote: so here is version 11. Looks great. Comment on docs: The SELECT docs refer only to SYSTEM and BERNOULLI. It doesn't mention that if other methods are available they could be used also. The phrasing was "sam

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 12:33, Amit Kapila wrote: On Sat, Apr 4, 2015 at 8:25 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > Yes I want extensibility here. And I think the tablesample method arguments are same thing as function arguments given that in the end they are arguments for the init func

Re: [HACKERS] TABLESAMPLE patch

On Sat, Apr 4, 2015 at 8:25 PM, Petr Jelinek wrote: > > On 04/04/15 14:57, Amit Kapila wrote: >> >> >> 1. >> +tablesample_clause: >> +TABLESAMPLE ColId '(' func_arg_list ')' opt_repeatable_clause >> >> It seems to me that you want to allow it to make it extendable >> to user defined Tablesample me

Re: [HACKERS] TABLESAMPLE patch

On 2 April 2015 at 17:36, Petr Jelinek wrote: > so here is version 11. Looks great. Comment on docs: The SELECT docs refer only to SYSTEM and BERNOULLI. It doesn't mention that if other methods are available they could be used also. The phrasing was "sampling method can be one of ." Are we re

Re: [HACKERS] TABLESAMPLE patch

On 04/04/15 14:57, Amit Kapila wrote: 1. +tablesample_clause: +TABLESAMPLE ColId '(' func_arg_list ')' opt_repeatable_clause Why do you want to allow func_arg_list? Basically if user tries to pass multiple arguments in TABLESAMPLE method's clause like (10,20), then I think that should be caug

Re: [HACKERS] TABLESAMPLE patch

On Fri, Apr 3, 2015 at 3:06 AM, Petr Jelinek wrote: > > Hi, > > so here is version 11. > Thanks, patch looks much better, but I think still few more things needs to discussed/fixed. 1. +tablesample_clause: + TABLESAMPLE ColId '(' func_arg_list ')' opt_repeatable_clause Why do you want to allow

Re: [HACKERS] TABLESAMPLE patch

On 01/04/15 18:38, Robert Haas wrote: On Wed, Apr 1, 2015 at 12:15 PM, Petr Jelinek wrote: REPEATABLE is mandated by standard. I did try for quite some time to make it unreserved but was not successful (I can only make it unreserved if I make it mandatory but that's not a solution). I haven't b

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 12:15 PM, Petr Jelinek wrote: > REPEATABLE is mandated by standard. I did try for quite some time to make it > unreserved but was not successful (I can only make it unreserved if I make > it mandatory but that's not a solution). I haven't been in fact even able to > find out

Re: [HACKERS] TABLESAMPLE patch

On 01/04/15 17:52, Robert Haas wrote: On Wed, Apr 1, 2015 at 9:49 AM, Amit Kapila wrote: I am still not sure whether it is okay to move REPEATABLE from unreserved to other category. In-fact last weekend I have spent some time to see the exact reason for shift/reduce errors and tried some ways

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 9:49 AM, Amit Kapila wrote: > I am still not sure whether it is okay to move REPEATABLE from > unreserved to other category. In-fact last weekend I have spent some > time to see the exact reason for shift/reduce errors and tried some ways > but didn't find a way to get away

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 6:31 PM, Tomas Vondra wrote: > > On 03/15/15 16:21, Petr Jelinek wrote: >> >> >> I also did all the other adjustments we talked about up-thread and >> rebased against current master (there was conflict with 31eae6028). >> > > Hi, > > I did a review of the version submitted o

Re: [HACKERS] TABLESAMPLE patch

On 03/15/15 16:21, Petr Jelinek wrote: I also did all the other adjustments we talked about up-thread and rebased against current master (there was conflict with 31eae6028). Hi, I did a review of the version submitted on 03/15 today, and only found a few minor issues: 1) The documentation

Re: [HACKERS] TABLESAMPLE patch

On 10/03/15 10:54, Amit Kapila wrote: On Tue, Mar 10, 2015 at 3:03 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > Ok now I think I finally understand what you are suggesting - you are saying let's go over whole page while tsmnexttuple returns something, and do the visibility check an

Re: [HACKERS] TABLESAMPLE patch

On Tue, Mar 10, 2015 at 3:03 PM, Petr Jelinek wrote: > > On 10/03/15 04:43, Amit Kapila wrote: >> >> On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek > > wrote: >> > >> > On 09/03/15 04:51, Amit Kapila wrote: >> >> >> >> On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek >

Re: [HACKERS] TABLESAMPLE patch

On 10/03/15 04:43, Amit Kapila wrote: On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > On 09/03/15 04:51, Amit Kapila wrote: >> >> On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek mailto:p...@2ndquadrant.com> >> > Double checking for tuple visibility is the

Re: [HACKERS] TABLESAMPLE patch

On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek wrote: > > On 09/03/15 04:51, Amit Kapila wrote: >> >> On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek > > Double checking for tuple visibility is the only downside I can think >> of. >> >> That will happen if we use heapgetpage and the way currently >>

Re: [HACKERS] TABLESAMPLE patch

On 09/03/15 04:51, Amit Kapila wrote: On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > On 05/03/15 09:21, Amit Kapila wrote: >> >> On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek mailto:p...@2ndquadrant.com> >>

Re: [HACKERS] TABLESAMPLE patch

On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek wrote: > > On 05/03/15 09:21, Amit Kapila wrote: >> >> On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek > > wrote: >> > >> > >> > I didn't add the whole page visibility caching as the tuple ids we >> get from sampling metho

Re: [HACKERS] TABLESAMPLE patch

On 05/03/15 09:21, Amit Kapila wrote: On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > > I didn't add the whole page visibility caching as the tuple ids we get from sampling methods don't map well to the visibility info we get from heapgetpage (it maps to

Re: [HACKERS] TABLESAMPLE patch

On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek wrote: > > > I didn't add the whole page visibility caching as the tuple ids we get from sampling methods don't map well to the visibility info we get from heapgetpage (it maps to the values in the rs_vistuples array not to to its indexes). Commented a

Re: [HACKERS] TABLESAMPLE patch

Hi, On 22.2.2015 18:57, Petr Jelinek wrote: > Tomas noticed that the patch is missing error check when TABLESAMPLE > is used on view, so here is a new version that checks it's only used > against table or matview. > > No other changes. Curious question - could/should this use page prefetch, sim

Re: [HACKERS] TABLESAMPLE patch

On 31/01/15 14:27, Amit Kapila wrote: On Fri, Jan 23, 2015 at 5:39 AM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: On 19/01/15 07:08, Amit Kapila wrote: On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek mailto:p...@2ndquadrant.com>

Re: [HACKERS] TABLESAMPLE patch

On Fri, Jan 23, 2015 at 5:39 AM, Petr Jelinek wrote: > On 19/01/15 07:08, Amit Kapila wrote: > >> On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek > > wrote: >> > > I think that's actually good to have, because we still do costing and the > partial index might help prod

Re: [HACKERS] TABLESAMPLE patch

On 1/29/15 10:44 AM, Bruce Momjian wrote: On Thu, Jan 29, 2015 at 11:08:55AM -0500, Robert Haas wrote: On Wed, Jan 28, 2015 at 5:19 AM, Petr Jelinek wrote: Yes, that's my view too. I would generally be for that change also and it would be worth it if the code was used in more than one place, b

Re: [HACKERS] TABLESAMPLE patch

On Thu, Jan 29, 2015 at 11:08:55AM -0500, Robert Haas wrote: > On Wed, Jan 28, 2015 at 5:19 AM, Petr Jelinek wrote: > > Yes, that's my view too. I would generally be for that change also and it > > would be worth it if the code was used in more than one place, but as it is > > it seems like it wil

Re: [HACKERS] TABLESAMPLE patch

On Wed, Jan 28, 2015 at 5:19 AM, Petr Jelinek wrote: > Yes, that's my view too. I would generally be for that change also and it > would be worth it if the code was used in more than one place, but as it is > it seems like it will just add code/complexity for no real benefit. It would > make sense

Re: [HACKERS] TABLESAMPLE patch

On 28/01/15 08:23, Kyotaro HORIGUCHI wrote: Hi, I took a look on this and found nice. By the way, the parameter for REPEATABLE seems allowing to be a expression in ParseTableSample but the grammer rejects it. It wasn't my intention to support it, but you are correct, the code is generic enou

Re: [HACKERS] TABLESAMPLE patch

On 28/01/15 09:41, Kyotaro HORIGUCHI wrote: As an issue related to size esmation, I got a explain result as following, =# explain (analyze on, buffers on) select a from t1 tablesample system(10) where a < 5; QUERY PLAN ---

Re: [HACKERS] TABLESAMPLE patch

Hello, > On 19/01/15 07:08, Amit Kapila wrote: > > On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek > > wrote: > > No issues, but it seems we should check other paths where > > different handling could be required for tablesample scan. > > In set_rel_size(), it uses nor

Re: [HACKERS] TABLESAMPLE patch

Hi, I took a look on this and found nice. By the way, the parameter for REPEATABLE seems allowing to be a expression in ParseTableSample but the grammer rejects it. The following change seems enough. diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 4578b5e..8cf09d5 100644

Re: [HACKERS] TABLESAMPLE patch

On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek wrote: > On 17/01/15 13:46, Amit Kapila wrote: > > >> 3. >> @@ -332,6 +334,11 @@ set_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, >> /* Foreign table */ >> set_foreign_pathlist(root, rel, rte); >> } >> +else if (rte->tablesample != NULL) >> +{ >>

Re: [HACKERS] TABLESAMPLE patch

On 17/01/15 13:46, Amit Kapila wrote: On Sun, Jan 11, 2015 at 1:29 AM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote: > > > In second patch which implements the TABLESAMPLE itself I changed the implementation of random generator because when I looked at the code again I realized the old one

Re: [HACKERS] TABLESAMPLE patch

On Sun, Jan 11, 2015 at 1:29 AM, Petr Jelinek wrote: > > > In second patch which implements the TABLESAMPLE itself I changed the implementation of random generator because when I looked at the code again I realized the old one would produce wrong results if there were multiple TABLESAMPLE statemen

Re: [HACKERS] TABLESAMPLE patch

On Fri, Jan 9, 2015 at 1:10 AM, Petr Jelinek wrote: > On 06/01/15 14:22, Petr Jelinek wrote: >> >> On 06/01/15 08:51, Michael Paquier wrote: >>> >>> On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek >>> wrote: Attached is v3 which besides the fixes mentioned above also includes changes

Re: [HACKERS] TABLESAMPLE patch

On 2015-01-06 14:22:16 +0100, Petr Jelinek wrote: > On 06/01/15 08:51, Michael Paquier wrote: > >On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek wrote: > >>Attached is v3 which besides the fixes mentioned above also includes changes > >>discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD

Re: [HACKERS] TABLESAMPLE patch

On 06/01/15 08:51, Michael Paquier wrote: On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek wrote: Attached is v3 which besides the fixes mentioned above also includes changes discussed with Tomas (except the CREATE/DROP TABLESAMPLE METHOD), fixes for crash with FETCH FIRST and is rebased against c

  1   2   >