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().
2 create-am.7.patch: Comment near get_am_name() incorrectly mentions get_am_oid
function
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.
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;
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;
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?
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.
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?
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.
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;
11 I'd really like to see regression tests (TAP-tests?) for replication with
generic xlog.
--
Teodor Sigaev E-mail: teo...@sigaev.ru
WWW: http://www.sigaev.ru/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
