Re: Sequence Access Methods, round two

> On Dec 23, 2025, at 16:48, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
>> On Dec 23, 2025, at 16:42, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>>
>> On Tue, Dec 23, 2025 at 02:55:45PM +0800, Chao Li wrote:
>>> I just started to review this patch. While reviewing 0001, I got the
>>> point where you now add the 3 attributes into pg_attributes, so I
>>> tried to test this behavior, but a basic “create sequence” command
>>> failed for me:
>>> ```
>>> evantest=# create sequence ts1 start with 1 increment by 1 cache 10 cycle;
>>> ERROR: access method "seqlocal" does not exist
>>> ```
>>>
>>> Did I do something wrong?
>>
>> I guess so. 0001 taken independently has no link to seqlocal.
>> Applying the full set of patches works here:
>> =# create sequence ts1 start with 1 increment by 1 cache 10 cycle;
>> CREATE SEQUENCE
>>
>> I doubt that the regression would pass at all with this class of
>> failures. You may need to clean your tree and rebuild.
>> --
>> Michael
>
> I have applied the full patch set. So, I guess I need a clean rebuild. I will try again.
>

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:

1 - 0001
```
+ /*
+ * Initial relation has no attributes, these are added later.
+ */
+ stmt->tableElts = NIL;
```

When I read the first few commits, I was thinking why cannot we still add the 3 attributes while creating the sequence relation. Finally, I got the idea: creating an empty sequence relation and adding attributes later enables different access method to add different attributes.

I think "these are added later” is too simple, it deserves more words, which would make readers easier to understand the change.

2 - 0002
```
--- a/src/backend/access/transam/rmgr.c
+++ b/src/backend/access/transam/rmgr.c
@@ -27,13 +27,13 @@
#include "access/gistxlog.h"
#include "access/hash_xlog.h"
#include "access/heapam_xlog.h"
+#include "access/seqlocal_xlog.h"
#include "access/multixact.h"
#include "access/nbtxlog.h"
#include "access/spgxlog.h"
#include "access/xact.h"
#include "catalog/storage_xlog.h"
#include "commands/dbcommands_xlog.h"
-#include "commands/sequence_xlog.h"
#include "commands/tablespace.h"
#include "replication/decode.h"
#include "replication/message.h”
```

AFAIK, headers files should placed in alphabetical order, so "access/seqlocal_xlog.h” should be placed after "access/nbtxlog.h”. You can see all existing includes follow alphabetical order.

3 - 0002 - seqlocalam.c
```
+/*
+ * Initialize a sequence's relation with the specified tuple as content
+ *
+ * This handles unlogged sequences by writing to both the main and the init
+ * fork as necessary.
+ */
+static void
+fill_seq_with_data(Relation rel, HeapTuple tuple)
+{
```

This function handles unlogged logic, but should UNLOGGED be handled by the core?

4 - 0002
```
+const char *
+seq_local_get_table_am(void)
+{
+ return "heap";
+}
```

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.

5 - 0003
```
--- a/src/backend/commands/amcmds.c
+++ b/src/backend/commands/amcmds.c
@@ -15,6 +15,7 @@

#include "access/htup_details.h"
#include "access/table.h"
+#include "access/sequenceam.h"
#include "catalog/catalog.h”
```

"access/sequenceam.h” should be placed before "access/table.h”.

6 - 0004
```
+ printf(_(" --no-sequence-access-method do not sequence table access methods\n"));
```

Seems a copy/paste bug, missing “dump” after “do not”.

7 - 0005 - sequenceam.sgml
```
+ methods</firstterm>, which manage the operations around sequences . The core
```

An unnecessary white-space between “sequences” and “.”.

8 - 0005 - sequenceam.sgml
```
+ so it is possible to develop entirely new access method types by writing
```

* “entirely” seems not necessary
* “types” seems not needed, maybe just “new access methods”. Because types might lead to confusion with table/index AM types.

9 - 0005 - config.sgml
```
+ This parameter specifies the default sequence access method to use when
+ creating sequences if the <command>CREATE SEQUENCE</command>
+ command does not explicitly specify an access method. The default is
+ <literal>local</literal>.
```

Should default be “seqlocal”?

10 - 0005 - sequenceam.sgml
```
+ Any developer of a new <literal>sequence access method</literal> can refer to
+ the existing <literal>local</literal> implementation present in
+ <filename>src/backend/access/sequence/local.c</filename> for details of
+ its implementation.
```

“local.c” => “seqlocalam.c”

11 - 0005 - create_access_method.sgml
```
- Only <literal>TABLE</literal> and <literal>INDEX</literal>
- are supported at present.
+ Only <literal>TABLE</literal>, <literal>SEQUENCE</literal> and
+ <literal>INDEX</literal> are supported at present.
```

Maybe put INDEX before SEQUENCE, because INDEX is more important than SEQUENCE.

12 - 0006
```
+#define SEQUENCE_PAGE_INIT(seqam_special, seqam_magic_value) \
+do { \
+ buf = ExtendBufferedRel(BMR_REL(rel), forkNum, NULL, \
+ EB_LOCK_FIRST | EB_SKIP_EXTENSION_LOCK); \
+ Assert(BufferGetBlockNumber(buf) == 0); \
+ \
+ page = BufferGetPage(buf); \
+ \
+ PageInit(page, BufferGetPageSize(buf), sizeof(seqam_special)); \
+ sm = (seqam_special *) PageGetSpecialPointer(page); \
+ sm->magic = seqam_magic_value; \
+ \
+ /* Now insert sequence tuple */ \
+ HeapTupleHeaderSetXmin(tuple->t_data, FrozenTransactionId); \
+ HeapTupleHeaderSetXminFrozen(tuple->t_data); \
+ HeapTupleHeaderSetCmin(tuple->t_data, FirstCommandId); \
+ HeapTupleHeaderSetXmax(tuple->t_data, InvalidTransactionId); \
+ tuple->t_data->t_infomask |= HEAP_XMAX_INVALID; \
+ ItemPointerSet(&tuple->t_data->t_ctid, 0, FirstOffsetNumber); \
+} while(0)
```

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
```
+#define SEQUENCE_PAGE_READ(Form_seqam_data, seqam_special, seqam_magic_value) \
+do { \
+ Page page; \
+ ItemId lp; \
+ \
+ *buf = ReadBuffer(rel, 0); \
+ LockBuffer(*buf, BUFFER_LOCK_EXCLUSIVE); \
+ \
+ page = BufferGetPage(*buf); \
+ sm = (seqam_special *) PageGetSpecialPointer(page); \
+ \
+ if (sm->magic != seqam_magic_value) \
+ elog(ERROR, "bad magic number in sequence \"%s\": %08X", \
+ RelationGetRelationName(rel), sm->magic); \
+ \
+ lp = PageGetItemId(page, FirstOffsetNumber); \
+ Assert(ItemIdIsNormal(lp)); \
+ \
+ /* \
+ * Note we currently only bother to set these two fields of \
+ * *seqdatatuple. \
+ */ \
+ seqdatatuple->t_data = (HeapTupleHeader) PageGetItem(page, lp); \
+ seqdatatuple->t_len = ItemIdGetLength(lp); \
+ \
+ seq = (Form_seqam_data) GETSTRUCT(seqdatatuple); \
+} while(0)
```

This macro not only assumes the caller has buf, also update buf. Maybe a static inline function is better here.

14 - 0007 - contrib/snowflake/meson.build
```
+contrib_targets += bloom
```

Looks like a copy/paste bug, should “bloom” be “snowflake”?

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