On Sat, Oct 19, 2024 at 11:33 PM Joel Jacobson <j...@compiler.org> wrote:
>
> > ProcessCopyOptions
> > /* Extract options from the statement node tree */
> > foreach(option, options)
> > {
> > }
> > /* --- DELIMITER option --- */
> > /* --- NULL option --- */
> > /* --- QUOTE option --- */
> > Currently the regress test passed, i think that means your refactor is fine.
>
> I believe that a passing test indicates it might be okay,
> but a failing test definitely means it's not. :D
>
> I've meticulously refactored one option at a time, checking which code in
> ProcessCopyOptions depends on each option field to ensure the semantics
> are preserved.
>
> I think the changes are easy to follow, and it's clear that each change is
> correct when looking at them individually, though it might be more challenging
> when viewing the total change.
>
> I've tried to minimize code movement, preserving as much of the original
> code placement as possible.
>
> > in ProcessCopyOptions, maybe we can rearrange the code after the
> > foreach loop (foreach(option, options)
> > based on the parameters order in
> > https://www.postgresql.org/docs/devel/sql-copy.html Parameters section.
> > so we can review it by comparing the refactoring with the
> > sql-copy.html Parameters section's description.
>
> That would be nice, but unfortunately, it's not possible because the order of
> the option code blocks matters due to the setting of defaults in else/else
> if branches when an option is not specified.
>
> For example, in the documentation, DEFAULT precedes QUOTE,
> but in ProcessCopyOptions, the QUOTE code block must come before
> the DEFAULT code block due to the check:
>
> /* Don't allow the CSV quote char to appear in the default string. */
>> I also believe there's value in minimizing code movement. > but v12-0001 was already hugely refactored. make the ProcessCopyOptions process in following order: 1. Extract options from the statement node tree 2. checking each option, if not there set default value. 3. checking for interdependent options I still think making step2 aligned with the doc parameter section order will make it more readable. based on your patch (v12-0001-Refactor-ProcessCopyOptions-introduce-CopyFormat-enu.patch) I put some checking to step3, make step2 checking order aligned with doc.
v12-0001-make-the-ProcessCopyOptions-option-aligned-wi.no-cfbot
Description: Binary data
