Re: 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 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.
>

(back from vacation, going over this thread)

Yes this sounds very sane (and we use something similar also for logical
decoding plugins, not just FDWs). I wish this has occurred to me before,
I would not have to spend time on the DDL support which didn't even get in.

> 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

It seems possible that we might not need catalog indeed.

This would also simplify the parser part which currently contains
specialized function search code as we could most likely just reuse the
generic code.

> 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 the system() so yeah we'd
need some kind of differentiator.

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

+1

I think this is very relevant to the proposed sequence am patch as well.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services