Re: WIP: Rework access method interface - Mailing list pgsql-hackers

On 2015-10-05 19:25, Alexander Korotkov wrote:

On Sun, Oct 4, 2015 at 4:27 PM, Amit Kapila <amit.kapila16@gmail.com
<mailto:amit.kapila16@gmail.com>> wrote:

    On Sat, Oct 3, 2015 at 5:07 PM, Petr Jelinek <petr@2ndquadrant.com
    <mailto:petr@2ndquadrant.com>> wrote:

        On 2015-10-03 08:27, Amit Kapila wrote:

            On Fri, Oct 2, 2015 at 8:14 PM, Alexander Korotkov
            <a.korotkov@postgrespro.ru
            <mailto:a.korotkov@postgrespro.ru>
            <mailto:a.korotkov@postgrespro.ru

            <mailto:a.korotkov@postgrespro.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;