Re: [HACKERS] Sequence Access Method WIP
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
[Partial review] Evaluated: 0002-gapless-seq-2016-03-29-2.patch
Needs updating code copyright years ... or is this really from 2013? [
contrib/gapless_seq/gapless_seq.c ]
Patch applies cleanly to current master
(3063e7a84026ced2aadd2262f75eebbe6240f85b)
It does compile cleanly.
DESIGN
The decision to hardcode the schema GAPLESS_SEQ_NAMESPACE ("gapless_seq") and
VALUES_TABLE_NAME ("seqam_gapless_values") strikes me a bit: while I understand
the creation of a private schema named after the extension, it seems overkill
for just a single table.
I would suggest to devote some reserved schema name for internal implementation
details and/or AM implementation details, if deemed reasonable.
On the other hand, creating the schema/table upon extension installation makes
the values table use the default tablespace for the database, which can be good
for concurrency --- direct writes to less loaded storage
(Note that users may want to move this table into an SSD-backed tablespace
or equivalently fast storage ... moreso when many --not just one-- gapless
sequences are being used)
Yet, there is <20141203102425.gt2...@alap3.anarazel.de> where Andres argues
against anything different than one-page-per-sequence implementations.
I guess this patch changes everything in this respect.
CODE
seqam_gapless_init(Relation seqrel, Form_pg_sequence seq, int64 restart_value,
bool restart_requested, bool is_init)
-> is_init sounds confusing; suggest renaming to "initialize" or "initial" to
avoid reading as "is_initIALIZED"
DEPENDS ON 0001-seqam-v10.patch , which isn't commited yet --- and it doesn't
apply cleanly to current git master.
Please update/rebase the patch and resubmit.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Access method extendability
Referenced by commit commit 473b93287040b20017cc25a157cffdc5b978c254 ("Support
CREATE ACCESS METHOD"), commited by alvherre
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Default Roles
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed * Applies cleanly to current master (3063e7a84026ced2aadd2262f75eebbe6240f85b) * ./configure && make -j4 ok DOCUMENTATION * Documentation ok, both code (code comments) and docs. * Documentation covers signalling backends/backup/monitor as well as the obvious modification to the role sections CODE * Checks on roles are fairly comprehensive: restrict reserved rolenames (creation/modification), prohibit granting to reserved roles * The patch is surprisingly non-intrusive/self-contained considering the functionality. TOOLS * Covers pg_upgrade -- "/* 9.5 and below should not have roles starting with pg_ */" * Covers pg_dumpall (do not export creation of system-reserved roles) * Includes support in psql (\dgS) + accompanying documentation REGRESSION TESTS * Includes regression tests; Seem quite complete (including GRANT/REVOKE on reserved roles) Suggestion for committer: add regression tests for each reserved role? (just for completeness' sake) * make installcheck-world passes; build on amd64 / gcc4.9.2 (Debian 4.9.2-10) - btree_gin tests fail / no contrib installed; Assumed ok * Nitpick: tests mention the still nonexistant pg_monitor / pg_backup reserved roles ; Might as well use some obviously reserved-but-absurd rolename instead Comment for future enhancement: might make sense to split role checking/access control functionality into a separate module, as opposed to having to include pg_authid.h everywhere I'm Thinking about Michael and Heikki's upcoming authentication revamp re. SCRAM/multiple authenticators: authentication != authorization (apropos "has_privs_of_role()" ) Testing: - pg_signal_backend Ok Looking forward to seeing the other proposed default roles in! The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
