On Mon, 15 Sept 2025 at 14:36, vignesh C <[email protected]> wrote:
>
> On Mon, 8 Sept 2025 at 12:05, Chao Li <[email protected]> wrote:
> >
> >
> >
> > On Sep 8, 2025, at 14:00, vignesh C <[email protected]> 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 saw initial value of log_cnt was 32,
> > then I thought log_cnt might get decreased if I ran nextval() again, but it
> > didn’t. Only after I ran 10 (cache size) more nextval(), log_cnt got
> > decreased by 10 to 22. The test code is a place for people to look 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
