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

On Thu, Jul 21, 2022 at 10:11 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jul 21, 2022 at 10:03 AM Michael Paquier <michael(at)paquier(dot)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