Re: Potential data loss due to race condition during logical replication slot creation

On Wed, Jul 10, 2024 at 2:30 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Jul 10, 2024 at 12:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 10, 2024 at 7:52 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > Overall I think the patches are in good shape, so I'm going to push
> > > them tomorrow, barring any objections and further comments.
> > >
> >
> > Agreed. Below are a few minor comments that you might want to consider:
> >
> > 1.
> > @@ -76,6 +77,7 @@ extern SnapBuildState
> > SnapBuildCurrentState(SnapBuild *builder);
> > extern Snapshot SnapBuildGetOrBuildSnapshot(SnapBuild *builder);
> >
> > extern bool SnapBuildXactNeedsSkip(SnapBuild *builder, XLogRecPtr ptr);
> > +extern void SnapBuildSetFindStartPoint(SnapBuild *builder, bool
> > find_start_point);
> >
> > This is not required in the latest version.
> >
> > 2.
> > + /*
> > + * Indicates if we are using the snapshot builder for the initial creation
> > + * of a logical replication slot.
> >
> > The word 'initial' in the above comment is not required. If you
> > consider this then a similar wording change is required in lower
> > branches as well.
> >
> > 3.
> > HEAD and v17
> > ---------------------
> > - /* b) valid on disk state and not building full snapshot */
> > +
> > + /*
> > + * b) valid on disk state and while neither building full snapshot nor
> > + * finding the start point.
> > + */
> > else if (!builder->building_full_snapshot &&
> > + !builder->in_slot_creation &&
> >
> > V16 and below
> > ---------------------
> > - /* b) valid on disk state and not building full snapshot */
> > +
> > + /*
> > + * b) valid on disk state and neither building full snapshot nor while
> > + * creating a slot.
> > + */
> > else if (!builder->building_full_snapshot &&
> > + !ctx->in_create &&
> >
> > Isn't it better to use the same comment in both places?
>
> Thank you for reviewing the patches! I agreed with all the points.
> I've attached the updated patches.
>

Pushed, bb19b70081 (and all supported branches).

Regards,

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