From: a.kondratov@postgrespro.ru
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Cc: simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
Date: Mon, 20 Jan 2020 17:50:06 +0300
Message-ID: <ecaf244e-322d-4e17-9b34-d2513a79da24@Spark> (raw)
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>
On 20 Jan 2020, 09:45 +0300, Michael Paquier <michael@paquier.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.
>
> Thoughts?
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 same purpose and slot saving routines check it automatically. 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 explicit.
Just eyeballed your patch and it looks fine at a first glance, excepting the logical slot advance part:
retlsn = MyReplicationSlot->data.confirmed_flush;
+ *advance_done = true;
/* free context, call shutdown callback */
FreeDecodingContext(ctx);
I am not sure that this is a right place to set advance_done flag to true. Wouldn’t it be set to true even in the case of no-op, when LogicalConfirmReceivedLocation was never executed? Probably we should set the flag near the LogicalConfirmReceivedLocation call?
--
Alexey Kondratov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, michael@paquier.xyz, horikyota.ntt@gmail.com, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <ecaf244e-322d-4e17-9b34-d2513a79da24@Spark>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox