On Mon, Jul 28, 2025 at 3:37 PM vignesh C <vignes...@gmail.com> wrote:
> Thanks for the comments, the attached v20250728 version patch has the
> changes for the same.
>
Thanks for the patches, please find a few comments:
1)
WARNING: WITH clause parameters do not affect sequence synchronization
a)
How about:
WITH clause parameters are not applicable to sequence synchronization
or
WITH clause parameters are not applicable to sequence synchronization
and will be ignored.
b)
Should it be NOTICE OR WARNING? I feel NOTICE Is more appropriate as
it is more of an information than a warning since it has no negative
consequences.
2)
AlterSubscription_refresh(Subscription *sub, bool copy_data,
- List *validate_publications)
+ List *validate_publications, bool refresh_tables,
+ bool refresh_sequences, bool resync_all_sequences)
{
Do we need 3 new arguments? If we notice, 'refresh_sequences' is
always true in all cases. I feel only the last one should suffice.
IIUC, this is the state:
When resync_all_sequences is true:
it indicates it is 'REFRESH PUBLICATION SEQUENCES', that means we have
to refresh new sequences and resync all sequences.
When resync_all_sequences is false:
That means it is 'REFRESH PUBLICATION', we have to refresh new tables
and new sequences alone.
So if the caller pass only 'resync_all_sequences', we should be able
to drive the rest of the values internally.
3)
ALTER SUBSCRIPTION regress_testsub REFRESH PUBLICATION;
-ERROR: ALTER SUBSCRIPTION ... REFRESH cannot run inside a transaction block
+ERROR: ALTER SUBSCRIPTION ... REFRESH PUBLICATION cannot run inside
a transaction block
In the same script, we can test REFRESH PUBLICATION SEQUENCES also in
trancsation block.
4)
Commit message of patch004 says:
This patch introduce a new command to synchronize the sequences of
a subscription:
ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES
a)
introduce --> introduces
b)
We should also add:
This patch also changes the scope of
ALTER SUBSCRIPTION ... REFRESH PUBLICATION
This command now also considers sequences (newly added or dropped ones).
5)
+ * Reset the last_start_time of the sequencesync worker in the subscription's
+ * apply worker.
last_start_time-->last_seqsync_start_time
6)
alter_subscription.sgml has this:
<term><literal>refresh</literal> (<type>boolean</type>)</term>
<listitem>
<para>
When false, the command will not try to refresh table information.
<literal>REFRESH PUBLICATION</literal> should then be
executed separately.
The default is <literal>true</literal>.
</para>
</listitem>
</varlistentry>
Shouldn't we mention sequence too here:
When false, the command will not try to refresh table and sequence information.
thanks
Shveta
