> The proposed patch requires us to create one function per option. I'm > concerned that it could cause bloating functions and be overkill just > to save changing a separate list. I would suggest starting with > putting a validation check for options at the top of foreach() loop. > When adding a new COPY option in the future, it wouldn't work if we > miss either changing the valid-options list or handling the option, > which seems a good prevention. >
Yep, Im +1 on that. "bloating functions" - that's precise wording, I did not know how to explain the same concern.
-- Best regards, Kirill Reshke
Thanks everyone for reviewing my proposal.
> The proposed patch requires us to create one function per option. I'm > concerned that it could cause bloating functions and be overkill just > to save changing a separate list. I would suggest starting with > putting a validation check for options at the top of foreach() loop. > When adding a new COPY option in the future, it wouldn't work if we > miss either changing the valid-options list or handling the option, > which seems a good prevention.
I completely agree with your feedback. I will proceed with a smaller and
simpler revision of the changes. The v3 patch was beneficial in that it
removed duplicated option names for COPY options in the code, but it
introduced too much refactoring for such a small improvement.
Attached is the new patch. One possible discussion point is that I choose FATAL
over ERROR at src/backend/commands/copy.c#L816, since reaching that point would
indicate an implementation problem. Please let me know if there is a better option.
