Re: Logical Replication of sequences - Mailing list pgsql-hackers

On Mon, 15 Sept 2025 at 14:36, vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 8 Sept 2025 at 12:05, Chao Li <li.evan.chao@gmail.com> wrote:
> >
> >
> >
> > On Sep 8, 2025, at 14:00, vignesh C <vignesh21@gmail.com> wrote:
> >
> >
> >
> > 1 - 0001
> > ```
> > diff --git a/src/test/regress/sql/sequence.sql b/src/test/regress/sql/sequence.sql
> > index 2c220b60749..c8adddbfa31 100644
> > --- a/src/test/regress/sql/sequence.sql
> > +++ b/src/test/regress/sql/sequence.sql
> > @@ -414,6 +414,6 @@ SELECT nextval('test_seq1');
> > SELECT nextval('test_seq1');
> >
> > -- pg_get_sequence_data
> > -SELECT * FROM pg_get_sequence_data('test_seq1');
> > +SELECT last_value, is_called, log_cnt, page_lsn <= pg_current_wal_lsn() as lsn FROM
pg_get_sequence_data('test_seq1');
> >
> > DROP SEQUENCE test_seq1;
> > ```
> >
> > As it shows log_cnt now, after calling pg_get_sequence_data(), I suggest add 8 nextval(), so that sequence goes to
11,and log_cnt should become to 22. 
> >
> >
> > Could you please explain the reason you’d like this to be done?
> >
> >
> > Because log_cnt is newly exposed, we want to verify its value in the test. When I first time ran the test code, I
sawinitial value of log_cnt was 32, then I thought log_cnt might get decreased if I ran nextval() again, but it didn’t.
Onlyafter I ran 10 (cache size) more nextval(), log_cnt got decreased by 10 to 22. The test code is a place for people
tolook for expected behavior. So I think adding more nextval() to verify and show the change of log_cnt is helpful. 
>
> This is addressed in the attached patch, also rebased the patch
> because of recent commits.
>
Hi Vignesh,

FYI: the patches are not applying on current HEAD.

I have reviewed the patches and here are my comments:

1. For patch 0001:
The 'log_cnt' column of 'pg_get_sequence_data' gets reset after a checkpoint.
For example:
postgres=# select * from pg_get_sequence_data('seq1');
 last_value | is_called | log_cnt |  page_lsn
------------+-----------+---------+------------
          3 | t         |      31 | 0/0177C800
(1 row)
postgres=# checkpoint;
CHECKPOINT
postgres=# select nextval('seq1');
 nextval
---------
       4
(1 row)
postgres=# select * from pg_get_sequence_data('seq1');
 last_value | is_called | log_cnt |  page_lsn
------------+-----------+---------+------------
          4 | t         |      32 | 0/0177C998

So, for tests:
+SELECT last_value, is_called, log_cnt, page_lsn <=
pg_current_wal_lsn() as lsn FROM pg_get_sequence_data('test_seq1');
+ last_value | is_called | log_cnt | lsn
+------------+-----------+---------+-----
+         20 | t         |      22 | t

Is there a possibility that it can show a different value of "log_cnt"
due checkpoint running in background or parallel test?

I see following comment in the similar test:
-- log_cnt can be higher if there is a checkpoint just at the right
-- time, so just test for the expected range
SELECT last_value, log_cnt IN (31, 32) AS log_cnt_ok, is_called FROM
foo_seq_new;

Thoughts?

2. For patch 0002:
I created a publication pub1 for all sequences, and now altering with
set give following error:
postgres=# alter publication pub1 SET (publish_via_partition_root);
ERROR:  WITH clause parameters are not supported for publications
defined as FOR ALL SEQUENCES

I think we should not use "WITH clause parameters" for "ALTER
PUBLICATION ... SET .." command.

3. For patch 0003:
We need to update the function 'HasSubscriptionRelationsCached'.
It has function call "has_subrels = FetchTableStates(&started_tx)"

I think FetchTableStates should be updated toFetchRelationStates.
This function call was added in the recent commit [1].

4. For patch 0007:
We should update the logical-replication.sgml for the occurrence of with \dRp+:

<programlisting><![CDATA[
/* pub # */ \dRp+
                                         Publication p1
  Owner   | All tables | Inserts | Updates | Deletes | Truncates |
Generated columns | Via root
----------+------------+---------+---------+---------+-----------+-------------------+----------
 postgres | f          | t       | t       | t       | t         |
none              | f
Tables:
    "public.t1" WHERE ((a > 5) AND (c = 'NSW'::text))

                                         Publication p2
  Owner   | All tables | Inserts | Updates | Deletes | Truncates |
Generated columns | Via root
----------+------------+---------+---------+---------+-----------+-------------------+----------
 postgres | f          | t       | t       | t       | t         |
none              | f
Tables:
    "public.t1"
    "public.t2" WHERE (e = 99)

                                         Publication p3
  Owner   | All tables | Inserts | Updates | Deletes | Truncates |
Generated columns | Via root
----------+------------+---------+---------+---------+-----------+-------------------+----------
 postgres | f          | t       | t       | t       | t         |
none              | f


[1]: https://github.com/postgres/postgres/commit/1f7e9ba3ac4eff13041abcc4c9c517ad835fa449

Thanks,
Shlok Kyal