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

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