Re: WIP: Rework access method interface

On Sun, Oct 4, 2015 at 4:27 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sat, Oct 3, 2015 at 5:07 PM, Petr Jelinek <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>> 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.

>
>>> Few assorted comments:
>>>
>>> 1.
>>> + * Get IndexAmRoutine structure from access method oid.
>>> + */
>>> + IndexAmRoutine *
>>> + GetIndexAmRoutine(Oid
>>> amoid)
>>> ...
>>> + if (!RegProcedureIsValid
>>> (amhandler))
>>> + elog(ERROR, "invalid %u regproc", amhandler);
>>>
>>> I have noticed that currently, the above kind of error is reported
>>> slightly
>>> differently as in below code:
>>> if (!RegProcedureIsValid(procOid)) \
>>> elog(ERROR, "invalid %s regproc", CppAsString
>>> (pname)); \
>>>
>>> If you feel it is better to do the way as it is in current code, then you
>>> can change accordingly.
>>>
>>
>> It's completely different use-case from existing code. And tbh I think it
>> should have completely different and more informative error message
>> something in the style of "index access method %s does not have a handler"
>> (see for example GetFdwRoutineByServerId or transformRangeTableSample how
>> this is handled for similar cases currently).
>>
>>
> makes sense to me, but in that case isn't it better to use ereport
> (as used in GetFdwRoutineByServerId()) rather than elog?
>

Changed to ereport.

> This however brings another comment - I think it would be better if the
>> GetIndexAmRoutine would be split into two interfaces. The GetIndexAmRoutine
>> itself would accept the amhandler Oid and should just do the
>> OidFunctionCall and then check the result is not NULL and possibly that it
>> is an IndexAmRoutine node. And then all the
>>
>> (IndexAmRoutine*)DatumGetPointer(!OidFunctionCall0(accessMethodForm->amhandler));
>> calls in the code should be replaced with calls to the GetIndexAmRoutine
>> instead.
>>
>> The other routine (let's call it GetIndexAmRoutineByAmId for example)
>> would get IndexAmRoutine from amoid by looking up catalog, doing that
>> validation of amhandler Oid/regproc and calling the GetIndexAmRoutine.
>>
>>
> +1, I think that will make this API's design closer to what we have
> for corresponding FDW API.
>

Good, I've changed interface.

2.
> <para>
> Access methods that always return entries in the natural ordering
> of their data (such
> as btree) should set
> ! <structname>pg_am</>.<structfield>amcanorder</> to true.
> Currently, such
> access methods must use btree-compatible strategy
> numbers for their equality and ordering operators.
>
> </para>
> --- 545,551 ----
> <para>
> Access methods that always return entries in the natural
> ordering
> of their data (such as btree) should set
> ! <structfield>amcanorder</> to true.
>
> Currently, such access methods must use btree-compatible strategy
> numbers for their equality and
> ordering operators.
>
> Isn't it better to use structure while referencing the field of it?
>

Done.

> 3.
> ! Handler function must be written in a compiled language such as C,
> using
> ! the version-1 interface.
> Here, it is not completely clear, what do you refer to as version-1
> interface.
>

As, Peter commented upthread it is the same in FDW and we should change
both places if needed.
It refers to version 1 calling convention for C-function.
http://www.postgresql.org/docs/devel/static/xfunc-c.html#AEN57330
However, I'm not sure that it can't be version 0 calling convention. It
probably could work, but nobody use it.

> 4.
> xindex.sgml
> <title>Index Methods and Operator Classes</title>
> ..
> It is possible to add a
> new index method by defining the required interface routines and
> then creating a row in <classname>pg_am</classname> &mdash; but that is
> beyond the scope of this chapter (see <xref linkend="indexam">).
> </para>
> I think changing above to indicate something about handler function
> could be useful.
>

Done.

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