Re: Sequence Access Method WIP

On 20/04/15 17:50, Heikki Linnakangas wrote:
> On 03/15/2015 09:07 PM, Petr Jelinek wrote:
>> Slightly updated version of the patch.
>>
>> Mainly rebased against current master (there were several conflicts) and
>> fixed some typos, no real functional change.
>>
>> I also attached initial version of the API sgml doc.
>
> I finally got around to have another round of review on this. I fixed a
> couple of little bugs, did some minor edition on comments etc. See
> attached. It is also available in my git repository at
> git://git.postgresql.org/git/users/heikki/postgres.git, branch "seqam",
> if you want to look at individual changes. It combines your patches 1
> and 4, I think those need to be applied together. I haven't looked at
> the DDL changes yet.

Thanks!

>
> I'm fairly happy with the alloc API now. I'm not sure it's a good idea
> for the AM to access the sequence tuple directly, though. I would seem
> cleaner if it only manipulated the amdata Datum. But it's not too bad as
> it is.

Yeah, I was thinking about this myself I just liked sending 10 
parameters to the function less than this.

>
> The division of labour between sequence.c and the AM, in the init and
> the get/set_state functions, is a bit more foggy:
>
> * Do we really need a separate amoptions() method and an init() method,
> when the only caller to amoptions() is just before the init() method?
> The changes in extractRelOptions suggest that it would call
> am_reloptions for sequences too, but that doesn't actually seem to be
> happening.

Hmm yes it should and I am sure it did at some point, must have messed 
it during one of the rebases :(

And it's the reason why we need separate API function.

>
> postgres=# create sequence fooseq using local with (garbage=option);
> CREATE SEQUENCE
>
> Invalid options probably should throw an error.
>
> * Currently, the AM's init function is responsible for basic sanity
> checking, like min < max. It also has to extract the RESTART value from
> the list of options. I think it would make more sense to move that to
> sequence.c. The AM should only be responsible for initializing the
> 'amdata' portion, and for handling any AM-specific options. If the AM
> doesn't support some options, like MIN and MAX value, it can check them
> and throw an error, but it shouldn't be responsible for just passing
> them through from the arguments to the sequence tuple.

Well then we need to send restart as additional parameter to the init 
function as restart is normally not stored anywhere.

The checking is actually if new value is withing min/max but yes that 
can be done inside sequence.c I guess.

>
> * It might be better to form the sequence tuple before calling the init
> function, and pass the ready-made tuple to it. All the other API
> functions deal with the tuple (after calling sequence_read_tuple), so
> that would be more consistent. The init function would need to
> deconstruct it to modify it, but we're not concerned about performance
> here.

Right, this is actually more of a relic of the custom columns 
implementation where I didn't want to build the tuple with NULLs for 
columns that might have been specified as NOT NULL, but with the amdata 
approach it can create the tuple safely.

>
> * The transformations of the arrays in get_state() and set_state()
> functions are a bit complicated. The seqam_get_state() function returns
> two C arrays, and pg_sequence_get_state() turns them into a text[]
> array. Why not construct the text[] array directly in the AM? I guess
> it's a bit more convenient to the AM, if the pg_sequence_get_state() do
> that, but if that's an issue, you could create generic helper function
> to turn two C string arrays into text[], and call that from the AM.

Yeah that was exactly the reasoning. Helper function works for me (will 
check what Álvaro's suggested, maybe it can be moved somewhere and reused).

>
>>     seq = (FormData_pg_sequence *) GETSTRUCT(sequence_read_tuple(seqh));
>>
>>     for (i = 0; i < count; i++)
>>     {
>>         if (pg_strcasecmp(keys[i], "last_value") == 0)
>>             seq->last_value = DatumGetInt64(DirectFunctionCall1(int8in,
>>
>> CStringGetDatum(values[i])));
>>         else if (pg_strcasecmp(keys[i], "is_called") == 0)
>>             seq->is_called = DatumGetBool(DirectFunctionCall1(boolin,
>>
>> CStringGetDatum(values[i])));
>>         else
>>             ereport(ERROR,
>>                     (errcode(ERRCODE_SYNTAX_ERROR),
>>                      errmsg("invalid state key \"%s\" for local
>> sequence",
>>                             keys[i])));
>>     }
>>
>>     sequence_save_tuple(seqh, NULL, true);
>
> If that error happens after having already processed a "last_value" or
> "is_called" entry, you have already modified the on-disk tuple. AFAICS
> that's the only instance of that bug, but sequence_read_tuple - modify
> tuple in place - sequence_save_tuple pattern is quite unsafe in general.
> If you modify a tuple directly in a Buffer, you should have a critical
> section around it. It would make sense to start a critical section in
> sequence_read_tuple(), except that not all callers want to modify the
> tuple in place. Perhaps the sequence_read_tuple/sequence_save_tuple
> functions should be split into two:
>
> sequence_read_tuple_for_update()
> sequence_save_tuple()

Agreed, however I am much more concerned about the way 
seqam_local_alloc() works, see the XXX comment there. I am thinking it's 
not really safe that way, but problem is that we can't put critical 
section around it currently as the sequence_save_tuple() can potentially 
call GetTopTransactionId(). Separating it into more functions might be 
solution. Except the split into two does not really help there, it would 
have to be something like this:
sequence_tuple_update_start() - does the GetTopTransactionId bit and 
starts critical section
sequence_tuple_save() - saves the tuple/does wal
sequence_tuple_update_finish() - ends the critical section

That looks slightly cumbersome but I don't really have better idea.

>
> * seqam_local_get_state() calls sequence_read_tuple() but not
> sequence_release_tuple(). It looks like sequence_close() releases the
> tuple and the buffer, but that seems sloppy. sequence_close() probably
> shouldn't be calling sequence_release_tuple at all, so that you'd get a
> warning at the end of transaction about the buffer leak.
>

Do you mean that you want to make call to sequence_release_tuple() 
mandatory when sequence_read_tuple() was called?


--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services