Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations. - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.
Date
Msg-id CAA4eK1LC-vLKpSgPgr96FZfSaKt_TVJ3N_8fh-GTtSHQeU+EMg@mail.gmail.com
Whole thread Raw
In response to Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.  (Peter Smith <smithpb2250@gmail.com>)
Responses Re: Refactor to make use of a common function for GetSubscriptionRelations and GetSubscriptionNotReadyRelations.
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: predefined role(s) for VACUUM and ANALYZE
Next
From: Masahiko Sawada
Date:
Subject: Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns