Re: WIP: Rework access method interface

On Fri, Oct 2, 2015 at 8:14 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>
> I agree about staying with one SQL-visible function.
>
> Other changes:
> * Documentation reflects interface changes.
> * IndexAmRoutine moved from CacheMemoryContext to indexcxt.
> * Various minor formatting improvements.
> * Error messages are corrected.
>

Few assorted comments:

1.
+ * Get IndexAmRoutine structure from access method oid.
+ */
+ IndexAmRoutine *
+ GetIndexAmRoutine(Oid
amoid)
+ {
+ IndexAmRoutine *result;
+ HeapTuple tuple;
+ regproc
amhandler;
+
+ tuple = SearchSysCache1(AMOID, ObjectIdGetDatum(amoid));
+ if (!HeapTupleIsValid
(tuple))
+ elog(ERROR, "cache lookup failed for access method %u",
+
amoid);
+ amhandler = ((Form_pg_am)GETSTRUCT(tuple))->amhandler;
+
+ 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.

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?

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.

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.

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