Re: Physical replication slot advance is not persistent

From: a(dot)kondratov(at)postgrespro(dot)ru
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, simon(at)2ndquadrant(dot)com
Subject: Re: Physical replication slot advance is not persistent
Date: 2020-01-20 14:50:06
Message-ID: ecaf244e-322d-4e17-9b34-d2513a79da24@Spark
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 20 Jan 2020, 09:45 +0300, Michael Paquier <michael(at)paquier(dot)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 */

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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Luis Carril 2020-01-20 15:04:56 Re: Option to dump foreign data in pg_dump
Previous Message Michael Paquier 2020-01-20 13:50:21 Re: TRUNCATE on foreign tables