Re: \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) - Mailing list pgsql-hackers
| From | Corey Huinker |
|---|---|
| Subject | Re: \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) |
| Date | |
| Msg-id | CADkLM=cfm2ZhHP=4hqydT6DMAYT3G0t5YrxhPR+yCg0aOwiTDw@mail.gmail.com Whole thread Raw |
| In response to | Re: [HACKERS] \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless) ("Daniel Verite" <daniel@manitou-mail.org>) |
| Responses |
Re: \if, \elseif, \else, \endif (was Re: PSQL commands:\quit_if, \quit_unless)
(Fabien COELHO <coelho@cri.ensmp.fr>)
|
| List | pgsql-hackers |
A few comments about the patch.
Patch applies. "make check" ok.
As already pointed out, there is one useless file in the patch.
Although currently there is only one expected argument for boolean expressions, the n² concatenation algorithm in gather_boolean_expression is not very elegant. Is there some string buffer data structure which could be used instead?
I wished for the same thing, happy to use one if it is made known to me.
I pulled that pattern from somewhere else in the code, and given that the max number of args for a command is around 4, I'm not too worried about scaling.
ISTM that ignore_boolean_expression may call free on a NULL pointer if the expression is empty?
True. The psql code is actually littered with a lot of un-checked free(p) calls, so I started to wonder if maybe we had a wrapper on free() that checked for NULL. I'll fix this one just to be consistent.
Generally I find the per-command functions rather an improvement.
I did too. I tried to split this patch up into two parts, one that broke out the functions, and one that added if-then, and found that the first patch was just as unwieldily without the if-then stuff as with.
However there is an impact on testing because of all these changes. ISTM that test cases should reflect this situation and test that \cd, \edit, ... are indeed ignored properly and taking account there expected args...
I think one grand
\if false
\a
\c some_connect_string
...
\z some_table_name
\endif
should do the trick, but it wouldn't detect memory leaks.
In "exec_command_connect" an argument is changed from "-reuse-previous" to "-reuse-previous=", not sure why.
It shouldn't have been. Good catch. Most commands were able to be migrated with simple changes (status => *status, strcmp() if-block becomes active-if-block, etc), but that one was slightly different.
There seems to be pattern repetition for _ev _ef and _sf _sv. Would it make sense to create a function instead of keeping the initial copy-paste?
Yes, and a few things like that, but I wanted this patch to keep as much code as-is as possible.
I think that some functions could be used for some repeated cases such as "discard one arg", "discard one or two arg", "discard whole line", for the various inactive branches, so as to factor out code.
I'd be in favor of that as well
I would suggest to put together all if-related backslash command, so that the stack management is all in one function instead of 4.
I recognize the urge to group them together, but would there be any actual code sharing between them? Wouldn't I be either re-checking the string "cmd" again, or otherwise setting an enum that I immediately re-check inside the all_branching_commands() function?
For pset the inactive branch does OT_NORMAL instead of OT_NOT_EVAL, not sure why.
An oversight. Good catch.
pgsql-hackers by date: