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

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

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

Petr Jelinek p...@2ndquadrant.com 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

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

Petr Jelinek p...@2ndquadrant.com 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

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

On 2015-07-25 00:36, Tom Lane wrote: I wrote: Petr Jelinek p...@2ndquadrant.com 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,

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

Petr Jelinek p...@2ndquadrant.com 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

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 BeginSampleScan

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

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

Petr Jelinek p...@2ndquadrant.com 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

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 another

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

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

Petr Jelinek p...@2ndquadrant.com 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

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

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 seems high with

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

Petr Jelinek p...@2ndquadrant.com 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

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

On 19 July 2015 at 21:56, Tom Lane t...@sss.pgh.pa.us 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/ http://www.2ndquadrant.com/

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

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

On 2015-07-16 17:08, Tom Lane wrote: Petr Jelinek p...@2ndquadrant.com 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

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

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 we

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

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 whole thing

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

On 2015-07-16 15:59, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com 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

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

On Thu, Jul 16, 2015 at 10:33 PM, Alvaro Herrera alvhe...@2ndquadrant.com 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,

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

Amit Langote amitlangot...@gmail.com writes: On Thu, Jul 16, 2015 at 10:33 PM, Alvaro Herrera alvhe...@2ndquadrant.com 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

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

Petr Jelinek p...@2ndquadrant.com 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

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

On 13 July 2015 at 14:39, Tom Lane t...@sss.pgh.pa.us wrote: Michael Paquier michael.paqu...@gmail.com 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

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

On 14 July 2015 at 15:32, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: On 13 July 2015 at 14:39, Tom Lane t...@sss.pgh.pa.us 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

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

Simon Riggs si...@2ndquadrant.com writes: On 13 July 2015 at 14:39, Tom Lane t...@sss.pgh.pa.us 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

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 t...@sss.pgh.pa.us 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

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

On 15 July 2015 at 05:58, Noah Misch n...@leadboat.com 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 Mon, Jul 13, 2015 at 7:36 AM, Tom Lane t...@sss.pgh.pa.us 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

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

Michael Paquier michael.paqu...@gmail.com 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

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

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

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

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

On 13 July 2015 at 17:00, Tom Lane t...@sss.pgh.pa.us 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

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;

Re: [HACKERS] TABLESAMPLE patch

On 19/04/15 01:24, Michael Paquier wrote: On Sat, Apr 18, 2015 at 8:38 PM, Michael Paquier michael.paqu...@gmail.com 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.

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 23, 2015 at 4:31 AM, Petr Jelinek p...@2ndquadrant.com wrote: On 19/04/15 01:24, Michael Paquier wrote: On Sat, Apr 18, 2015 at 8:38 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Fri, Apr 17, 2015 at 10:54 PM, Petr Jelinek wrote: On 10/04/15 06:46, Michael Paquier

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 Sat, Apr 11, 2015 at 12:56 AM, Peter Eisentraut pete...@gmx.net 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

Re: [HACKERS] TABLESAMPLE patch

On 10 April 2015 at 15:26, Peter Eisentraut pete...@gmx.net 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

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.

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

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

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 5:12 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Thu, Apr 9, 2015 at 4:30 AM, Jeff Janes jeff.ja...@gmail.com wrote: On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek p...@2ndquadrant.com wrote: On 06/04/15 14:30, Petr Jelinek wrote: On 06/04/15 11:02, Simon

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 si...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 5:52 PM, Simon Riggs si...@2ndquadrant.com wrote: On 9 April 2015 at 04:12, Michael Paquier michael.paqu...@gmail.com 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

Re: [HACKERS] TABLESAMPLE patch

On 9 April 2015 at 04:52, Simon Riggs si...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Thu, Apr 9, 2015 at 4:30 AM, Jeff Janes jeff.ja...@gmail.com wrote: On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 9 April 2015 at 04:12, Michael Paquier michael.paqu...@gmail.com 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)

Re: [HACKERS] TABLESAMPLE patch

On Mon, Apr 6, 2015 at 11:02 AM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Sat, Apr 4, 2015 at 8:25 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 2 April 2015 at 17:36, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 12:33, Amit Kapila wrote: On Sat, Apr 4, 2015 at 8:25 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 15:07, Amit Kapila wrote: On Mon, Apr 6, 2015 at 5:56 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Mon, Apr 6, 2015 at 5:56 PM, Petr Jelinek 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 thing in case you just want to support it via FROM/USING, example

Re: [HACKERS] TABLESAMPLE patch

On 06/04/15 11:02, Simon Riggs wrote: On 2 April 2015 at 17:36, Petr Jelinek p...@2ndquadrant.com 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.

Re: [HACKERS] TABLESAMPLE patch

On Fri, Apr 3, 2015 at 3:06 AM, Petr Jelinek p...@2ndquadrant.com 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

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

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

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 6:31 PM, Tomas Vondra tomas.von...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 01/04/15 17:52, Robert Haas wrote: On Wed, Apr 1, 2015 at 9:49 AM, Amit Kapila amit.kapil...@gmail.com 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

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 9:49 AM, Amit Kapila amit.kapil...@gmail.com 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

Re: [HACKERS] TABLESAMPLE patch

On 01/04/15 18:38, Robert Haas wrote: On Wed, Apr 1, 2015 at 12:15 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Wed, Apr 1, 2015 at 12:15 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 10/03/15 10:54, Amit Kapila wrote: On Tue, Mar 10, 2015 at 3:03 PM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On 10/03/15 04:43, Amit Kapila wrote: On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek p...@2ndquadrant.com 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 p...@2ndquadrant.com mailto:p...@2ndquadrant.com Double

