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;