Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.

On Mon, Jul 25, 2022 at 10:22 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jul 25, 2022 at 8:34 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > 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.
> >
>
> 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