public inbox for [email protected]help / color / mirror / Atom feed
Re: [PATCH] Fix psql tab completion for REPACK boolean options 4+ messages / 3 participants [nested] [flat]
* Re: [PATCH] Fix psql tab completion for REPACK boolean options @ 2026-05-18 04:17 Fujii Masao <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Fujii Masao @ 2026-05-18 04:17 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Baji Shaik <[email protected]>; [email protected] On Fri, May 15, 2026 at 11:32 PM Álvaro Herrera <[email protected]> wrote: > > Second, when the same option is specified multiple times, the last value > > is not always honored. In particular, if any occurrence sets an option to ON, > > the option remains enabled even when the final setting is OFF. > > > > I think these are bugs, and the attached patch fixes them. Thoughts? > > I agree that these are bugs. The patch looks good to me on a cursory > look, though I didn't actually test it. Thanks for the review! I've tested it again and then pushed it. Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix psql tab completion for REPACK boolean options @ 2026-05-18 11:04 Thom Brown <[email protected]> 1 sibling, 1 reply; 4+ messages in thread From: Thom Brown @ 2026-05-18 11:04 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Baji Shaik <[email protected]>; [email protected] On Fri, 15 May 2026 at 10:36, Fujii Masao <[email protected]> wrote: > > On Wed, May 13, 2026 at 9:46 PM Álvaro Herrera <[email protected]> wrote: > > > Thanks for the patch! It looks good to me. > > > Unless there are objections, I will commit it. > > > > Yeah, looks good to me, thanks. > > I've pushed the patch. Thanks! > > > Fine with me. I wouldn't have a problem saying it's a backpatchable > > bugfix, but it's certainly not very high priority or criticality. > > Agreed. > > > BTW, while testing REPACK boolean options, I found two other issues > in their parsing. > > First, REPACK (CONCURRENTLY OFF) failed with: > > ERROR: unrecognized REPACK option "concurrently" > > even though REPACK (CONCURRENTLY ON) works correctly. That is, > CONCURRENTLY was treated as an unrecognized option when disabled. > > Second, when the same option is specified multiple times, the last value > is not always honored. In particular, if any occurrence sets an option to ON, > the option remains enabled even when the final setting is OFF. > > I think these are bugs, and the attached patch fixes them. Thoughts? I saw this just got committed. Is there a reason the option handling for this, VACUUM, and EXPLAIN aren't done in one place? Thom ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix psql tab completion for REPACK boolean options @ 2026-05-18 11:49 Álvaro Herrera <[email protected]> parent: Thom Brown <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Álvaro Herrera @ 2026-05-18 11:49 UTC (permalink / raw) To: Thom Brown <[email protected]>; +Cc: Fujii Masao <[email protected]>; Baji Shaik <[email protected]>; [email protected] On 2026-May-18, Thom Brown wrote: > I saw this just got committed. Is there a reason the option handling > for this, VACUUM, and EXPLAIN aren't done in one place? I don't think they take exactly the same options; but if you see room for improvement, by all means send a patch. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "This is a foot just waiting to be shot" (Andrew Dunstan) ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [PATCH] Fix psql tab completion for REPACK boolean options @ 2026-05-18 12:29 Thom Brown <[email protected]> parent: Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Thom Brown @ 2026-05-18 12:29 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Fujii Masao <[email protected]>; Baji Shaik <[email protected]>; [email protected] On Mon, 18 May 2026 at 12:49, Álvaro Herrera <[email protected]> wrote: > > On 2026-May-18, Thom Brown wrote: > > > I saw this just got committed. Is there a reason the option handling > > for this, VACUUM, and EXPLAIN aren't done in one place? > > I don't think they take exactly the same options; but if you see room > for improvement, by all means send a patch. I wasn't thinking of a shared handler for the options themselves, because, as you say, those differ per command. I meant the handling of the DefElem list: last-duplicate-wins, rejecting unknown options, coercion via defGetBoolean, etc. That part is the same everywhere, but each command writes its own set of checks, and the bug fixed here is what you get from that: a loop that enabled the option if any occurrence was ON, instead of just overwriting. So each command would hand its DefElem list to a common routine along with a table of valid option names and types. The routine iterates the list, rejects unknown names, coerces values, and applies last-wins for repeated options. Command-specific semantic checks would stay in the command. I could try writing a patch, but at this point in the dev cycle, it's probably something for later. Thom ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-05-18 12:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-18 04:17 ` Fujii Masao <[email protected]> 2026-05-18 11:04 ` Thom Brown <[email protected]> 2026-05-18 11:49 ` Álvaro Herrera <[email protected]> 2026-05-18 12:29 ` Thom Brown <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox