Re: WIP: Rework access method interface

On 2015-10-05 19:25, Alexander Korotkov wrote:
> On Sun, Oct 4, 2015 at 4:27 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com
> <mailto:amit(dot)kapila16(at)gmail(dot)com>> wrote:
>
> On Sat, Oct 3, 2015 at 5:07 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com
> <mailto:petr(at)2ndquadrant(dot)com>> wrote:
>
> On 2015-10-03 08:27, Amit Kapila wrote:
>
> On Fri, Oct 2, 2015 at 8:14 PM, Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)ru
> <mailto:a(dot)korotkov(at)postgrespro(dot)ru>
> <mailto:a(dot)korotkov(at)postgrespro(dot)ru
> <mailto:a(dot)korotkov(at)postgrespro(dot)ru>>> wrote:
> >
> >
> > I agree about staying with one SQL-visible function.
>
>
> Okay, this does not necessarily mean there should be only one
> validation function in the C struct though. I wonder if it would
> be more future proof to name the C interface as something else
> than the current generic amvalidate. Especially considering that
> it basically only does opclass validation at the moment (It's
> IMHO saner in terms of API evolution to expand the struct with
> more validator functions in the future compared to adding
> arguments to the existing function).
>
>
> I also agree with you that adding more arguments in future might
> not be a good idea for exposed API. I don't know how much improvement
> we can get if we use structure and then keep on adding more members
> to it based on future need, but atleast that way it will be less
> prone to
> breakage.
>
> I think adding multiple validator functions is another option, but that
> also doesn't sound like a good way as it can pose difficulty in
> understanding the right version of API to be used.
>
> I think the major priority is to keep compatibility. For now, user can
> easily define invalid opclass and he will just get the error runtime.
> Thus, the opclass validation looks like improvement which is not
> strictly needed. We can add new validator functions in the future but
> make them not required. Thus, old access method wouldn't loose
> compatibility from this.

Yes that was what I was thinking as well. We don't want to break
anything in this patch as it's mainly API change, but we want to have
API that can potentially evolve. I think evolving the API by adding more
interfaces in the *Routine struct so far worked well for the FDW for
example and given that those structs are nodes, the individual pointers
get initialized to NULL automatically so it's easy to add optional
interfaces (like validators) without breaking anything. Besides, it's
not unreasonable to expect that custom AM authors will have to check if
their implementation is compatible with new major version.

But this is also why I don't think it's good idea to call the opclass
validator just "amvalidate" in the IndexAmRoutine struct because it
implies to be the only validate function we'll ever have.

Other than the above gripe and the following spurious change, the patch
seems good to me now.

> RelationInitIndexAccessInfo(Relation relation)
> {
> HeapTuple tuple;
> - Form_pg_am aform;
> Datum indcollDatum;
> Datum indclassDatum;
> Datum indoptionDatum;
> @@ -1178,6 +1243,7 @@ RelationInitIndexAccessInfo(Relation relation)
> MemoryContext oldcontext;
> int natts;
> uint16 amsupport;
> + Form_pg_am aform;

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