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

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.

Regards,

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