Re: WIP: Access method extendability

Jim Nasby <jim.nasby@bluetreble.com>

From: Jim Nasby <Jim.Nasby@BlueTreble.com>
To: pgsql-hackers@postgresql.org
Date: 2016-02-02T01:09:15Z
Lists: pgsql-hackers
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?

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

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

METHOD becomes an unreserved keyword.

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.

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

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

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

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.

Commits

  1. Support CREATE ACCESS METHOD

  2. Restructure index access method API to hide most of it at the C level.