Thread

Re: Sequence Access Methods, round two

Chao Li <li.evan.chao@gmail.com> — 2025-12-26T06:41:01Z

> On Dec 26, 2025, at 13:45, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Wed, Dec 24, 2025 at 04:14:06PM +0800, Chao Li wrote:
>> Okay, a clean build has made the patch working for me. I did some
>> basic tests and went through all commits. After 26 rounds of
>> revision, this patch now looks solid already. Here comes my
>> comments:
> 
> Thanks for the review.
> 
>> 3 - 0002 - seqlocalam.c
>> +static void
>> +fill_seq_with_data(Relation rel, HeapTuple tuple)
>> +{
>> ```
>> 
>> This function handles unlogged logic, but should UNLOGGED be handled
>> by the core?
> 
> That is something I have considered while splitting the code, but this
> makes the init() and reset() callbacks weirder in shape as, so the
> separation felt more natural to me, leaving up to the AM layer what
> should be done.  So the answer from me is nope on this one.

Then that has to be documented clearly. Meaning that, if a user chooses to use a non-default sequence AM, then UNLOGGED might not work, I don’t see a mechanism to enforce a custom AM to honor UNLOGGED.

> 
>> I am thinking if we should use the constant
>> DEFAULT_TABLE_ACCESS_METHOD that is also “heap” today. My theory is
>> that, in future, PG introduces a new table access method, say
>> “heap_ex” (enhanced heap”), and switch to use it,
>> DEFAULT_TABLE_ACCESS_METHOD will be updated to “heap_ex”, then I
>> guess local sequence am should switch to use “heap_ex” for table am
>> as well.
> 
> Using DEFAULT_TABLE_ACCESS_METHOD makes sense I guess.  I doubt that
> we'll ever rename that, though.

I didn’t mean to suggest that we would ever rename “heap” to “heap_ex”. That was just an example to illustrate that, at some point in the future, a new default table AM might be introduced. If we use the constant today, it reduces the risk of forgetting to update this code when such a change happens.

>> 12 - 0006
>> This macro assumes the caller has buf, page, sm, tuple, rel and
>> forkNum, which makes the macro fragile. Actually, buf, page and sm
>> are purely local and temp, they can be defined within the do{}while
>> block, and tuple, rel and forkNum can be passed in as
>> arguments. Also, given this macro is relatively long, maybe consider
>> an inline static function.
>> 
>> 13 - 0006
>> This macro not only assumes the caller has buf, also update
>> buf. Maybe a static inline function is better here.
> 
> These two are intentional for one reason: it makes people aware that
> pages should have a special area enforced with a specific type and
> that the special area should be cross-checked on read.  It is true
> that in the init() case we could return a Buffer and pass the size of
> the special area, setting the contents of the special area outside,
> but the second case brings trouble as the special area should be
> checked before reading any fields from the page.  Perhaps the case for
> this layer is not that much justified, this only mimics the core
> sequence method that relies on a single page per sequence with a
> buffer lock when getting a value.

Should the macro comment documents something like: “to use this macro, the caller must define the following variables 1) Buffer buf;, 2) Page page;, 3) seam_speical *sm;” If we use a function, the interface is clear; to use a macro, the interface and dependencies are not that clear, so more documentation is needed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/