Re: Sequence Access Method WIP - Mailing list pgsql-hackers

I've been looking at these patches a bit and here are some comments:

Patch 1: seqam

I would like to see an example in the docs for CREATE SEQUENCE.  That's
perhaps not possible (or desirable) with only the "local" seqam?  Not sure.

In the docs for pg_class, there is no mention that relam refers to
pg_seqam for sequences but pg_am for all other types.

There are errant half-sentences in the documentation, for example "to
the options in the CREATE SEQUENCE or ALTER SEQUENCE statement." in
Sequence Access Method Functions.

I'd prefer a README instead of the long comment at the start of seqam.c.The other ams do that.

As mentioned upthread, this patch isn't a seamless replacement for
what's already there because of the amdata field.  I wasn't part of the
conversation of FOSDEM unfortunately, and there's not enough information
in this thread to know why this solution is preferred over each seqam
having its own table type with all the columns it needs.  I see that
Heikki is waffling a bit between the two, and I have a fairly strong
opinion that amdata should be split into separate columns.  The patch
already destroys and recreates what it needs when changing access method
via ALTER SEQUENCE, so I don't really see what the problem is.

There is no psql tab completion for the new USING clause in ALTER
SEQUENCE, and in looking at that I noticed we don't have tab completion
for CREATE SEQUENCE at all.  I know we don't complete everything, but if
we're going to do ALTER, I think we should do CREATE.  I'll submit a
patch for that on its own thread, but then this patch will need to
modify it to include USING.

There is no \d command for sequence access methods.  Without querying
pg_seqam directly, how does one discover what's available?

There are some unfinished comments, such as "When relam is specified,
record dependency on the" in heap.c.

Comments and code in pg_dump.c check that the server is at least 9.5.
Those'll need to be changed to at least 9.6.

Patch 2: seqam ddl

When defining a new access method for sequences, it is possible to list
the arguments multiple times (last one wins).  Other defel loops raise
an error if the argument is specified more than once.  I haven't looked
at all of such loops to see if this is the only odd man out or not, but
I prefer the error behavior.

It doesn't seem possible to create a comment for a seqam, is that
intentional?  Ah, after more review I see it is possible, just not
documented.  I think that needs to be corrected.

Same comment as above about testing for 9.5.

Patch 3: gapless_seq

I really like the idea of having a gapless sequence in contrib.
Although it has big potential to be abused, doing it manually when it's
needed (like for invoices, at least in France) is a major pain.  So big
+1 for including this.

However, the patch doesn't update the main contrib Makefile so you have
to compile it explicitly.  It also doesn't have the .sgml file in the
right place, so that's not installed either.

There is a FIXME in get_last_value_tup() which should probably be
removed in light of the code comment in catcache.c stating that pg_seqam
is too small to benefit from an index scan.


It would be nice to be able to specify an access method when declaring a
serial or bigserial column.  This could be a separate patch later on,
though.


On the whole, I think this is a pretty good patchset.  Aside from the
design decision of whether amdata is a single opaque column or a set of
columns, there are only a few things that need to be changed before it's
ready for committer, and those things are mostly documentation.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support