Hi,
Thanks for review.
On 30/03/16 15:22, Jose Luis Tallon wrote:
[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)
Ouch good point, it does show how long the whole sequence am thing has
been around.
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)
Schema is needed for the handler function as well. In general I don't
want to add another internal schema that will be empty when no sequence
AM is installed.
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.
Not really, gapless just needs table for transactionality and as such is
an exception. It does not change how the nontransactional sequence
storage works though. I agree with Andres on this one.
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"
Sounds good.
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.
The current version of seqam is 0001-seqam-2016-03-29 which should apply
correctly.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
