Обсуждение: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.

Hi,

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?

Regards,
Vignesh

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.
--
Michael

On Wed, Jul 13, 2022 at 5:43 PM Michael Paquier <michael@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.

+1

(or some enum)

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <michael@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.

Regards,
Vignesh

On Wed, Jul 13, 2022 at 7:55 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <michael@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.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 7:55 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <michael@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

On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Wed, Jul 13, 2022 at 7:55 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 1:13 PM Michael Paquier <michael@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)
>

I think this will be an invalid combination if caller ever used it.
However, one might need to use a combination like
(SUBSCRIPTION_REL_STATE_READY | SUBSCRIPTION_REL_STATE_DONE). For such
cases, I feel the bitmask idea will be better.

-- 
With Regards,
Amit Kapila.

On Wed, Jul 20, 2022 at 02:32:44PM +0530, Amit Kapila wrote:
> On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>> 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)
>
> I think this will be an invalid combination if caller ever used it.
> However, one might need to use a combination like
> (SUBSCRIPTION_REL_STATE_READY | SUBSCRIPTION_REL_STATE_DONE). For such
> cases, I feel the bitmask idea will be better.

It feels unnatural to me to have a flag saying "not-ready" and one
saying "ready", while we could have a flag saying "ready" that can be
combined with a second flag to decide if the contents of srsubstate
should be matched or *not* matched with the states expected by the
caller.  This could be extended to more state values, for example.

I am not sure if we actually need this much as I have no idea if
future features would use it, so please take my suggestion lightly :)
--
Michael

On Thu, Jul 21, 2022 at 7:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 20, 2022 at 02:32:44PM +0530, Amit Kapila wrote:
> > On Thu, Jul 14, 2022 at 4:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >> 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)
> >
> > I think this will be an invalid combination if caller ever used it.
> > However, one might need to use a combination like
> > (SUBSCRIPTION_REL_STATE_READY | SUBSCRIPTION_REL_STATE_DONE). For such
> > cases, I feel the bitmask idea will be better.
>
> It feels unnatural to me to have a flag saying "not-ready" and one
> saying "ready", while we could have a flag saying "ready" that can be
> combined with a second flag to decide if the contents of srsubstate
> should be matched or *not* matched with the states expected by the
> caller.  This could be extended to more state values, for example.
>
> I am not sure if we actually need this much as I have no idea if
> future features would use it, so please take my suggestion lightly :)
>

Yeah, it is not very clear to me either. I think this won't be
difficult to change one or another way depending on future needs. At
this stage, it appeared to me that bitmask is a better way to
represent this information but if you and other feels using enum is a
better idea then I am fine with that as well.

-- 
With Regards,
Amit Kapila.

On Thu, Jul 21, 2022 at 09:54:05AM +0530, Amit Kapila wrote:
> Yeah, it is not very clear to me either. I think this won't be
> difficult to change one or another way depending on future needs. At
> this stage, it appeared to me that bitmask is a better way to
> represent this information but if you and other feels using enum is a
> better idea then I am fine with that as well.

Please don't get me wrong :)

I favor a bitmask over an enum here, as you do, with a clean
layer for those flags.
--
Michael

On Thu, Jul 21, 2022 at 10:03 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 21, 2022 at 09:54:05AM +0530, Amit Kapila wrote:
> > Yeah, it is not very clear to me either. I think this won't be
> > difficult to change one or another way depending on future needs. At
> > this stage, it appeared to me that bitmask is a better way to
> > represent this information but if you and other feels using enum is a
> > better idea then I am fine with that as well.
>
> Please don't get me wrong :)
>
> I favor a bitmask over an enum here, as you do, with a clean
> layer for those flags.
>

Okay, let's see what Peter Smith has to say about this as he was in
favor of using enum here?

-- 
With Regards,
Amit Kapila.

On Thu, Jul 21, 2022 at 10:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 10:03 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Thu, Jul 21, 2022 at 09:54:05AM +0530, Amit Kapila wrote:
> > > Yeah, it is not very clear to me either. I think this won't be
> > > difficult to change one or another way depending on future needs. At
> > > this stage, it appeared to me that bitmask is a better way to
> > > represent this information but if you and other feels using enum is a
> > > better idea then I am fine with that as well.
> >
> > Please don't get me wrong :)
> >
> > I favor a bitmask over an enum here, as you do, with a clean
> > layer for those flags.
> >
>
> Okay, let's see what Peter Smith has to say about this as he was in
> favor of using enum here?
>

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)

Also, it may be better to add the bit constants for every one of the
current states, even if you are not needing to use all of them just
yet. In fact, I thought this patch probably can implement the fully
capable common function (i.e. capable of multiple keys etc) right now,
so there will be no need to revisit it again in the future.

------
[1] https://www.postgresql.org/message-id/Ytiuj4hLykTvBF46%40paquier.xyz

Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Jul 22, 2022 at 3:47 AM Peter Smith <smithpb2250@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.

> Also, it may be better to add the bit constants for every one of the
> current states, even if you are not needing to use all of them just
> yet. In fact, I thought this patch probably can implement the fully
> capable common function (i.e. capable of multiple keys etc) right now,
> so there will be no need to revisit it again in the future.
>

I don't know whether we need to go that far. Say for a year or so if
we don't have such a use case arising which appears to be quite likely
then one can question the need for additional defines.

-- 
With Regards,
Amit Kapila.

At Fri, 22 Jul 2022 11:11:23 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in 
> 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.

FWIW, I vote for (only_)not_ready after rading through the thread..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jul 22, 2022 at 11:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 22, 2022 at 3:47 AM Peter Smith <smithpb2250@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

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.
--
Michael

On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <michael@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.

====

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

On Mon, Jul 25, 2022 at 8:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <michael@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.
>

This suggestion sounds good. Also, I don't much like "only" in the
parameter name. I think the explanation makes it clear.

> ====
>
> 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.
>

I am not sure how much this will be helpful. This sounds like overkill.

-- 
With Regards,
Amit Kapila.

On Mon, Jul 25, 2022 at 8:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <michael@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

On Mon, Jul 25, 2022 at 10:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 25, 2022 at 8:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Mon, Jul 25, 2022 at 11:08 AM Michael Paquier <michael@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.
> >
>
> This suggestion sounds good. Also, I don't much like "only" in the
> parameter name. I think the explanation makes it clear.

I have changed the parameter name to not_ready. The v5 patch attached
at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm2sQD-bwMKavLyiogMBBrg3fx5PTaV5RyV8UiczR9K_tw%40mail.gmail.com

Regards,
Vignesh

On Wed, Jul 27, 2022 at 08:47:46AM +0530, vignesh C wrote:
> I feel this would be an overkill, I did not make any changes for this.

Agreed.  Using an extra layer of wrappers for that seems a bit too
much, so I have applied your v5 with a slight tweak to the comment.
--
Michael

On Wed, Jul 27, 2022 at 4:22 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 27, 2022 at 08:47:46AM +0530, vignesh C wrote:
> > I feel this would be an overkill, I did not make any changes for this.
>
> Agreed.  Using an extra layer of wrappers for that seems a bit too
> much, so I have applied your v5 with a slight tweak to the comment.

Thanks for pushing this patch.

Regards,
Vignesh