On 20 Jan 2020, 09:45 +0300, Michael Paquier <mich...@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
