Re: WIP: Access method extendability

Hi!

Next version of patch is attached.

On Tue, Feb 2, 2016 at 4:09 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, failed
> Spec compliant: not tested
> Documentation: not tested
>
> There are currently no docs or unit tests. I suspect this patch is still
> WIP?
>

I hope to exit WIP state soon...

create-am.5.patch:
> General notes:
> Needs catversion bump.
>

Yes. I think catversion bump is committer responsibility and shouldn't be
included into patch.

> IndexQualInfo and GenericCosts have been moved to
> src/include/utils/selfuncs.h.
>

Yes, they have been moved in order to be accessed by custom index access
method.

METHOD becomes an unreserved keyword.
>

Seems to be inevitable if we want CREATE ACCESS METHOD command.

> generic-xlog.5.patch:
> generic_xlog.c: At least needs a bunch of explanatory comments, if not
> info in a README. Since I don't really understand what the design here is
> my review is just cursory.
>

Make detailed explanation of API in generic_xlog.h. Could be moved into
README if needed.

> static memoryMove() - seems like an overly-generic function name to me...
>

I've simplified generic xlog to just per byte comparison without support of
memory move. Now it would be much easier to commit. Support of memory move
could be added in the separate patch though.

> writeCopyFlagment(), writeMoveFlagment(): s/Fl/Fr/?
>

Fixed.

bloom-control.5:
> README:
> + CREATE INDEX bloomidx ON tbloom(i1,i2,i3)
> + WITH (length=5, col1=2, col2=2, col3=4);
> +
> + Here, we create bloom index with signature length 80 bits and attributes
> + i1, i2 mapped to 2 bits, attribute i3 - to 4 bits.
>
> It's not clear to me where 80 bits is coming from...
>

length is measure in uint16...
For now, it's documented.

> bloom.h:
> + #define BITBYTE (8)
> ISTR seeing this defined somewhere in the Postgres headers; dunno if it's
> worth using that definition instead.
>

Got rid of this. Using BITS_PER_BYTE now.

> Testing:
> I ran the SELECT INTO from the README, as well as CREATE INDEX bloomidx. I
> then ran
>
> insert into tbloom select
> (generate_series(1,1000)*random())::int as i1,
> (generate_series(1,1000)*random())::int as i2,
> (generate_series(1,1000)*random())::int as i3,
> (generate_series(1,1000)*random())::int as i4,
> (generate_series(1,1000)*random())::int as i5,
> (generate_series(1,1000)*random())::int as i6,
> (generate_series(1,1000)*random())::int as i7,
> (generate_series(1,1000)*random())::int as i8,
> (generate_series(1,1000)*random())::int as i9,
> (generate_series(1,1000)*random())::int as i10,
> (generate_series(1,1000)*random())::int as i11,
> (generate_series(1,1000)*random())::int as i12,
> (generate_series(1,1000)*random())::int as i13
> from generate_series(1,999);
>
> and kill -9'd the backend. After restart I did VACUUM VERBOSE without
> issue. I ran the INSERT INTO, kill -9 and VACUUM VERBOSE sequence again.
> This time I got an assert:
>
> TRAP: FailedAssertion("!(((bool) (((const void*)((ItemPointer) left) !=
> ((void*)0)) && (((ItemPointer) left)->ip_posid != 0))))", File:
> "vacuumlazy.c", Line: 1823)
>
> That line is
>
> lblk = ItemPointerGetBlockNumber((ItemPointer) left);
>
> which does
>
> AssertMacro(ItemPointerIsValid(pointer)), \
>
> which is the assert that's failing.
>
> I've squirreled this install away for now, in case you can't repro this
> failure.

Should be fixed.

General notes about current version of patch:
* A lot of comments added.
* bloom documentation is moved from README to SGML with a lot of addons and
cleanup.
* Memory move support in generic xlog is removed. Now it's much more simple
and clean.
* Tests for CREATE ACCESS METHOD added. For now, it creates a mirror of
GiST access method.
* Syntax for CREATE ACCESS METHOD is changed. For now, it's "CREATE ACCESS
METHOD amname TYPE INDEX HANDLER handler;" in respect of parallel work on
sequential access methods. New access method attribute added: amtype.

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