Re: WIP: Access method extendability

On Wed, Feb 17, 2016 at 5:56 PM, Teodor Sigaev <teodor@sigaev.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