Re: Logical Replication of sequences

On Tue, 1 Jul 2025 at 15:20, shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> On Mon, Jun 30, 2025 at 3:21 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
> >
> > Please find the attached v20250630 patch set addressing above comments
> > and other comments in [1],[2],[3] and [4].
>
> Thanks for the patches. I am still in process of reviewing it but
> please find few comments:
>
> 1)
> + if (pset.sversion >= 180000)
> + appendPQExpBuffer(&buf,
> + ",\n puballsequences AS \"%s\"",
> + gettext_noop("All sequences"));
>
> The server version check throughout the patch can be modified to 19000
> as a new branch is created now.

Modified

> 2)
> + bool all_pub; /* Special publication for all tables,
> + * sequecnes */
>
> a) Typo: sequecnes --> sequences

Modified

> b) It is not clear from the comment that when will it be true? Will it
> be set when either of all-tables or all-sequences is given or does it
> need both?

Updated the comments

> 3)
> postgres=# create publication pub1 for all sequences WITH ( PUBLISH='delete');
> CREATE PUBLICATION
> postgres=# create publication pub2 for all tables, sequences WITH
> (PUBLISH='update');
> CREATE PUBLICATION
>
> For the first command, 'WITH ( publication_parameter..' is useless.
> For the second command, it is applicable only for 'all tables'.
>
> a) I am not sure if we even allow WITH in the first command?
> b) In the second command, even if we allow it, there should be some
> sort of NOTICE informing that it is applicable only to 'TABLES'.
>
> Thoughts?
>
> Also we allowed altering publication_parameter for all-sequences publication:
>
> postgres=# alter publication pub1 set (publish='insert,update');
> ALTER PUBLICATION
>
> c) Should this be restricted as well? Thoughts?

We have documented that the parameter of with clause is not applicable
for sequences. I feel that all the above statements are ok with the
documentation mentioned.

Regarding the comments from [1].
> 5)
> In LogicalRepSyncSequences, why are we allocating it in a permanent
> memory context?
>
> + /* Allocate the tracking info in a permanent memory context. */
> + oldctx = MemoryContextSwitchTo(CacheMemoryContext);
> + foreach_ptr(SubscriptionRelState, seq_state, sequences)
> + {
> + SubscriptionRelState *rstate = palloc(sizeof(SubscriptionRelState));
> +
> + memcpy(rstate, seq_state, sizeof(SubscriptionRelState));
> + sequences_not_synced = lappend(sequences_not_synced, rstate);
> + }
> + MemoryContextSwitchTo(oldctx);
>
> Same for 'seq_info' allocation.

When we are in between a transaction we will be using
TopTransactionContext. We can palloc() in TopTransactionContext and
safely use that memory throughout the transaction. But we cannot
cannot access memory allocated in TopTransactionContext after
CommitTransaction() finishes, because TopTransactionContext is
explicitly reset (or deleted) at the end of the transaction.
This is the reason we have to use CacheMemoryContext here.

The rest of the comments are fixed.

Also one of the pending comment from [2] is fixed.
> 4. Since we are not adding sequences in the list 'sub_remove_rels',
> should we only palloc for (the count of no. of tables)? Is it worth
> the effort?
> /*
> * Rels that we want to remove from subscription and drop any slots
> * and origins corresponding to them.
> */
> sub_remove_rels = palloc(subrel_count * sizeof(SubRemoveRels));

The attached v20250704 version patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAJpy0uD%2B7UtDs9%3DCx03BAckMPDdW7C6ifGF_Lc54B8iH6RNXWQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CANhcyEWKhHWFzpdAF6czbwq76NRDNCecDqQNtN6Bomn26mqHFw%40mail.gmail.com

Regards,
Vignesh