Re: review: psql: edit function, show function commands patch

On Tue, Aug 10, 2010 at 11:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The \e patch definitely needs another read-through.  I noticed a number
> of comments that were still pretty poor English, and one ---
>        /* skip header lines */
> --- that seems just plain wrong.  The actual intent of that next bit is
> to increase lineno to account for header lines, which is not well
> conveyed by "skip".

Interestingly, I had already rewritten pretty much every comment in
the patch, and the entirety of the documentation, but I found a very
small number of stragglers this morning and made a few more
adjustments. If you're still unhappy with it, you're going to need to
be more specific, or hack on it yourself.

> BTW, at least in the usage in that loop, get_functiondef_dollarquote_tag
> seems grossly overdesigned.  It would be clearer, shorter, and faster if
> you just had a strncmp test for "AS $function" there.

As far as I can see, the only purpose of that code is to support the
desire to have \sf+ display **** rather than a line number for the
lines that FOLLOW the function body. But I'm wondering if we should
just forget about that and let the numbering run continuously from the
first "AS $function" line to end of file. That would get rid of a
bunch of rather grotty code in the \sf patch, also.

> Also, the entire
> thing is subject to misbehavior in the case of \e (as opposed to \ef),
> which really cannot safely assert() that it's reading the output of
> pg_get_functiondef().  My inclination is to pull that part out of
> do_edit and put it into \ef-specific code.

Oh, for pity's sake. I had thought that code WAS \ef-specific
(because it doesn't make any sense otherwise) but I see that you are
correct.

> Also, there seemed to be some gratuitous inconsistency in the handling
> of tests on line number variables, eg some places lineno > 0 and others
> lineno >= 1.

I think this is now fixed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company