From: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
---|---|
To: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Daniel Verite <daniel(at)manitou-mail(dot)org>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Greg Stark <stark(at)mit(dot)edu>, Erik Rijkers <er(at)xs4all(dot)nl>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, PostgreSQL <pgsql-hackers(at)postgresql(dot)org>, pgsql-hackers-owner(at)postgresql(dot)org |
Subject: | Re: \if, \elseif, \else, \endif (was Re: PSQL commands: \quit_if, \quit_unless) |
Date: | 2017-03-24 19:16:09 |
Message-ID: | CADkLM=cfm2ZhHP=4hqydT6DMAYT3G0t5YrxhPR+yCg0aOwiTDw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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.
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2017-03-24 19:21:18 | Re: Re: [COMMITTERS] pgsql: Implement multivariate n-distinct coefficients |
Previous Message | Robert Haas | 2017-03-24 19:13:06 | Re: pageinspect and hash indexes |