Re: logical replication: restart_lsn can go backwards (and more), seems broken since 9.4

On Fri, Nov 15, 2024 at 9:48 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
>
>
> On 11/15/24 18:40, Masahiko Sawada wrote:
> > On Thu, Nov 14, 2024 at 10:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>
> >> On Thu, Nov 14, 2024 at 7:08 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >>>
> >>> Sure. I've attached the updated patch. I just added the commit message.
> >>>
> >>
> >> @@ -1815,6 +1818,8 @@ LogicalIncreaseRestartDecodingForSlot(XLogRecPtr
> >> current_lsn, XLogRecPtr restart
> >> confirmed_flush = slot->data.confirmed_flush;
> >> SpinLockRelease(&slot->mutex);
> >>
> >> + spin_released = true;
> >> +
> >> elog(DEBUG1, "failed to increase restart lsn: proposed %X/%X, after
> >> %X/%X, current candidate %X/%X, current after %X/%X, flushed up to
> >> %X/%X",
> >> LSN_FORMAT_ARGS(restart_lsn),
> >> LSN_FORMAT_ARGS(current_lsn),
> >> @@ -1823,6 +1828,9 @@ LogicalIncreaseRestartDecodingForSlot(XLogRecPtr
> >> current_lsn, XLogRecPtr restart
> >> LSN_FORMAT_ARGS(confirmed_flush));
> >> }
> >>
> >> + if (!spin_released)
> >> + SpinLockRelease(&slot->mutex);
> >>
> >> This coding pattern looks odd to me. We can consider releasing
> >> spinlock in the other two if/else if checks. I understand it is a
> >> matter of individual preference, so, if you and or others prefer the
> >> current way, that is also fine with me. Other than this, the patch
> >> looks good to me.
> >
> > Indeed, I prefer your idea. I"ve attached the updated patch. I'll push
> > it early next week unless there are further comments.
> >
>
> I'm not particularly attached to how I did this in my WIP patch, it was
> simply the simplest way to make it work for experimentation. I'd imagine
> it'd be best to just mirror how LogicalIncreaseXminForSlot() does this.

I was going to push it next Monday but we're going to have
out-of-cycle minor releases next week and we don't have to wait for
February minor releases given that we already agreed with the current
fix. So I pushed the fix.

Looking at buildfarm animal, it seems that alligator started complain
a build error:

plancat.c: In function \342\200\230get_relation_info\342\200\231:
plancat.c:331:54: error: assignment to \342\200\230void
(*)(void)\342\200\231 from incompatible pointer type
\342\200\230amcostestimate_function\342\200\231 {aka \342\200\230void
(*)(struct PlannerInfo *, struct IndexPath *, double, double *,
double *, double *, double *, double *)\342\200\231}
[-Wincompatible-pointer-types]
331 | info->amcostestimate =
amroutine->amcostestimate;
| ^
make[4]: *** [<builtin>: plancat.o] Error 1

I think that it's not relevant with this fix and recent commits but
caused by changes happening in gcc[1] (it's using nightly build gcc).

From a success log, 5f28e6b[2]:

configure: using compiler=gcc (GCC) 15.0.0 20241115 (experimental)

On the other hand, from a failure log[3]:

configure: using compiler=gcc (GCC) 15.0.0 20241116 (experimental)

FYI it started to report build errors with 20241116 build also on v12
where this fix is not pushed.

Regards,

[1] likely https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=55e3bd376b2214e200fa76d12b67ff259b06c212
[2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=alligator&dt=2024-11-16%2000%3A20%3A53&stg=configure
[3] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=alligator&dt=2024-11-16%2002%3A14%3A05&stg=configure

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com