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 

Ouch good point, it does show how long the whole sequence am thing has been around.

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.

   seqam_gapless_init(Relation seqrel, Form_pg_sequence seq, int64 
                                 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:

Reply via email to