Re: Logical Replication of sequences

On Wed, 17 Sept 2025 at 19:08, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Mon, 15 Sept 2025 at 14:36, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Mon, 8 Sept 2025 at 12:05, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > >
> > >
> > >
> > > On Sep 8, 2025, at 14:00, vignesh C <vignesh21(at)gmail(dot)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 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,
>
> Here are some more review comments:
>
> For patch 0006:
>
> 1. Spelling mistake in:
> + appendStringInfo(combined_error_detail, "Insufficent permission for
> sequence(s): (%s).",
> + insuffperm_seqs->data);
>
> Insufficent -> Insufficient
>
> 2. Spelling mistake in:
> + ereport(LOG,
> + errmsg("logical replication sequence synchronization for
> subscription \"%s\" - batch #%d = %d attempted, %d succeeded, %d
> skipped, %d mismatched, %d insufficient pemission, %d missing, ",
> + MySubscription->name, (current_index /
> MAX_SEQUENCES_SYNC_PER_BATCH) + 1, batch_size,
> + batch_succeeded_count, batch_skipped_count,
> batch_mismatched_count, batch_insuffperm_count,
> + batch_size - (batch_succeeded_count + batch_skipped_count +
> batch_mismatched_count + batch_insuffperm_count)));
>
> pemission -> permission
>
> 3. I ran the ALTER SUBSCRIPTION .. REFRESH PUBLICATION command and
> DROP SEQUENCE command and got a warning for "leaked hash_seq_search
> scan". Is it expected?
>
> 2025-09-17 19:06:48.663 IST [2995060] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has started
> 2025-09-17 19:06:48.677 IST [2995060] LOG: logical replication
> sequence synchronization for subscription "sub1" - total
> unsynchronized: 0
> 2025-09-17 19:06:48.677 IST [2995060] WARNING: leaked hash_seq_search
> scan for hash table 0x62b0a61d3450
> 2025-09-17 19:06:48.677 IST [2995060] LOG: logical replication
> sequence synchronization worker for subscription "sub1" has finished
>
> Steps to reproduce:
> 1. create publication for ALL SEQUENCES and create a subscription for
> the publication on another node.
> 2. create sequence s1 both on publisher and subscriber.
> 3. Attach gdb on a psql terminal and add breakpoint at "line of
> function call to AddSubscriptionRelState" inside function
> AlterSubscription_refresh and run ALTER PUBLICATION .. REFRESH
> PUBLICATION command on psql terminal
> 4. DROP sequence s1 on another terminal(for subscriber)
> 5. Continue the gdb
> We will get the above warning.

Thanks for the comments, these are handled in the attached patch.

Regards,
Vignesh