Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1itYOD-0000u7-PF for pgsql-hackers@arkaria.postgresql.org; Mon, 20 Jan 2020 14:50:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1itYOA-00058x-57 for pgsql-hackers@arkaria.postgresql.org; Mon, 20 Jan 2020 14:50:50 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1itYO9-00058m-MA for pgsql-hackers@lists.postgresql.org; Mon, 20 Jan 2020 14:50:49 +0000 Received: from cyclops.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by makus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1itYO3-0001IO-0W for pgsql-hackers@postgresql.org; Mon, 20 Jan 2020 14:50:48 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id 44EEA21C4959; Mon, 20 Jan 2020 17:50:39 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from [10.32.153.127] (unknown [85.174.207.249]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id C039821C4954; Mon, 20 Jan 2020 17:50:38 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1579531839; bh=4khmSSGK3U97CaFTU0M8iwJnnM3zB1rJqsjSR/2pHi0=; h=Date:From:To:Cc:In-Reply-To:References:Subject; b=kywwY3aM+w+DnER0+U8/flLiewQ3a49V46fNGvj4+RH/IkC11wqJtcLFqcGjKGSL5 l3qeMk0VXtIDXPjPmH8qNXemYK6lLywQ7wh5AHbjUN3SWoPJ0ajyIIbe/0TAmDXB6S 4/K4pIn+Cg0JoYCMbJ24MCYnBGcEklIwYLAS001s= Date: Mon, 20 Jan 2020 17:50:06 +0300 From: a.kondratov@postgrespro.ru To: Michael Paquier Cc: Kyotaro Horiguchi , pgsql-hackers@postgresql.org, simon@2ndquadrant.com Message-ID: In-Reply-To: <20200120064540.GA57042@paquier.xyz> References: <20191226.173349.2159296357637287391.horikyota.ntt@gmail.com> <682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru> <175c2760666a78205e053207794c0f8f@postgrespro.ru> <20200109.153607.1891950504261689123.horikyota.ntt@gmail.com> <298e5b24-2628-3be0-840c-9ed6a45127ed@postgrespro.ru> <20200120064540.GA57042@paquier.xyz> Subject: Re: Physical replication slot advance is not persistent X-Readdle-Message-ID: ecaf244e-322d-4e17-9b34-d2513a79da24@Spark MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="5e25be3d_71f32454_6ef8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --5e25be3d_71f32454_6ef8 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On 20 Jan 2020, 09:45 +0300, Michael Paquier , wro= te: > > So, I have been looking at this patch by myself, and updated it so as > the extra slot save is done only if any advancing has been done, on > top of the other computations that had better be around for > consistency. The patch includes TAP tests for physical and logical > slots' durability across restarts. > > Thoughts=3F I still think that this extra check of whether any advance just happened = or not just adds extra complexity. We could use slot dirtiness for the sa= me purpose and slot saving routines check it automatically. Anyway, appro= ach with adding a new flag should resolve this bug as well, of course, an= d maybe it will be a bit more transparent and explicit. Just eyeballed your patch and it looks fine at a first glance, excepting = the logical slot advance part: retlsn =3D MyReplicationSlot->data.confirmed=5Fflush; + *advance=5Fdone =3D true; /* free context, call shutdown callback */ =46reeDecodingContext(ctx); I am not sure that this is a right place to set advance=5Fdone flag to tr= ue. Wouldn=E2=80=99t it be set to true even in the case of no-op, when Lo= gicalConfirmReceivedLocation was never executed=3F Probably we should set= the flag near the LogicalConfirmReceivedLocation call=3F -- Alexey Kondratov --5e25be3d_71f32454_6ef8 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline
On 20 Jan 2020, 09:45 +0300, Michae= l Paquier <michael=40paquier.xyz>, wrote:
So, I have been looking at this patch by myself, and updated it so as
the extra slot save is done only if any advancing has been done, on
= top of the other computations that had better be around for
consistency. The patch includes TAP tests for physical and logical
slots' durability across restarts. &=23160;
Thoughts=3F&=23160;<= /blockquote>


I still think that this extra check of whether any = advance just happened or not just adds extra complexity. We could use slo= t dirtiness for the same purpose and slot saving routines check it automa= tically. Anyway, approach with adding a new flag should resolve this bug = as well, of course, and maybe it will be a bit more transparent and expli= cit.

Just eyeballed your patch and it looks fine at a fi= rst glance, excepting the logical slot advance part:

            retlsn =3D MyReplicationSlot->data.confirmed=5Fflu=
sh;
+               *advance=5Fdone =3D true;
 =20
                /* free context, call shutdown callback */
                =46reeDecodingContext(ctx);

I am not sure that t=
his is a right place to set advance=5Fdone flag to true. Wouldn=E2=80=99t=
 it be set to true even in the case of no-op, when LogicalConfirmReceived=
Location was never executed=3F Probably we should set the flag near the L=
ogicalConfirmReceivedLocation call=3F


--
Alexey Kondratov

--5e25be3d_71f32454_6ef8--