Re: Sequence Access Method WIP

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(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.

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