From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Sequence Access Method WIP |
Date: | 2015-01-13 12:24:37 |
Message-ID: | 54B50E85.2060901@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 12.1.2015 22:33, Petr Jelinek wrote:
> On 15/12/14 11:36, Petr Jelinek wrote:
>> On 10/12/14 03:33, Petr Jelinek wrote:
>>> On 24/11/14 12:16, Heikki Linnakangas wrote:
>>>
>>> About the rough edges:
>>> - The AlterSequence is not prettiest code around as we now have to
>>> create new relation when sequence AM is changed and I don't know how to
>>> do that nicely
>>> - I am not sure if I did the locking, command order and dependency
>>> handling in AlterSequence correcly
>>
>> This version does AlterSequence differently and better. Didn't attach
>> the gapless sequence again as that one is unchanged.
>>
>>
>
> And another version, separated into patch-set of 3 patches.
> First patch contains the seqam patch itself, not many changes there,
> mainly docs/comments related. What I wrote in the previous email for
> version 3 still applies.
I did a review of the first part today - mostly by reading through the
diff. I plan to do a bit more thorough testing in a day or two. I'll
also look at the two (much smaller) patches.
comments/questions/general nitpicking:
(1) Why treating empty string as equal to 'local'? Isn't enforcing the
actual value a better approach?
(2) NITPICK: Maybe we could use access_method in the docs (instead of
sequence_access_method), as the 'sequence' part is clear from the
context?
(3) Why index_reloptions / sequence_reloptions when both do exactly the
same thing (call to common_am_reloptions)? I'd understand this if
the patch(es) then change the sequence_reloptions() but that's not
happening. Maybe that's expected to happen?
(4) DOCS: Each sequence can only use one access method at a time.
Does that mean a sequence can change the access method during it's
lifetime? My understanding is that the AM is fixed after creating
the sequence?
(5) DOCS/NITPICK: SeqAM / SeqAm ... (probably should be the same?)
(6) cache lookup failed for relation %u
I believe this should reference 'sequence access method' instead of
a relation.
(7) seqam_init
I believe this is a bug (not setting nulls[4] but twice nulls[3]):
+ fcinfo.argnull[0] = seqparams == NULL;
+ fcinfo.argnull[1] = reloptions_parsed == NULL;
+ fcinfo.argnull[2] = false;
+ fcinfo.argnull[3] = false;
+ fcinfo.argnull[3] = false;
(8) check_default_seqam without a transaction
* If we aren't inside a transaction, we cannot do database access so
* cannot verify the name. Must accept the value on faith.
In which situation this happens? Wouldn't it be better to simply
enforce the transaction and fail otherwise?
regards
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2015-01-13 12:33:38 | Re: Unused variables in hstore_to_jsonb |
Previous Message | Andres Freund | 2015-01-13 12:11:31 | Re: Escaping from blocked send() reprised. |