From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations. |
Date: | 2022-07-25 03:03:44 |
Message-ID: | CAHut+Ps8p1CuKE6M9mVpuUMTuzbKTPcvyNr0zzH2Zbq2WiiYfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <michael(at)paquier(dot)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.
====
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);
}
------
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2022-07-25 03:12:18 | Re: Collect ObjectAddress for ATTACH DETACH PARTITION to use in event trigger |
Previous Message | Kyotaro Horiguchi | 2022-07-25 03:02:22 | Re: Proposal to provide the facility to set binary format output for specific OID's per session |