Re: [HACKERS] TABLESAMPLE patch

On Tue, Mar 10, 2015 at 3:03 PM, Petr Jelinek p...@2ndquadrant.com wrote: On 10/03/15 04:43, Amit Kapila wrote: On Mon, Mar 9, 2015 at 3:08 PM, Petr Jelinek p...@2ndquadrant.com mailto:p...@2ndquadrant.com wrote: On 09/03/15 04:51, Amit Kapila wrote: On Sat, Mar 7, 2015 at 10:37

Re: [HACKERS] TABLESAMPLE patch

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

Re: [HACKERS] TABLESAMPLE patch

On 09/03/15 04:51, Amit Kapila wrote: On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek p...@2ndquadrant.com 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 p...@2ndquadrant.com mailto:p...@2ndquadrant.com

Re: [HACKERS] TABLESAMPLE patch

On Sat, Mar 7, 2015 at 10:37 PM, Petr Jelinek p...@2ndquadrant.com wrote: On 05/03/15 09:21, Amit Kapila wrote: On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek p...@2ndquadrant.com mailto:p...@2ndquadrant.com wrote: I didn't add the whole page visibility caching as the tuple ids we

Re: [HACKERS] TABLESAMPLE patch

On 05/03/15 09:21, Amit Kapila wrote: On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Tue, Feb 17, 2015 at 3:29 AM, Petr Jelinek 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 the values in the rs_vistuples array not to to its

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, similar

Re: [HACKERS] TABLESAMPLE patch

On Fri, Jan 23, 2015 at 5:39 AM, Petr Jelinek p...@2ndquadrant.com wrote: On 19/01/15 07:08, Amit Kapila wrote: On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek p...@2ndquadrant.com mailto:p...@2ndquadrant.com wrote: I think that's actually good to have, because we still do costing and the

Re: [HACKERS] TABLESAMPLE patch

On 31/01/15 14:27, Amit Kapila wrote: On Fri, Jan 23, 2015 at 5:39 AM, Petr Jelinek p...@2ndquadrant.com 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 p...@2ndquadrant.com mailto:p...@2ndquadrant.com

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 p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Wed, Jan 28, 2015 at 5:19 AM, Petr Jelinek p...@2ndquadrant.com 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

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 p...@2ndquadrant.com 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

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 p...@2ndquadrant.com mailto:p...@2ndquadrant.com 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

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

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

Re: [HACKERS] TABLESAMPLE patch

On Sun, Jan 18, 2015 at 12:46 AM, Petr Jelinek p...@2ndquadrant.com 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 p...@2ndquadrant.com 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

Re: [HACKERS] TABLESAMPLE patch

On Sun, Jan 11, 2015 at 1:29 AM, Petr Jelinek 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 would produce wrong results if there were multiple

Re: [HACKERS] TABLESAMPLE patch

On Fri, Jan 9, 2015 at 1:10 AM, Petr Jelinek p...@2ndquadrant.com 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 p...@2ndquadrant.com wrote: Attached is v3 which besides the fixes mentioned above also

Re: [HACKERS] TABLESAMPLE patch

On Tue, Dec 23, 2014 at 5:21 AM, Petr Jelinek p...@2ndquadrant.com 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 current master. This

Re: [HACKERS] TABLESAMPLE patch

On 21/12/14 18:38, Tomas Vondra wrote: Hi, On 18.12.2014 13:14, Petr Jelinek wrote: Hi, v2 version of this patch is attached. I did a review of this v2 patch today. I plan to do a bit more testing, but these are my comments/questions so far: Thanks for looking at it! (0) There's a

Re: [HACKERS] TABLESAMPLE patch

On 22.12.2014 10:07, Petr Jelinek wrote: On 21/12/14 18:38, Tomas Vondra wrote: (1) The patch adds a new catalog, but does not bump CATVERSION. I thought this was always done by committer? Right. Sorry for the noise. (2) The catalog naming (pg_tablesamplemethod) seems a bit awkward,

Re: [HACKERS] TABLESAMPLE patch

On Thu, Dec 18, 2014 at 7:14 AM, Petr Jelinek p...@2ndquadrant.com wrote: Hi, v2 version of this patch is attached. a few more tests revealed that passing null as the sample size argument works, and it shouldn't. in repeatable it gives an error if i use null as argument but it gives a syntax

Re: [HACKERS] TABLESAMPLE patch

On 22/12/14 20:14, Jaime Casanova wrote: On Thu, Dec 18, 2014 at 7:14 AM, Petr Jelinek p...@2ndquadrant.com wrote: Hi, v2 version of this patch is attached. a few more tests revealed that passing null as the sample size argument works, and it shouldn't. Fixed. in repeatable it gives an

Re: [HACKERS] TABLESAMPLE patch

Hi, On 18.12.2014 13:14, Petr Jelinek wrote: Hi, v2 version of this patch is attached. I did a review of this v2 patch today. I plan to do a bit more testing, but these are my comments/questions so far: (0) There's a TABLESAMPLE page at the wiki, not updated since 2012:

Re: [HACKERS] TABLESAMPLE patch

On Mon, Dec 22, 2014 at 2:38 AM, Tomas Vondra t...@fuzzy.cz wrote: (1) The patch adds a new catalog, but does not bump CATVERSION. FWIW, this part is managed by the committer when this patch is picked up. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make

  1   2   >