Re: Custom Scan APIs (Re: Custom Plan node)

> On 24 March 2014 10:25, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>
> > Brief summary of the current approach that has been revised from my
> > original submission through the discussion on pgsql-hackers:
> >
> > The plannode was renamed to CustomPlan, instead of CustomScan, because
> > it dropped all the hardcoded portion that assumes the custom-node
> > shall perform as alternative scan or join method, because it prevents
> > this custom-node to perform as other stuff; like sort or append
> potentially.
> > According to the suggestion by Tom, I put a structure that contains
> > several function pointers on the new CustomPlan node, and extension
> > will allocate an object that extends CustomPlan node.
> > It looks like polymorphism in object oriented programming language.
> > The core backend knows abstracted set of methods defined in the tables
> > of function pointers, and extension can implement its own logic on the
> > callback, using private state on the extended object.
>
> I just wanted to add some review comments here. I also apologise for not
> reviewing this earlier; I had misunderstood the maturity of the patch and
> had assumed it was a request for comments/WIP.
>
Thanks for your interest and many comments.

> Overall, I very much support the concept of providing for alternate scans.
> I like the placement of calls in the optimizer and we'll be able to do much
> with that. Other comments in order that I consider them important.
>
> * There is no declarative structure for this at all. I was expecting to
> see a way to declare that a specific table might have an alternate scan
> path, but we just call the plugin always and it has to separately make a
> cache lookup to see if anything extra is needed. The Index AM allows us
> to perform scans, yet indexes are very clearly declared and easily and
> clearly identifiable. We need the same thing for alternate plans.
>
> * There is no mechanism at all for maintaining other data structures.
> Are we supposed to use the Index AM? Triggers? Or? The lack of clarity there
> is disturbing, though I could be simply missing something big and obvious.
>
> * There is no catalog support. Complex APIs in Postgres typically have a
> structure like pg_am which allows the features to be clearly identified.
> I'd be worried about our ability to keep track of so many calls in such
> pivotal places without that. I want to be able to know what a plugin is
> doing, especially when it will likely come in binary form. I don't see an
> easy way to have plugins partially override each other or work together.
> What happens when I want to use Mr.X's clever new join plugin at the same
> time as Mr.Y's GPU accelerator?
>
It was a choice on implementation. I just followed usual PG's hook manner;
that expects loaded extension saves the original function pointer and
has secondary call towards the function on its invocation. Thus, it needs
to walk on chain of extensions if multiple custom providers are loaded.

Even though I initially chose this design, it is an option to have catalog
support to track registered custom-scan providers and its metadata; what
function generate paths, what flags are turned on or what kind of relations
are supported...etc. Probably, optimizer skip some extensions that don't
support the target relation obviously.

> * How do we control security? What stops the Custom Scan API from overriding
> privileges? Shouldn't the alternate data structures be recognised as
> objects so we can grant privileges? Or do we simply say if an alternate
> data structure is linked to a heap then has implied privileges. It would
> be a shame to implement better security in one patch and then ignore it
> in another (from the same author).
>
In general, we have no mechanism to prevent overriding privilege mechanism
by c-binary extensions. Extension can override (existing) hooks and modify
requiredPerms bits of RangeTblEntry; that eventually cause privilege bypass.
But it is neutral for custom-scan API itself. Even though we implements
an alternative scan logic on the API, the core backend still checks required
privileges on ExecCheckRTPerms being called on the head of executor (unless
author of extension does not do something strange).

> All of the above I can let pass in this release, but in the longer term
> we need to look for more structure around these ideas so we can manage and
> control what happens. The way this is now is quite raw - suitable for R&D
> but not for longer term production usage by a wider audience, IMHO. I
> wouldn't like to make commitments about the longevity of this API either;
> if we accept it, it should have a big "may change radically" sign hanging
> on it. Having said that, I am interested in progress here and I accept that
> things will look like this at this stage of the ideas process, so these
> are not things to cause delay.
>
> Some things I would like to see change on in this release are...
>
> * It's not clear to me when we load/create the alternate data structures.
> That can't happen at _init time. I was expecting this to look like an
> infrastructure for unlogged indexes, but it doesn't look like that either.
>
I expected *_preload_libraries GUCs to load extensions.
If we have catalog support, extension shall be loaded prior to the first
invocation when optimizer asks the registered one capability of alternative
scan. I love the idea.

> * The name Custom makes me nervous. It sounds too generic, as if the design
> or objectives for this is are a little unclear. AlternateScan sounds like
> a better name since its clearer that we are scanning an alternate data
> structure rather than the main heap.
>
I don't have special preference on its name.

> * The prune hook makes me feel very uneasy. It seems weirdly specific
> implementation detail, made stranger by the otherwise lack of data
> maintenance API calls. Calling that for every dirty page sounds like an
> issue and my patch rejection indicator is flashing red around that.
>
All I want to do is cache-invalidation on the timing when vacuum is
running, but the proposed prune hook might not be an only answer.
In case when extension manages its cache data structure, which way
can we have to invalidate it? I never stick on existing proposition.

> Two additional use cases I will be looking to explore will be
>
> * Code to make Mat Views recognised as alternate scan targets
> * Code to allow queries access to sampled data rather the fully detailed
> data, if the result would be within acceptable tolerance for user
>
Let me investigate how to implement it. Probably, the idea around
materialized-view is more simple to do.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>