On Mon, Jul 25, 2022 at 8:34 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <mich...@paquier.xyz> wrote:
> >
> > On Sun, Jul 24, 2022 at 09:52:16PM +0530, vignesh C wrote:
> > > Thanks for the comments, i have modified it by changing it to a
> > > boolean parameter. The attached v4 patch has the changes for the same.
> >
> > Okay, thanks for the patch. This looks good to me, so let's do as
> > Amit suggests. I'll apply that if there are no objections.
> > --
>
> OK. I have no objections to just passing a boolean, but here are a
> couple of other small review comments for the v4-0001 patch:
>
> ======
>
> 1. src/backend/catalog/pg_subscription.c
>
> @@ -533,65 +533,14 @@ HasSubscriptionRelations(Oid subid)
> }
>
> /*
> - * Get all relations for subscription.
> + * Get the relations for subscription.
> *
> - * Returned list is palloc'ed in current memory context.
> + * If only_not_ready is false, return all the relations for subscription. If
> + * true, return all the relations for subscription that are not in a ready
> + * state. Returned list is palloc'ed in current memory context.
> */
>
> The function comment was describing the new boolean parameter in a
> kind of backwards way. It seems more natural to emphasise what true
> means.
>
>
> SUGGESTION
> Get the relations for the subscription.
>
> If only_not_ready is true, return only the relations that are not in a
> ready state, otherwise return all the subscription relations. The
> returned list is palloc'ed in the current memory context.
Modified
> ====
>
> 2. <General - calling code>
>
> Perhaps this suggestion is overkill, but given that the param is not
> going to be a bitmask or enum anymore, IMO it means the calls are no
> longer very self-explanatory.The calling code will be more readable if
> the patch introduced some descriptive wrapper functions. e.g.
>
>
> List *
> GetSubscriptionAllRelations(Oid subid)
> {
> return GetSubscriptionRelations(subid, false);
> }
>
> List *
> GetSubscriptionNotReadyRelations(Oid subid)
> {
> return GetSubscriptionRelations(subid, true);
> }
I feel this would be an overkill, I did not make any changes for this.
Thanks for the comments, the attached v5 patch has the changes for the same.
Regards,
Vignesh
From 4172eeaef8f7890e6ff0148de65332264d039075 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C <vignesh21@gmail.com>
Date: Wed, 13 Jul 2022 11:54:59 +0530
Subject: [PATCH v5] 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.
Author: Vignesh C
Reviewed-By: Michael Paquier, Peter Smith, Amit Kapila, Kyotaro Horiguchi
Discussion: https://www.postgresql.org/message-id/flat/CALDaNm0eW-9g4G_EzHebnFT5zZoasWCS_EzZQ5BgnLZny9S%3Dpg%40mail.gmail.com
---
src/backend/catalog/pg_subscription.c | 70 +++------------------
src/backend/commands/subscriptioncmds.c | 4 +-
src/backend/replication/logical/tablesync.c | 2 +-
src/include/catalog/pg_subscription_rel.h | 3 +-
4 files changed, 14 insertions(+), 65 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 33ae3da8ae..a60fbaca26 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -533,65 +533,14 @@ HasSubscriptionRelations(Oid subid)
}
/*
- * Get all relations for subscription.
+ * Get the relations for the subscription.
*
- * Returned list is palloc'ed in current memory context.
+ * If not_ready is true, return only the relations that are not in a ready
+ * state, otherwise return all the subscription relations. The returned list is
+ * palloc'ed in the 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, bool not_ready)
{
List *res = NIL;
Relation rel;
@@ -607,10 +556,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 (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 bd0cc0848d..f73dfb6067 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -814,7 +814,7 @@ 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, false);
/*
* Build qsorted array of local table oids for faster lookup. This can
@@ -1494,7 +1494,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, true);
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..6a01ffd273 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1479,7 +1479,7 @@ FetchTableStates(bool *started_tx)
}
/* Fetch all non-ready tables. */
- rstates = GetSubscriptionNotReadyRelations(MySubscription->oid);
+ rstates = GetSubscriptionRelations(MySubscription->oid, true);
/* 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..8e88de7b2b 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -88,7 +88,6 @@ 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, bool not_ready);
#endif /* PG_SUBSCRIPTION_REL_H */
--
2.32.0
