Hi, Alexander! On Thu, Oct 23, 2025 at 8:58 PM Xuneng Zhou <[email protected]> wrote: > > Hi, > > On Thu, Oct 23, 2025 at 6:46 PM Alexander Korotkov <[email protected]> > wrote: > > > > Hi! > > > > In Thu, Oct 16, 2025 at 10:12 AM Xuneng Zhou <[email protected]> wrote: > > > On Wed, Oct 15, 2025 at 8:48 PM Xuneng Zhou <[email protected]> wrote: > > > > > > > > Hi, > > > > > > > > Thank you for the grammar review and the clear recommendation. > > > > > > > > On Wed, Oct 15, 2025 at 4:51 PM Álvaro Herrera <[email protected]> > > > > 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
v18-0001-Add-pairingheap_initialize-for-shared-memory-usa.patch
Description: Binary data
v18-0002-Add-infrastructure-for-efficient-LSN-waiting.patch
Description: Binary data
v18-0003-Implement-WAIT-FOR-command.patch
Description: Binary data
