Re: WIP: Access method extendability

On Wed, Feb 17, 2016 at 5:56 PM, Teodor Sigaev <teodor(at)sigaev(dot)ru> wrote:

> Next version of patch is attached:
>> * Documentation for CREATE ACCESS METHOD command is added.
>> * Refactoring and comments for bloom contrib.
>>
>
> Cool work, nice to see.
>
> Some comments
> 1 create-am.7.patch: function lookup_index_am_handler_func() why doesn't
> it emit error in case of emtpy input? If it checks signature of function and
> empty handler is not allowed then, seems, all checks about handler have to
> be moved in lookup_index_am_handler_func().
>

Fixed. Now it's assumed that lookup_index_am_handler_func() returns valid
function Oid.

2 create-am.7.patch: Comment near get_am_name() incorrectly mentions
> get_am_oid function
>

Fixed.

> 3 create-am.7.patch: get_am_name(Oid amOid, char amtype). Seems, amtype
> argument is overengineering. get_am_name() is used only in error messages
> and additional check in it will do nothing or even confuse user.
>

Fixed.

> 4 create-am.7.patch: Inconsistentcy with psql help. As I can see other
> code, it's forbidden to create access method without handler
> postgres=# \h create access method
> Command: CREATE ACCESS METHOD
> Description: define a new access method
> Syntax:
> CREATE ACCESS METHOD name
> TYPE INDEX
> [ HANDLER handler_function | NO HANDLER ]
>
> postgres=# create access method yyy type index no handler;
> ERROR: syntax error at or near "no"
> LINE 1: create access method yyy type index no handler;
>

It comes from inconsistency in docs. Fixed.

> 5 create-am.7.patch: file src/bin/pg_dump/pg_dump.h. Extra comma near
> DO_POLICY:
> *** 77,83 ****
> DO_POST_DATA_BOUNDARY,
> DO_EVENT_TRIGGER,
> DO_REFRESH_MATVIEW,
> ! DO_POLICY
> } DumpableObjectType;
>
> typedef struct _dumpableObject
> --- 78,84 ----
> DO_POST_DATA_BOUNDARY,
> DO_EVENT_TRIGGER,
> DO_REFRESH_MATVIEW,
> ! DO_POLICY,
> } DumpableObjectType;
>

Fixed.

> 6 generic-xlog.7.patch: writeDelta() Seems, even under USE_ASSERT_CHECKING
> define checking diff by its applying is to expensive. May be, let we use
> here GENERIC_WAL_DEBUG macros?
>

I decided not to introduce special macros for this. Now, this check is
enabled on WAL_DEBUG.

7 generic-xlog.7.patch: src/backend/access/transam/generic_xlog.c It's
> unclear for me, what does MATCH_THRESHOLD do or intended for? Pls, add
> comments here.
>

Explicit comments are added.

8 generic-xlog.7.patch: src/backend/access/transam/generic_xlog.c. Again,
> it's unclear for me, what is motivation of size of PageData.data?
>

Explicit comments are added.

> 9 generic-xlog.7.patch: GenericXLogRegister(), Seems, emitting an error if
> caller tries to register buffer which is already registered isn't good
> idea. In practice, say, SP-GIST, buffer variable is used and page could be
> easily get from buffer. Suppose, GenericXLogRegister() should not emit an
> error and ignore isnew flag if case of second registration of the same
> buffer.
>

Changed.

> 10 bloom-contrib.7.patch:
> blutils.c: In function 'initBloomState':
> blutils.c:128:20: warning: variable 'opaque' set but not used
> [-Wunused-but-set-variable]
> BloomPageOpaque opaque;
>

Fixed.

> 11 I'd really like to see regression tests (TAP-tests?) for replication
> with generic xlog.

TAP test for replication added to bloom contrib. This test run on
additional make target wal-check.

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