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.
> 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.
> + printf(_(" --no-sequence-access-method do not sequence table access methods\n"));
> Seems a copy/paste bug, missing “dump” after “do not”.
Oops, fixed.
> 7 - 0005 - sequenceam.sgml
> 8 - 0005 - sequenceam.sgml
> 9 - 0005 - config.sgml
> 10 - 0005 - sequenceam.sgml
> 11 - 0005 - create_access_method.sgml
Yeah, fixed these ones. For the last one, I am not really sure how
much the ordering matters, but I guess that your suggestion is fine as
well.
> 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.
> 14 - 0007 - contrib/snowflake/meson.build
> ```
> +contrib_targets += bloom
> ```
>
> Looks like a copy/paste bug, should “bloom” be “snowflake”?
Indeed.
--
Michael
