Re: [patch] Proposal for \crosstabview in psql

From: "Daniel Verite" <daniel(at)manitou-mail(dot)org>
To: "Alvaro Herrera" <alvherre(at)2ndquadrant(dot)com>
Cc: "Pavel Stehule" <pavel(dot)stehule(at)gmail(dot)com>,"Robert Haas" <robertmhaas(at)gmail(dot)com>,"Dean Rasheed" <dean(dot)a(dot)rasheed(at)gmail(dot)com>,"Andres Freund" <andres(at)anarazel(dot)de>,"Teodor Sigaev" <teodor(at)sigaev(dot)ru>,"PostgreSQL Hackers" <pgsql-hackers(at)postgresql(dot)org>,"Jim Nasby" <Jim(dot)Nasby(at)bluetreble(dot)com>
Subject: Re: [patch] Proposal for \crosstabview in psql
Date: 2016-04-07 15:40:46
Message-ID: 75b7763e-eb09-4fbc-8f4e-435d9102735e@mm
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Alvaro Herrera wrote:

Thanks for looking into that patch!

> regression=# select * from pg_class \crosstabview relnatts
> \crosstabview: missing second argument
> regression-#

Fixed. This was modelled after the behavior of:
select 1 \badcommand
but I've changed to mimic what happens with:
select 1 \g /some/invalid/path
the query buffer is not discarded by the error but the prompt
is ready for a fresh new command.

> alvherre=# select * from pg_class \crosstabview relnatts relkinda
> Invalid column name: relkinda
> alvherre=# select 1;
> The query must return at least two columns to be shown in crosstab

Definitely a bug. Fixed.

Also fixed a one-off bug with quoted columns: in parseColumnRefs(),
first call to PQmblen(), I wrongly assumed that
PQmblen("", ..) returns 0, whereas in fact it returns 1.

> * A few examples in docs. The psql manpage should have at least two new
> examples showing the crosstab features, one with the simplest case you
> can think of, and another one showing all the features.

Added that in the EXAMPLES section at the very end of the manpage.

> * Add regression test cases somewhere for the regression database.
> Probably use "FROM tenk1 WHERE hundred < 5", which provides you with 500
> rows, enough for many interesting games. Make sure to test all the
> provided features. I would use a new psql.sql file for this.

Looking into regression tests, not yet done.

> * How did you come up with the 1600 value? Whatever it is, please use a
> #define instead of hardcoding it.

Done with accompanying comment in crosstabview.h

> * In the "if (cont.cells[idx] != NULL && cont.cells[idx][0] != '\0')"
> block (line 497 in the attached), can't we do the same thing by using
> psprintf?

In that block, we can't pass a cell contents as a valist and be done with
that cell, because duplicates of (col value,row value) may happen
at any iteration of the upper loop over PQntuples(results). Any cell really
may need reallocation unpredictably until that loop is done, whereas
psprintf starts by allocating a new buffer unconditionally, so it doesn't
to me like it could help to simplify that block.

Best regards,
Daniel Vérité
PostgreSQL-powered mailer:
Twitter: @DanielVerite

Attachment Content-Type Size
psql-crosstabview-v15.diff text/x-patch 40.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2016-04-07 15:40:51 Re: Speed up Clog Access by increasing CLOG buffers
Previous Message Robert Haas 2016-04-07 15:32:27 Re: Timeline following for logical slots