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