Re: Physical replication slot advance is not persistent

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 */
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