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

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

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

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.

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

This seems to be copy paste from fdw docs, if we decide this should be 
explained differently then it should be explained differently there as well.

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

+1

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