I went over the latest version of this, here are my notes.


+               case DO_ACCESS_METHOD:
+                       snprintf(buf, bufsize,
+                                        "");
+                       return;

Missing the actual description.

+       appendPQExpBuffer(q, "CREATE ACCESS METHOD %s HANDLER %s;\n",
+                                         qamname, aminfo->amhandler);

Generates invalid syntax (missing TYPE).

I don't like that you hardcode 'i' everywhere in the code as am type index. It would be better to define it in pg_am.h and use that.

I was also debating in my head if amtype is correct name as it maps to relkind so amkind might be better name for the column, otoh then it won't map to the syntax we have agreed on for CREATE ACCESS METHOD so I guess there is no ideal name here.

object_type_map record is missing, as is getObjectTypeDescription and getObjectIdentityParts support

(you can check what I sent as part of sequence access methods where I fixed these things, otherwise it reuses most of your code)

Not much to say here, the api makes sense to me, it will have performance impact but don't see how we can do this generically without callbacks to extension in any better way.

One thing I don't understand is why there is support for unlogged relations. Shouldn't that be handled by whoever is using this to actually not call this at all? It's just call to RelationNeedsWAL() nothing to hard and hidden magic like not doing anything with WAL for the unlogged tables is seldomly good idea.

Another small thing is that we put the API explanation comments into .c file not .h file.

Didn't look at the bloom index too deeply yet.

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to