Re: Logical Replication of sequences

On Wed, 6 Aug 2025 at 16:29, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, Aug 6, 2025 at 2:28 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > The attached v20250806 version patch has the changes for the same.
> >
>
> Thank You for the patches. Please find a few comments:
>
> 1)
> * If 'resync_all_sequences' is false:
> * Add or remove tables and sequences that have been added to or removed
> * from the publication since the last subscription creation or refresh.
> * If 'resync_all_sequences' is true:
> * Perform the above operation only for sequences.
>
> Shall we update:
> Perform the above operation only for sequences and resync all the
> sequences including existing ones.

Modified

> 2)
> XLogRecPtr srsublsn BKI_FORCE_NULL; /* remote LSN of the
> state change
>
> Shall we rename it to srremotelsn or srremlsn? srsublsn gives a
> feeling that it is local lsn and should be in sync with the one
> displayed by pg_get_sequence_data() locally but that is not the case.

I felt this is an existing column which is also used for tables, the
same table behavior is used for sequences too. Since this is an
existing column which has been used from long time, I prefer not to
change it.

> 3)
> create sequence myseq1 start 1 increment 100;
> postgres=# select last_value, is_called, log_cnt, page_lsn from
> pg_get_sequence_data('myseq1');
> last_value | is_called | log_cnt | page_lsn
> ------------+-----------+---------+------------
> 1 | f | 0 | 0/017BEF10
>
> postgres=# select sequencename, last_value from pg_sequences;
> sequencename | last_value
> --------------+------------
> myseq1 |
>
> For a fresh sequence created, last_value shown by pg_get_sequence_data
> seems wrong. On doging nextval for the first time, last_value shown by
> pg_get_sequence_data does not change as the original value was wrong
> itself to start with.

This behavior is implemented like that in the base code. I will
analyze more why it was implemented like that and discuss this in the
original thread.

> 4)
> + Returns information about the sequence. <literal>last_value</literal>
> + is the current value of the sequence, <literal>is_called</literal>
>
> It looks odd to say that 'last_value is the current value of the
> sequence'. Why don't we name it curr_val? If this is an existing
> function and thus we do not want to change the name, then we can say
> something on the line that 'last sequence value set in sequence by
> nextval or setval' or something
> similar to what pg_sequences says for last_value.

Modified

> 5)
> + and <literal>page_lsn</literal> is the page LSN of the sequence
> + relation.
>
> Is the page_lsn the page lsn of sequence relation or lsn of the last
> WAL record written (or in simpler terms that particular record's
> page_lsn)? If it is relation page-lsn, it should not change.

Updated documentation

> 6)
> I have noticed that when I do nextval, logcnt reduces and page_lsn is
> not changed until it crosses the threshold. This is in context of
> output returned by pg_get_sequence_data. But on doing setval, page_lsn
> changes everytime and logcnt is reset to 0. Is this expected behaviour
> or the issue in output of pg_get_sequence_data()? I did not get this
> information from setval's doc. Can you please review and confirm?
>
> postgres=# SELECT nextval('myseq2');
> nextval
> ---------
> 155
> postgres=# select last_value, is_called, log_cnt, page_lsn from
> pg_get_sequence_data('myseq2');
> last_value | is_called | log_cnt | page_lsn
> ------------+-----------+---------+------------
> 155 | t | 28 | 0/017C4498
>
> postgres=# SELECT nextval('myseq2');
> nextval
> ---------
> 175
>
> postgres=# select last_value, is_called, log_cnt, page_lsn from
> pg_get_sequence_data('myseq2');
> last_value | is_called | log_cnt | page_lsn
> ------------+-----------+---------+------------
> 175 | t | 27 | 0/017C4498
>
> postgres=# SELECT setval('myseq2', 55, true);
> setval
> --------
> 55
>
> postgres=# select last_value, is_called, log_cnt, page_lsn from
> pg_get_sequence_data('myseq2');
> last_value | is_called | log_cnt | page_lsn
> ------------+-----------+---------+------------
> 55 | t | 0 | 0/017C4568

I believe this behavior is expected. In the case of nextval,
PostgreSQL prefetches 32 values in advance and uses the increment_by
setting to serve the next value from this cached range. Since these
values are predictable and already reserved, they don't need to be
WAL-logged individually.
However, with setval, the new value being set is arbitrary and cannot
be assumed to follow the previous sequence. It could be a jump
forward, backward, or even the same value. Because of this
unpredictability, the change must be explicitly WAL-logged to ensure
durability and consistency in case of recovery.

Please find my response for the comments from [1]:
>7)
>For an all-seq publication, we see this:
>
> Owner | All tables | All sequences | Inserts | Updates | Deletes |
>Truncates | Generated columns | Via root
>--------+------------+---------------+---------+---------+---------+-----------+-------------------+----------
> shveta | f | t | t | t | t | t
> | none | f
>(1 row)
>
>I feel Inserts, Updates, Deletes and Truncates -- all should be marked
>as 'f' instead of default 't'. If we look at the doc of
>pg_publication, it points to DML operations of these pages while
>explaining pubinsert, pubupdate etc. These DML operations have no
>meaning for sequences, thus it makes more sense to make these as 'f'
>for sequences. Thoughts?

Modified

>8)
>In pg_publication doc, we shall have a NOTE mentioning that pubinsert,
>pubupdate, pubdelete, pubtruncate are not applicable to sequences and
>thus will always be false for an all-seq publication. For an all
>table, all seq publication; these fields will reflect values for
>tables alone.

I felt this is not required, it is mentioned in the docs that it is
only for tables. ex: If true, INSERT operations are replicated for
tables in the publication.

>9)
>GetAllTablesPublicationRelations
>
>Earlier we had this name because the publication was for 'ALL TABLES',
>but now it could be ALL SEQUNECES too. We shall rename this function.
>Some options are: GetAllPublicationRelations,
>GetPublicationRelationsForAll,
>GetPublicationRelationsForAllTablesSequences

Modified it to GetAllPublicationRelations

Please find my response for the comments from [2]:
>patch-0005: sequencesync.c
>+ aclresult = pg_class_aclcheck(RelationGetRelid(sequence_rel), GetUserId(),
>+ ACL_UPDATE);
>+ if (aclresult != ACLCHECK_OK)
>+ aclcheck_error(aclresult,
>+ get_relkind_objtype(sequence_rel->rd_rel->relkind),
>+ seqname);
>
>I see that the run_as_owner check has been removed from
>LogicalRepSyncSequences() and added to copy_sequences() for the
>SetSequence() call.
>
>However, IIUC, the same check is also needed in
>LogicalRepSyncSequences(). Currently, the sequencesync worker can fail
>in the above permission check since user switching doesn’t happen when
>run_as_owner is false.
>
>```
>ERROR: permission denied for sequence n1
>```
>Should we add the run_as_owner handling here as well to avoid this?

This check here is not required as this check will be done during the
set sequence. updated it.

The attached v20250813 patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAJpy0uADzXSyx9YYPB-tuCfNWfGi4__CotQ1T3-q7AwBVCZRrg@mail.gmail.com
[2] - https://www.postgresql.org/message-id/CABdArM7aY%2Bu5Fv9KMHp_iX%3DAEixfDum5e2ixZkWS8YcOt_NO7Q%40mail.gmail.com

Regards,
Vignesh