Re: Logical Replication of sequences

On Wed, Aug 6, 2025 at 4:29 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Aug 6, 2025 at 2:28 PM vignesh C <vignesh21@gmail.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.
>
> 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.
>
> 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.
>
> 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.
>
> 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.
>
> 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
>


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?

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.

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

thanks
Shveta