Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rD8qM-0013V3-0e for pgsql-hackers@arkaria.postgresql.org; Tue, 12 Dec 2023 19:55:02 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1rD8qK-007qRA-NH for pgsql-hackers@arkaria.postgresql.org; Tue, 12 Dec 2023 19:55:00 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rD8qJ-007qR2-NQ for pgsql-hackers@lists.postgresql.org; Tue, 12 Dec 2023 19:55:00 +0000 Received: from out3-smtp.messagingengine.com ([66.111.4.27]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rD8qG-00BfhM-Ds for pgsql-hackers@lists.postgresql.org; Tue, 12 Dec 2023 19:54:59 +0000 Received: from compute2.internal (compute2.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 3E6455C0261; Tue, 12 Dec 2023 14:54:54 -0500 (EST) Received: from imap49 ([10.202.2.99]) by compute2.internal (MEProxy); Tue, 12 Dec 2023 14:54:54 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eulerto.com; h= cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm2; t=1702410894; x=1702497294; bh=2GqaOpNp3q 8S90BhiM9qe9Xn3zt4eXT5C5AwPu3UpMM=; b=bKOHbkiNTJllNtfD4zGaIpIEgz AHD6av7gWKRU106e2qPp0VkXbrF5CjoAGQvaIWDX99x2XlTbSvC25vtxak5NfHLS w85fzB2mPYzT11nxcMNrBzUnp4GWkPRSbEHZXNIoKSNpPztGoEEJPbHCjU9ZJo6C OXU5q2FiltRwNatItbmeRhpBvJQB8cumk8lF/SnZQkqyKiLKfsdWX6EK/uSkbLpn SpyBCh2zP+2kbZjC1WUsvSxO8lFyggLJWu0DU7STI3PRh9KuX/mIiQP2zVL26Y3p /egvVzC6/nmci0nxcuXhQfxTl4tsr42mtnbVqF/Fxc0MViLwZkR3xeKfBtbg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; t=1702410894; x=1702497294; bh=2GqaOpNp3q8S90BhiM9qe9Xn3zt4 eXT5C5AwPu3UpMM=; b=yEXa+dE+vKt/EnUuy3fFBdBw0aq/Oq/tP0pGfXreHkBT TXb3WWe+h+I89tm+cBL5ZareYY/1DfNduppNorLekXjXMA+50pPOv0+6RS9ya8kP uf74OLv7WO86VzEl9UpTnNXxTgygcL+9H8C0mE5RPg29NN8AbektauZqmwAvvwDS SisckNxXZa8c/WfHVstd3Jb5Fdr7T7ouse8emWxSJyOnV1wGy7saSw0ol7adKX2M f+WQ4pGCc/ZRHXNZyxTies9+90Z7zs/T275Hl701A/W7LLT13Pr198mCq9mS4pMW ReK6BSOI89mgVWCzVobcq+Q65WDYIvtsd+N0vCCMxA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrudelgedguddvkecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefofgggkfgjfhffhffvufgtsegrtderreerredtnecuhfhrohhmpedfgfhu lhgvrhcuvfgrvhgvihhrrgdfuceovghulhgvrhesvghulhgvrhhtohdrtghomheqnecugg ftrfgrthhtvghrnhepjeethfeuvdfgffdtteevudeftdejueffkeelveekhfevkeegleef hfejhedtjeegnecuffhomhgrihhnpegvnhhtvghrphhrihhsvggusgdrtghomhenucevlh hushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpegvuhhlvghrsegv uhhlvghrthhordgtohhm X-ME-Proxy: Feedback-ID: i0c21471d:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id EF8FF15A0092; Tue, 12 Dec 2023 14:54:53 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.9.0-alpha0-1283-g327e3ec917-fm-20231207.002-g327e3ec9 MIME-Version: 1.0 Message-Id: <3abb8c54-11de-4220-ad65-c2785df01124@app.fastmail.com> In-Reply-To: <11ff040c-2eca-421a-b74a-0e99937c2d93@gmail.com> References: <11ff040c-2eca-421a-b74a-0e99937c2d93@gmail.com> Date: Tue, 12 Dec 2023 16:54:32 -0300 From: "Euler Taveira" To: "Drouvot, Bertrand" , "PostgreSQL Hackers" Subject: Re: Move walreceiver state assignment (to WALRCV_STREAMING) in WalReceiverMain() Content-Type: multipart/alternative; boundary=d79b6ab2fd394a37bfbfbc1186eaa485 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --d79b6ab2fd394a37bfbfbc1186eaa485 Content-Type: text/plain On Tue, Dec 12, 2023, at 12:58 PM, Drouvot, Bertrand wrote: > Currently walrcv->walRcvState is set to WALRCV_STREAMING at the > beginning of WalReceiverMain(). > > But it seems that after this assignment things could be wrong before the > walreicever actually starts streaming (like not being able to connect > to the primary). > > It looks to me that WALRCV_STREAMING should be set once walrcv_startstreaming() > returns true: this is the proposal of this patch. Per the state name (streaming), it seems it should be set later as you proposed, however, I'm concerned about the previous state (WALRCV_STARTING). If I'm reading the code correctly, WALRCV_STARTING is assigned at RequestXLogStreaming(): SetInstallXLogFileSegmentActive(); RequestXLogStreaming(tli, ptr, PrimaryConnInfo, PrimarySlotName, wal_receiver_create_temp_slot); flushedUpto = 0; } /* * Check if WAL receiver is active or wait to start up. */ if (!WalRcvStreaming()) { lastSourceFailed = true; break; } After a few lines the function WalRcvStreaming() has: SpinLockRelease(&walrcv->mutex); /* * If it has taken too long for walreceiver to start up, give up. Setting * the state to STOPPED ensures that if walreceiver later does start up * after all, it will see that it's not supposed to be running and die * without doing anything. */ if (state == WALRCV_STARTING) { pg_time_t now = (pg_time_t) time(NULL); if ((now - startTime) > WALRCV_STARTUP_TIMEOUT) { bool stopped = false; SpinLockAcquire(&walrcv->mutex); if (walrcv->walRcvState == WALRCV_STARTING) { state = walrcv->walRcvState = WALRCV_STOPPED; stopped = true; } SpinLockRelease(&walrcv->mutex); if (stopped) ConditionVariableBroadcast(&walrcv->walRcvStoppedCV); } } Couldn't it give up before starting if you apply your patch? My main concern is due to a slow system, the walrcv_connect() took to long in WalReceiverMain() and the code above kills the walreceiver while in the process to start it. Since you cannot control the hardcoded WALRCV_STARTUP_TIMEOUT value, you might have issues during overload periods. -- Euler Taveira EDB https://www.enterprisedb.com/ --d79b6ab2fd394a37bfbfbc1186eaa485 Content-Type: text/html Content-Transfer-Encoding: quoted-printable
On Tue, Dec 12,= 2023, at 12:58 PM, Drouvot, Bertrand wrote:
Currently walrcv->walRcvState is set= to WALRCV_STREAMING at the
beginning of WalReceiverMain()= .

But it seems that after this assignment t= hings could be wrong before the
walreicever actually start= s streaming (like not being able to connect
to the primary= ).

It looks to me that WALRCV_STREAMING sho= uld be set once walrcv_startstreaming()
returns true: this= is the proposal of this patch.

Per the state name (streaming), it seems it should be set later as you=
proposed, however, I'm concerned about the previous state= (WALRCV_STARTING). If
I'm reading the code correctly, WAL= RCV_STARTING is assigned at
RequestXLogStreaming():

        &= nbsp;           &= nbsp;   SetInstallXLogFileSegmentActive();
 = ;            = ;           RequestXLo= gStreaming(tli, ptr, PrimaryConnInfo,
   &n= bsp;           &n= bsp;           &n= bsp;           &n= bsp;     PrimarySlotName,
  =             =             =             =        wal_receiver_create_temp_slot);
=
          &= nbsp;           &= nbsp; flushedUpto =3D 0; 
    &nb= sp;           &nb= sp;   }

    &= nbsp;           &= nbsp;   /*
      &= nbsp;           &= nbsp;  * Check if WAL receiver is active or wait to start up.
          &nb= sp;          */
           &nb= sp;        if (!WalRcvStreaming())
          =           {  &nbs= p; 
        &= nbsp;           &= nbsp;   lastSourceFailed =3D true;
  &= nbsp;           &= nbsp;         break;
           &nb= sp;        }

