On Mon, Aug 18, 2025 at 3:36 PM vignesh C <vignes...@gmail.com> wrote:
>
> Thanks for the comments, the updated version has the changes for the same.
>
I wanted to first discuss a few design points. The patch implements
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES" such that it
copies the existing sequences values and also adds/removes any missing
sequences. For the second part (add/remove sequences), we already have
a separate command "ALTER SUBSCRIPTION ... REFRESH PUBLICATION". So, I
feel the new command should only copy the sequence values, as that
will keep the interface easy to define and understand. Additionally,
it will help to simplify the code in the patch, especially in the
function AlterSubscription_refresh.
We previously discussed *not* to launch an apply worker if the
corresponding publication(s) only publish sequences. See [1]. We
should consider it again to see if that is a good idea. It will have
some drawbacks as compared to the current approach of doing sync via
sync worker. The command could take time for a large number of
sequences, and on failure, retry won't happen which can happen with
background workers. Additionally, when the connect option is false for
a subscription during creation, the user needs to later call REFRESH
to sync the sequences after enabling the subscription. OTOH, doing the
sync during the command will bring more predictability and simplify
the patch. What do others think?
A few other comments:
1.
If the publication includes tables as well,
+ * issue a warning.
+ */
+ if (!stmt->for_all_tables)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("WITH clause parameters are not supported for publications
defined as FOR ALL SEQUENCES"));
+
+ ereport(NOTICE,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("WITH clause parameters are not applicable to sequence
synchronization and will be ignored"));
Though we are issuing a NOTICE but the comment refers to WARNING.
2.
/*
- * In case of ALTER SUBSCRIPTION ... REFRESH, subrel_local_oids contains
- * the list of relation oids that are already present on the subscriber.
- * This check should be skipped for these tables if checking for table
- * sync scenario. However, when handling the retain_dead_tuples scenario,
- * ensure all tables are checked, as some existing tables may now include
- * changes from other origins due to newly created subscriptions on the
- * publisher.
+ * In case of ALTER SUBSCRIPTION ... REFRESH PUBLICATION,
+ * subrel_local_oids contains the list of relation oids that are already
+ * present on the subscriber. This check should be skipped for these
+ * tables if checking for table sync scenario. However, when handling the
+ * retain_dead_tuples scenario, ensure all tables are checked, as some
+ * existing tables may now include changes from other origins due to newly
+ * created subscriptions on the publisher.
IIUC, this and other similar comments and err_message changes are just
using REFRESH PUBLICATION instead of REFRESH because now we have added
a SEQUENCES alternative as well. If so, let's make this a refactoring
patch for this just before the 0004 patch?
3.
ALTER_SUBSCRIPTION_DROP_PUBLICATION,
- ALTER_SUBSCRIPTION_REFRESH,
+ ALTER_SUBSCRIPTION_REFRESH_PUBLICATION,
+ ALTER_SUBSCRIPTION_REFRESH_PUBLICATION_SEQUENCES,
The length of the new type seems a bit longer. Can we try to slightly
reduce it by using ALTER_SUBSCRIPTION_REFRESH_PUBLICATION_SEQ?
4.
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION_SEQUENCES:
+ {
+ if (!sub->enabled)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES is not
allowed for disabled subscriptions"));
- AlterSubscription_refresh(sub, opts.copy_data, NULL);
+ PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ...
REFRESH PUBLICATION SEQUENCES");
Is there a need to restrict this new command in a transaction block?
We restrict other commands because those can lead to a drop of slots
that can't be rolled back whereas the sequencesync doesn't use slots,
so it should be okay to allow this new command in the transaction
block.
5.
static void
AlterSubscription_refresh(Subscription *sub, bool copy_data,
- List *validate_publications)
+ List *validate_publications, bool resync_all_sequences)
…
+ if (resync_all_sequences)
+ {
+ UpdateSubscriptionRelState(sub->oid, relid, SUBREL_STATE_INIT,
+ InvalidXLogRecPtr);
…
During refresh, we are re-initializing the sequence state by marking
its previously synced LSN to InvalidXLogRecPtr and relation state as
SUBREL_STATE_INIT. This will lose its previously synced value, and
also changing it to SUBREL_STATE_INIT also doesn't sound intuitive,
even though it serves the purpose. I feel it is better to use
SUBREL_STATE_DATASYNC state as that indicates data is being
synchronized, and let the LSN value be the same as the previous.
[1]:
https://www.postgresql.org/message-id/CAA4eK1LcBoPBCKa9yFOQnvpBv3a2ejf_EWC%3DZKksGcvqW7e0Zg%40mail.gmail.com
--
With Regards,
Amit Kapila.
