Re: WIP: Rework access method interface

On Sat, Oct 10, 2015 at 6:03 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> 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;
>
>
Good catch, this change was reverted.
Also, it was planned that documentation changes would be reviewed by native
english speaker.
Tom, could you take a look at them?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company