On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 7:55 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <mich...@paquier.xyz> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 12:22:06PM +0530, vignesh C wrote:
> > > > Most of the code is common between GetSubscriptionRelations and
> > > > GetSubscriptionNotReadyRelations. Added a parameter to
> > > > GetSubscriptionRelations which could provide the same functionality as
> > > > the existing GetSubscriptionRelations and
> > > > GetSubscriptionNotReadyRelations. Attached patch has the changes for
> > > > the same. Thoughts?
> > >
> > > Right. Using all_rels to mean that we'd filter relations that are not
> > > ready is a bit confusing, though. Perhaps this could use a bitmask as
> > > argument.
> >
> > The attached v2 patch has the modified version which includes the
> > changes to make the argument as bitmask.
> >
>
> By using a bitmask I think there is an implication that the flags can
> be combined...
>
> Perhaps it is not a problem today, but later you may want more flags. e.g.
> #define SUBSCRIPTION_REL_STATE_READY 0x02 /* READY relations */
>
> then the bitmask idea falls apart because IIUC you have no intentions
> to permit things like:
> (SUBSCRIPTION_REL_STATE_NOT_READY | SUBSCRIPTION_REL_STATE_READY)
>
> IMO using an enum might be a better choice for that parameter.
Changed it to enum so that it can be extended to support other
subscription relations like ready state subscription relations later
easily. The attached v3 patch has the changes for the same.
Regards,
Vignesh
From e0186ae685acb6334b711ea821b287be61d6cbd3 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C <vignes...@gmail.com>
Date: Wed, 13 Jul 2022 11:54:59 +0530
Subject: [PATCH v3] Refactor to make use of a common function for
GetSubscriptionRelations and GetSubscriptionNotReadyRelations.
Most of the code is common between GetSubscriptionRelations and
GetSubscriptionNotReadyRelations. Added a parameter to GetSubscriptionRelations
which could provide the same functionality for GetSubscriptionRelations
and GetSubscriptionNotReadyRelations.
---
src/backend/catalog/pg_subscription.c | 71 ++++-----------------
src/backend/commands/subscriptioncmds.c | 5 +-
src/backend/replication/logical/tablesync.c | 3 +-
src/include/catalog/pg_subscription_rel.h | 19 +++++-
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 34 insertions(+), 65 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 8856ce3b50..46eedbc062 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -525,65 +525,15 @@ HasSubscriptionRelations(Oid subid)
}
/*
- * Get all relations for subscription.
+ * Get the relations for subscription.
*
- * Returned list is palloc'ed in current memory context.
+ * If subrel_options is SUBSCRIPTION_REL_ALL, return all the relations for
+ * subscription. If SUBSCRIPTION_REL_NOT_READY, return all the relations for
+ * subscription that are not in a ready state. Returned list is palloc'ed in
+ * current memory context.
*/
List *
-GetSubscriptionRelations(Oid subid)
-{
- List *res = NIL;
- Relation rel;
- HeapTuple tup;
- ScanKeyData skey[1];
- SysScanDesc scan;
-
- rel = table_open(SubscriptionRelRelationId, AccessShareLock);
-
- ScanKeyInit(&skey[0],
- Anum_pg_subscription_rel_srsubid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(subid));
-
- scan = systable_beginscan(rel, InvalidOid, false,
- NULL, 1, skey);
-
- while (HeapTupleIsValid(tup = systable_getnext(scan)))
- {
- Form_pg_subscription_rel subrel;
- SubscriptionRelState *relstate;
- Datum d;
- bool isnull;
-
- subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);
-
- relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState));
- relstate->relid = subrel->srrelid;
- relstate->state = subrel->srsubstate;
- d = SysCacheGetAttr(SUBSCRIPTIONRELMAP, tup,
- Anum_pg_subscription_rel_srsublsn, &isnull);
- if (isnull)
- relstate->lsn = InvalidXLogRecPtr;
- else
- relstate->lsn = DatumGetLSN(d);
-
- res = lappend(res, relstate);
- }
-
- /* Cleanup */
- systable_endscan(scan);
- table_close(rel, AccessShareLock);
-
- return res;
-}
-
-/*
- * Get all relations for subscription that are not in a ready state.
- *
- * Returned list is palloc'ed in current memory context.
- */
-List *
-GetSubscriptionNotReadyRelations(Oid subid)
+GetSubscriptionRelations(Oid subid, SubscriptionRelationState subrel_state)
{
List *res = NIL;
Relation rel;
@@ -599,10 +549,11 @@ GetSubscriptionNotReadyRelations(Oid subid)
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(subid));
- ScanKeyInit(&skey[nkeys++],
- Anum_pg_subscription_rel_srsubstate,
- BTEqualStrategyNumber, F_CHARNE,
- CharGetDatum(SUBREL_STATE_READY));
+ if (subrel_state == SUBSCRIPTION_REL_NOT_READY)
+ ScanKeyInit(&skey[nkeys++],
+ Anum_pg_subscription_rel_srsubstate,
+ BTEqualStrategyNumber, F_CHARNE,
+ CharGetDatum(SUBREL_STATE_READY));
scan = systable_beginscan(rel, InvalidOid, false,
NULL, nkeys, skey);
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index bdc1208724..ac517dd539 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -785,7 +785,8 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
pubrel_names = fetch_table_list(wrconn, sub->publications);
/* Get local table list. */
- subrel_states = GetSubscriptionRelations(sub->oid);
+ subrel_states = GetSubscriptionRelations(sub->oid,
+ SUBSCRIPTION_REL_ALL);
/*
* Build qsorted array of local table oids for faster lookup. This can
@@ -1457,7 +1458,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
* the apply and tablesync workers and they can't restart because of
* exclusive lock on the subscription.
*/
- rstates = GetSubscriptionNotReadyRelations(subid);
+ rstates = GetSubscriptionRelations(subid, SUBSCRIPTION_REL_NOT_READY);
foreach(lc, rstates)
{
SubscriptionRelState *rstate = (SubscriptionRelState *) lfirst(lc);
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 670c6fcada..b072d9c349 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1479,7 +1479,8 @@ FetchTableStates(bool *started_tx)
}
/* Fetch all non-ready tables. */
- rstates = GetSubscriptionNotReadyRelations(MySubscription->oid);
+ rstates = GetSubscriptionRelations(MySubscription->oid,
+ SUBSCRIPTION_REL_NOT_READY);
/* Allocate the tracking info in a permanent memory context. */
oldctx = MemoryContextSwitchTo(CacheMemoryContext);
diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h
index 9df99c3418..a4a45997b6 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -80,6 +80,21 @@ typedef struct SubscriptionRelState
char state;
} SubscriptionRelState;
+/*---------
+ * Expected values for subrel_state parameter of GetSubscriptionRelations(),
+ * which allows callers to specify which relations of the subscriptions they
+ * expect to see.
+ *
+ * SUBSCRIPTION_REL_ALL: all the subscription relations
+ * SUBSCRIPTION_REL_NOT_READY: all the subscription relations that are not
+ * in ready state.
+ */
+typedef enum SubscriptionRelationState
+{
+ SUBSCRIPTION_REL_ALL,
+ SUBSCRIPTION_REL_NOT_READY
+} SubscriptionRelationState;
+
extern void AddSubscriptionRelState(Oid subid, Oid relid, char state,
XLogRecPtr sublsn);
extern void UpdateSubscriptionRelState(Oid subid, Oid relid, char state,
@@ -88,7 +103,7 @@ extern char GetSubscriptionRelState(Oid subid, Oid relid, XLogRecPtr *sublsn);
extern void RemoveSubscriptionRel(Oid subid, Oid relid);
extern bool HasSubscriptionRelations(Oid subid);
-extern List *GetSubscriptionRelations(Oid subid);
-extern List *GetSubscriptionNotReadyRelations(Oid subid);
+extern List *GetSubscriptionRelations(Oid subid,
+ SubscriptionRelationState subrel_state);
#endif /* PG_SUBSCRIPTION_REL_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 34a76ceb60..d2ff829735 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2641,6 +2641,7 @@ SubscriptingRefState
Subscription
SubscriptionInfo
SubscriptionRelState
+SubscriptionRelationState
SupportRequestCost
SupportRequestIndexCondition
SupportRequestRows
--
2.32.0
