Re: Sequence Access Method WIP

On 01/23/2015 02:34 AM, Petr Jelinek wrote:
> On 22/01/15 17:02, Petr Jelinek wrote:
>>
>> The new version (the one that is not submitted yet) of gapless sequence
>> is way more ugly and probably not best example either but does guarantee
>> gaplessness (it stores the last value in it's own value table). So I am
>> not sure if it should be included either...
>
> Here it is as promised.

I generally like the division of labour between the seqam 
implementations and the API now.

I don't like the default_sequenceam GUC. That seems likely to create 
confusion. And it's not really enough for something like a remote 
sequence AM anyway that definitely needs some extra reloptions, like the 
hostname of the remote server. The default should be 'local', unless you 
specify something else with CREATE SEQUENCE USING - no GUCs.

Some comments on pg_dump:

* In pg_dump's dumpSequence function, check the server version number 
instead of checking whether pg_sequence_dump_state() function exists. 
That's what we usually do when new features are introduced. And there's 
actually a bug there: you have the check backwards. (try dumping a 
database with any sequences in it; it fails)

* pg_dump should not output a USING clause when a sequence uses the 
'local' sequence. No point in adding such noise - making the SQL command 
not standard-compatible - for the 99% of databases that don't use other 
sequence AMs.

* Be careful to escape all strings correctly in pg_dump. I think you're 
missing escaping for the 'state' variable at least.

In sequence_save_tuple:
>     else
>     {
>         /*
>          * New tuple was not sent, so the original tuple was probably just
>          * changed inline, all we need to do is mark the buffer dirty and
>          * optionally log the update tuple.
>          */
>         START_CRIT_SECTION();
>
>         MarkBufferDirty(seqh->buf);
>
>         if (do_wal)
>             log_sequence_tuple(seqh->rel, &seqh->tup, seqh->buf, page);
>
>         END_CRIT_SECTION();
>     }

This is wrong when checksums are enabled and !do_wal. I believe this 
should use MarkBufferDirtyHint().

> Notable changes:
> - The gapless sequence rewritten to use the transactional storage as
> that's the only way to guarantee gaplessness between dump and restore.

Can you elaborate?

Using the auxiliary seqam_gapless_values is a bit problematic. First of 
all, the event trigger on DROP SEQUENCE doesn't fire if you drop the 
whole schema containing the sequence with DROP SCHEMA CASCADE. Secondly, 
updating a row in a table for every nextval() call is pretty darn 
expensive. But I don't actually see a problem with dump and restore.

Can you rewrite it without using the values table? AFAICS, there are a 
two of problems to solve:

1. If the top-level transaction aborts, you need to restore the old 
value. I suggest keeping two values in the sequence tuple: the old, 
definitely committed one, and the last value. The last value is only 
considered current if the associated XID committed; otherwise the old 
value is current. When a transaction is about to commit, it stores its 
top-level XID and the new value in the "last" field, and copies the 
previously current value to the "old" field.

2. You need to track the last value on a per-subtransaction basis, until 
the transaction commits/rolls back, in order to have "rollback to 
savepoint" to retreat the sequence's value. That can be done in 
backend-private memory, maintaining a stack of subtransactions and the 
last value of each. Use RegisterSubXactCallback to hook into subxact 
commit/abort. Just before top-level commit (in pre-commit callback), 
update the sequence tuple with the latest value, so that it gets 
WAL-logged before the commit record.

- Heikki