At Fri, 16 Sep 2022 09:15:58 +0530, Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote in > On Thu, Sep 15, 2022 at 1:52 PM Kyotaro Horiguchi > <horikyota....@gmail.com> wrote: > > > > At Thu, 15 Sep 2022 10:28:12 +0530, Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote in > > > I'm attaching the v6 patch that's rebased on to the latest HEAD. > > > Please consider this for review. > > > > Thaks for the new version! > > > > +#define StreamingReplRetryEnabled() \ > > + (streaming_replication_retry_interval > 0 && \ > > + StandbyMode && \ > > + currentSource == XLOG_FROM_ARCHIVE) > > > > It seems to me a bit too complex..
In other words, it seems to me that the macro name doesn't manifest the condition correctly. > I don't think so, it just tells whether a standby is allowed to switch > source to stream from archive. > > > + /* Save the timestamp at which we're switching to > > archive. */ > > + if (StreamingReplRetryEnabled()) > > + switched_to_archive_at = > > GetCurrentTimestamp(); > > > > Anyway we are going to open a file just after this so > > GetCurrentTimestamp() cannot cause a perceptible degradation. > > Coulnd't we do that unconditionally, to get rid of the macro? > > Do we really need to do it unconditionally? I don't think so. And, we > can't get rid of the macro, as we need to check for the current > source, GUC and standby mode. When this feature is disabled, it > mustn't execute any extra code IMO. I don't think we don't particularly want to do that unconditionally. I wanted just to get rid of the macro from the usage site. Even if the same condition is used elsewhere, I see it better to write out the condition directly there.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center