On Fri, Jul 22, 2022 at 11:11 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jul 22, 2022 at 3:47 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > I was in favour of enum mostly because I thought the bitmask of an > > earlier patch was mis-used; IMO each bit should only be for > > representing something as "on/set". So a bit for > > SUBSCRIPTION_REL_STATE_READY makes sense, but a bit for > > SUBSCRIPTION_REL_STATE_NOT_READY seemed strange/backwards to me. YMMV. > > > > So using a bitmask is fine, except I thought it should be implemented > > so that one of the bits is for a "NOT" modifier (IIUC this is kind of > > similar to what Michael [1] suggested above?). So "Not READY" would be > > (SUBSCRIPTION_REL_STATE_MOD_NOT | SUBSCRIPTION_REL_STATE_READY) > > > > Hmm, I think that sounds more complicated than what I expected. I > suggest let's go with a simple idea of using a boolean not_ready which > will decide whether to use the additional key to search. I feel we can > extend it by using a bitmask or enum when we have a clear need for > more states.
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. Regards, Vignesh
From e5edbf8dd47bbfcd7d8fcf3add8acab502e48665 Mon Sep 17 00:00:00 2001 From: Vigneshwaran C <vignes...@gmail.com> Date: Wed, 13 Jul 2022 11:54:59 +0530 Subject: [PATCH v4] 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..3d81449382 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 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. */ 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 only_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 (only_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..e835c42ef7 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 only_not_ready); #endif /* PG_SUBSCRIPTION_REL_H */ -- 2.32.0
