Re: Sequence Access Method WIP

On 15.11.2013 20:21, Andres Freund wrote:
> On 2013-11-15 20:08:30 +0200, Heikki Linnakangas wrote:
>> It's pretty hard to review the this without seeing the "other"
>> implementation you're envisioning to use this API. But I'll try:
>
> We've written a distributed sequence implementation against it, so it
> works at least for that use case.
>
> While certainly not release worthy, it still might be interesting to
> look at.
> https://urldefense.proofpoint.com/v1/url?u=http://git.postgresql.org/gitweb/?p%3Dusers/andresfreund/postgres.git%3Ba%3Dblob%3Bf%3Dcontrib/bdr/bdr_seq.c%3Bh%3Dc9480c8021882f888e35080f0e7a7494af37ae27%3Bhb%3Dbdr&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=xGch4oNJbpD%2BKPJECmgw4SLBhytSZLBX7UnkZhtNcpw%3D%0A&m=Rbmo%2BDar4PZrQmGH2adz7cCgqRl2%2FXH845YIA1ifS7A%3D%0A&s=8d287f35070fe7cb586f10b5bfe8664ad29e986b5f2d2286c4109e09f615668d
>
> bdr_sequencer_fill_sequence pre-allocates ranges of values,
> bdr_sequence_alloc returns them upon nextval().

Thanks. That pokes into the low-level details of sequence tuples, just
like I was afraid it would.

>> I wonder if the level of abstraction is right. The patch assumes that the
>> on-disk storage of all sequences is the same, so the access method can't
>> change that. But then it leaves the details of actually updating the on-disk
>> block, WAL-logging and all, as the responsibility of the access method.
>> Surely that's going to look identical in all the seqams, if they all use the
>> same on-disk format. That also means that the sequence access methods can't
>> be implemented as plugins, as plugins can't do WAL-logging.
>
> Well, it exposes log_sequence_tuple() - together with the added "am
> private" column of pg_squence that allows to do quite a bit of different
> things. I think unless we really implement pluggable RMGRs - which I
> don't really see coming - we cannot be radically different.

The proposed abstraction leaks like a sieve. The plugin should not need
to touch anything but the private amdata field. log_sequence_tuple() is
way too low-level.

Just as a thought-experiment, imagine that we decided to re-implement
sequences so that all the sequence values are stored in one big table,
or flat-file in the data directory, instead of the current
implementation where we have a one-block relation file for each
sequence. If the sequence access method API is any good, it should stay
unchanged. That's clearly not the case with the proposed API.

>> The comment in seqam.c says that there's a private column reserved for
>> access method-specific data, called am_data, but that seems to be the only
>> mention of am_data in the patch.
>
> It's amdata, not am_data :/. Directly at the end of pg_sequence.

Ah, got it.

Stepping back a bit and looking at this problem from a higher level, why
do you need to hack this stuff into the sequences? Couldn't you just
define a new set of functions, say bdr_currval() and bdr_nextval(), to
operate on these new kinds of sequences? You're not making much use of
the existing sequence infrastructure, anyway, so it might be best to
just keep the implementation completely separate. If you need it for
compatibility with applications, you could create facade
currval/nextval() functions that call the built-in version or the bdr
version depending on the argument.

- Heikki