Re: Implement waiting for wal lsn replay: reloaded

Hi, Alexander!

On Thu, Oct 23, 2025 at 8:58 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> Hi,
>
> On Thu, Oct 23, 2025 at 6:46 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi!
> >
> > In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thank you for the grammar review and the clear recommendation.
> > > >
> > > > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > > > >
> > > > > I didn't review the patch other than look at the grammar, but I disagree
> > > > > with using opt_with in it.  I think WITH should be a mandatory word, or
> > > > > just not be there at all.  The current formulation lets you do one of:
> > > > >
> > > > > 1. WAIT FOR LSN '123/456' WITH (opt = val);
> > > > > 2. WAIT FOR LSN '123/456' (opt = val);
> > > > > 3. WAIT FOR LSN '123/456';
> > > > >
> > > > > and I don't see why you need two ways to specify an option list.
> > > >
> > > > I agree with this as unnecessary choices are confusing.
> > > >
> > > > >
> > > > > So one option is to remove opt_wait_with_clause and just use
> > > > > opt_utility_option_list, which would remove the WITH keyword from there
> > > > > (ie. only keep 2 and 3 from the above list).  But I think that's worse:
> > > > > just look at the REPACK grammar[1], where we have to have additional
> > > > > productions for the optional parenthesized option list.
> > > > >
> > > > >
> > > > >
> > > > > So why not do just
> > > > >
> > > > > +opt_wait_with_clause:
> > > > > +           WITH '(' utility_option_list ')'        { $$ = $3; }
> > > > > +           | /*EMPTY*/                             { $$ = NIL; }
> > > > > +           ;
> > > > >
> > > > > which keeps options 1 and 3 of the list above.
> > > >
> > > > Your suggested approach of making WITH mandatory when options are
> > > > present looks better.
> > > > I've implemented the change as you recommended. Please see patch 3 in v16.
> > > >
> > > > >
> > > > >
> > > > >
> > > > > Note: you don't need to worry about WITH_LA, because that's only going
> > > > > to show up when the user writes WITH TIME or WITH ORDINALITY (see
> > > > > parser.c), and that's a syntax error anyway.
> > > > >
> > > >
> > > > Yeah, we require '(' immediately after WITH in our grammar, the
> > > > lookahead mechanism will keep it as regular WITH, and any attempt to
> > > > write "WITH TIME" or "WITH ORDINALITY" would be a syntax error anyway,
> > > > which is expected.
> > > >
> > >
> > > The filename of patch 1 is incorrect due to coping. Just correct it.
> >
> > Thank you for rebasing the patch.
> >
> > I've revised it.  The significant changes has been made to 0002, where
> > I reduced the code duplication.  Also, I run pgindent and pgperltidy
> > and made other small improvements.
> > Please, check.
>
> Thanks for updating the patch set!
> Patch 2 looks more elegant after the revision. I’ll review them soon.

I’ve made a few minor updates to the comments and docs in patches 2
and 3. The patch set LGTM now.

Best,
Xuneng