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 extensions. > > I had some thoughts about how we might fix that, without going to the > rather tedious lengths of creating a complete set of DDL infrastructure > for CREATE/DROP TABLESAMPLE METHOD. > > First off, the extension API designed for the tablesample patch is > evidently modeled on the index AM API, which was not a particularly good > precedent --- it's not a coincidence that index AMs can't be added or > dropped on-the-fly. Modeling a server internal API as a set of > SQL-visible functions is problematic when the call signatures of those > functions can't be accurately described by SQL datatypes, and it's rather > pointless and inefficient when none of the functions in question is meant > to be SQL-callable. It's even less attractive if you don't think you've > got a completely stable API spec, because adding, dropping, or changing > signature of any one of the API functions then involves a pile of > easy-to-mess-up catalog changes on top of the actually useful work. > Not to mention then having to think about backwards compatibility of > your CREATE command's arguments. > > 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 to all the other functions > that the particular wrapper needs to supply (and, if necessary, the struct > could have non-function-pointer fields containing other info the core > system might need to know about the handler). We could similarly invent a > pseudotype "tablesample_handler" and represent each tablesample method by > a single SQL function returning tablesample_handler. Any future changes > in the API for tablesample handlers would then appear as changes in the C > definition of the struct returned by the handler, which requires no > SQL-visible thrashing, hence creates no headaches for pg_upgrade, and it > is pretty easy to design it so that failure to update an external module > that implements the API results in C compiler errors and/or sane fallback > behavior.
That's elegant. > Once we've done that, I think we don't even need a special catalog > representing tablesample methods. Given "FROM foo TABLESAMPLE > bernoulli(...)", the parser could just look for a function bernoulli() > returning tablesample_handler, and it's done. The querytree would have an > ordinary function dependency on that function, which would be sufficient > 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.) > > Thoughts? 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. When looking at this patch I was as well surprised that the BERNOUILLI method can only be applied on a physical relation and was not able to fire on a materialized set of tuples, say the result of a WITH clause for example. > 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 ought to consider doing that. Any API redesign looks to be clearly 9.6 work IMO at this point. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
