From: | Jose Luis Tallon <jltallon(at)adv-solutions(dot)net> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Sequence Access Method WIP |
Date: | 2016-03-30 13:22:41 |
Message-ID: | 20160330132241.1315.90869.pgcf@coridan.postgresql.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(dot)GT2456(at)alap3(dot)anarazel(dot)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.
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2016-03-30 13:40:24 | Re: PATCH: index-only scans with partial indexes |
Previous Message | Shulgin, Oleksandr | 2016-03-30 13:22:36 | Re: unexpected result from to_tsvector |