Re: WIP: Rework access method interface

On Mon, Aug 10, 2015 at 1:12 PM, Petr Jelinek <petr(at)2ndquadrant(dot)com> wrote:

> On 2015-08-09 23:56, Alexander Korotkov wrote:
>
>> Hacker,
>>
>> some time before I proposed patches implementing CREATE ACCESS METHOD.
>>
>> http://www.postgresql.org/message-id/CAPpHfdsXwZmojm6Dx+TJnpYk27kT4o7Ri6X_4OSWcByu1Rm+VA@mail.gmail.com
>> As I get from comments to my patches and also from Tom's comment about
>> AM interface in tablesampling thread – AM interface needs reworking. And
>> AFAICS AM interface rework is essential to have CREATE ACCESS METHOD
>> command.
>> http://www.postgresql.org/message-id/5438.1436740611@sss.pgh.pa.us
>>
>
> Cool, I was planning to take a stab at this myself when I have time, so I
> am glad you posted this.
>
> This is why I'd like to show a WIP patch implementing AM interface
>> rework. Patch is quite dirty yet, but I think it illustrated the idea
>> quite clear. AM now have single parameter – handler function. This
>> handler returns pointer to AmRoutine which have the same data as pg_am
>> had before. Thus, API is organized very like FDW.
>>
>>
> I wonder if it would make sense to call it IndexAmRoutine instead in case
> we add other AMs (like the sequence am, or maybe column store if that's
> done as AM) in the future.
>

Good point!

> The patch should probably add the am_handler type which should be return
> type of the am handler function (see fdw_handler and tsm_handler types).
>

Sounds reasonable!

> I also think that the CHECK_PROCEDUREs should be in the place of the
> original GET_REL_PROCEDUREs (just after relation check). If the interface
> must exist we may as well check for it at the beginning and not after we
> did some other work which is useless without the interface.

Ok, good point too.

However, this patch appears to take more work than I expected. It have
>> to do many changes spread among many files.
>>
>
> Yeah you need to change the definition and I/O handling of every AM
> function, but that's to be expected.
>

Yes, this is why I decided to get some feedback on this stage of work.

> Also, it appears not so easy
>> to hide amsupport into AmRoutine, because it's needed for relcache. As a
>> temporary solution it's duplicated in RelationData.
>>
>
> I don't understand this, there is already AmRoutine in RelationData, why
> the need for additional field for just amsupport?
>

We need amsupport in load_relcache_init_file() which reads
"pg_internal.init". I'm not sure this is correct place to call am_handler.
It should work in the case of built-in AM. But if AM is defined in the
extension then we wouldn't be able to do catalog lookup for am_handler on
this stage of initialization.

Another point is that amsupport and amstrategies are used for regression
tests of opclasses and opfamilies. Thus, we probably can keep them in pg_am.

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