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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