=
After a few lines the function WalRcvStreaming() has:

    SpinLockRelease(&walrcv->mutex= );

    /*  
     * If it has taken too long for walrecei= ver to start up, give up. Setting
    = * the state to STOPPED ensures that if walreceiver later does start up<= br>
     * after all, it will see that it'= s not supposed to be running and die
   &nb= sp; * without doing anything.
     */<= br>
    if (state =3D=3D WALRCV_STARTING)
    {   
 &nbs= p;      pg_time_t   now =3D (pg_time_= t) time(NULL);

    &nbs= p;   if ((now - startTime) > WALRCV_STARTUP_TIMEOUT)
        {   
          = ;  bool        stopped =3D false= ;

       = ;     SpinLockAcquire(&walrcv->mutex);
          &nbs= p; if (walrcv->walRcvState =3D=3D WALRCV_STARTING)
&nbs= p;           { &n= bsp; 
        = ;        state =3D walrcv->walRcvS= tate =3D WALRCV_STOPPED;
     &nb= sp;          stopped =3D tr= ue;
         =    }
       &= nbsp;    SpinLockRelease(&walrcv->mutex);

        &nb= sp;   if (stopped)
     = ;           ConditionV= ariableBroadcast(&walrcv->walRcvStoppedCV);
 &= nbsp;      }
    }=    

Couldn't it give up befo= re starting if you apply your patch? My main concern is
du= e to a slow system, the walrcv_connect() took to long in WalReceiverMain= ()
and the code above kills the walreceiver while in the p= rocess to start it.
Since you cannot control the hardcoded= WALRCV_STARTUP_TIMEOUT value, you might
have issues durin= g overload periods.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

--d79b6ab2fd394a37bfbfbc1186eaa485--