Re: WIP: Access method extendability

Hi!

Next revision of patches is attached.

On Wed, Mar 9, 2016 at 4:56 AM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> I went over the latest version of this, here are my notes.
>
> create-am.9:
>
> + case DO_ACCESS_METHOD:
> + snprintf(buf, bufsize,
> + "");
> + return;
>

Fixed.

> Missing the actual description.
>
> + appendPQExpBuffer(q, "CREATE ACCESS METHOD %s HANDLER %s;\n",
> + qamname, aminfo->amhandler);
>
> Generates invalid syntax (missing TYPE).
>

Fixed.

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

Fixed.

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

I leave it amtype for now.

> object_type_map record is missing, as is getObjectTypeDescription and
> getObjectIdentityParts support
>

Fixed.

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

Thank you! I used it as cheat sheet.

> generic-xlog.9:
> 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.
>

Yep, performance might be much less than with other resource managers. But
that seems to be the only way to do this in extension since we don't want
extensions to have redo callbacks. Also, this is the basic version. There
could be more advances in future. In the previous version I has more
effective handling of data shift inside page. But it was removed for the
sake of simplicity to increase chance to commit.

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

Besides formation of xlog record generic xlog also does copying page images
and atomic replacement of them inside critical section. For sure, it could
be done without generic xlog. But I found it useful for generic xlog to
support this since code of extension becomes much more simple and elegant.
But I can remove it if you insist.

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

Explanation was moved from .h to .c.

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