On Fri, Jan 6, 2023 at 12:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > > 3.
> > > + else if (shmq_res == SHM_MQ_WOULD_BLOCK)
> > > + {
> > > + /* Replay the changes from the file, if any. */
> > > + if (pa_has_spooled_message_pending())
> > > + {
> > > + pa_spooled_messages();
> > > + }
> > >
> > > I think we do not need this pa_has_spooled_message_pending() function.
> > > Because this function is just calling pa_get_fileset_state() which is
> > > acquiring mutex and getting filestate then if the filestate is not
> > > FS_EMPTY then we call pa_spooled_messages() that will again call
> > > pa_get_fileset_state() which will again acquire mutex. I think when
> > > the state is FS_SERIALIZE_IN_PROGRESS it will frequently call
> > > pa_get_fileset_state() consecutively 2 times, and I think we can
> > > easily achieve the same behavior with just one call.
> > >
> >
> > This is just to keep the code easy to follow. As this would be a rare
> > case, so thought of giving preference to code clarity.
>
> I think the code will be simpler with just one function no? I mean
> instead of calling pa_has_spooled_message_pending() in if condition
> what if we directly call pa_spooled_messages();, this is anyway
> fetching the file_state and if the filestate is EMPTY then it can
> return false, and if it returns false we can execute the code which is
> there in else condition. We might need to change the name of the
> function though.
>
But anyway it is not a performance-critical path so if you think the
current way looks cleaner then I am fine with that too.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com