Re: WIP: Rework access method interface

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.

>
>> 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?

> 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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com