Thread
Commits
-
Make crosstabview honor boolean/null display settings
- 4df5fe3833a8 19 (unreleased) landed
-
Fix \crosstabview to honor \pset display_true/display_false
Chao Li <li.evan.chao@gmail.com> — 2026-06-03T07:16:13Z
Hi, While testing “[645cb44c5] Add \pset options for boolean value display”, I noticed that it seems to miss support for \crosstabview. Here is a repro: ``` evantest=# \pset display_true YES Boolean true display is "YES". evantest=# \pset display_false NO Boolean false display is "NO". evantest=# evantest=# select true as direct_true, false as direct_false; direct_true | direct_false -------------+-------------- YES | NO (1 row) evantest=# evantest=# select false as row_key, true as col_key, true as val evantest-# union all evantest-# select true, false, false evantest-# \crosstabview row_key col_key val row_key | t | f ---------+---+--- f | t | t | | f (2 rows) ``` In normal query output, true/false are shown as YES/NO, but \crosstabview still shows them as t/f. I noticed this problem by searching for nullPrint and finding it in crosstabview.c. Since nullPrint is already honored there, display_true/display_false should be honored as well. While working on the fix, I further noticed that numericlocale is also not honored by \crosstabview: ``` evantest=# \pset numericlocale on evantest=# evantest=# SELECT 12345::numeric AS direct_value; direct_value -------------- 12,345 (1 row) evantest=# evantest=# SELECT 1 AS row_key, 1 AS col_key, 12345::numeric AS val evantest-# \crosstabview row_key col_key val row_key | 1 ---------+------- 1 | 12345 (1 row) ``` Since we are supposed to fix only v19 bugs at this stage, this patch only makes \crosstabview honor display_true/display_false, as that is a new PG19 feature. I will add the numericlocale work to my TODO and revisit it for v20. In this patch, I add a helper function, printQueryOptDisplayValue(), to handle both nullPrint and display_true/display_false in a single place. In the future, we may also be able to handle numericlocale in this function. See the attached patch for details. With the fix, \crosstabview displays boolean values according to the \pset settings: ``` evantest=# select false as row_key, true as col_key, true as val evantest-# union all evantest-# select true, false, false evantest-# \crosstabview row_key col_key val row_key | YES | NO ---------+-----+---- NO | YES | YES | | NO (2 rows) ``` Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix \crosstabview to honor \pset display_true/display_false
David G. Johnston <david.g.johnston@gmail.com> — 2026-06-18T22:47:43Z
On Wed, Jun 3, 2026 at 12:16 AM Chao Li <li.evan.chao@gmail.com> wrote: > While testing “[645cb44c5] Add \pset options for boolean value display”, I > noticed that it seems to miss support for \crosstabview. > > In this patch, I add a helper function, printQueryOptDisplayValue(), to > handle both nullPrint and display_true/display_false in a single place. In > the future, we may also be able to handle numericlocale in this function. > > Thanks Chao! Adding a function indeed seems like the best choice - the only question is whether expanding print.h at this point is acceptable. If not, making it private to crosstab.c will accomplish the same immediate need. The name is a bit odd to me but I'm also not that familiar with naming conventions in the C code. But this is another reason to make it private in v19 and figure out the public interface in v20. > cont.cells[idx] = > printQueryOptDisplayValue(!PQgetisnull(result, rn, field_for_data) ? > PQgetvalue(result, rn, field_for_data) : > NULL, > data_ftype, &popt, ""); I'd either remove the not operator and flip the order of the output ternary positions, or move the entire "compute the unadjusted value" to a local variable (assignment from ternary or just an if block) and then pass it to printQueryOptDisplayValue directly. Having a ternary expression as the first argument is what the other changes got rid of and makes this much easier to read (hence the need for the function). David J.
-
Re: Fix \crosstabview to honor \pset display_true/display_false
Chao Li <li.evan.chao@gmail.com> — 2026-06-19T01:35:34Z
> On Jun 19, 2026, at 06:47, David G. Johnston <david.g.johnston@gmail.com> wrote: > > On Wed, Jun 3, 2026 at 12:16 AM Chao Li <li.evan.chao@gmail.com> wrote: > While testing “[645cb44c5] Add \pset options for boolean value display”, I noticed that it seems to miss support for \crosstabview. > > In this patch, I add a helper function, printQueryOptDisplayValue(), to handle both nullPrint and display_true/display_false in a single place. In the future, we may also be able to handle numericlocale in this function. > > > Thanks Chao! > > Adding a function indeed seems like the best choice - the only question is whether expanding print.h at this point is acceptable. If not, making it private to crosstab.c will accomplish the same immediate need. > > The name is a bit odd to me but I'm also not that familiar with naming conventions in the C code. But this is another reason to make it private in v19 and figure out the public interface in v20. Sounds fair. I removed the helper declaration from print.h and added local temporary declaration in the .c files with a TODO comment for v20. > > > cont.cells[idx] = > > printQueryOptDisplayValue(!PQgetisnull(result, rn, field_for_data) ? > > PQgetvalue(result, rn, field_for_data) : > > NULL, > > data_ftype, &popt, ""); > > I'd either remove the not operator and flip the order of the output ternary positions, or move the entire "compute the unadjusted value" to a local variable (assignment from ternary or just an if block) and then pass it to printQueryOptDisplayValue directly. Having a ternary expression as the first argument is what the other changes got rid of and makes this much easier to read (hence the need for the function). Agreed. PFA v2: addressed David’s comments. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix \crosstabview to honor \pset display_true/display_false
David G. Johnston <david.g.johnston@gmail.com> — 2026-06-19T15:51:22Z
On Thu, Jun 18, 2026 at 6:36 PM Chao Li <li.evan.chao@gmail.com> wrote: > PFA v2: addressed David’s comments. > Looked a bit closer at printQuery this time; disliking that the if condition has an awareness of the implementation of printQueryOptDisplayValue since it lists !isnull and ftype != BOOLOID explicitly. How about not touching print.c at all with this patch, moving the printQueryOptDisplayValue into crosstabview.c and leaving printQuery alone since it is already correct? David J. P.S. When we do get to cleaning this up it seems annoying/odd that we determine whether to apply numeric locale based upon whether the display alignment for the column is right-aligned or not as opposed to the data type of the value.
-
Re: Fix \crosstabview to honor \pset display_true/display_false
Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-19T17:24:31Z
On 2026-Jun-19, David G. Johnston wrote: > Looked a bit closer at printQuery this time; disliking that the if > condition has an awareness of the implementation of > printQueryOptDisplayValue since it lists !isnull and ftype != BOOLOID > explicitly. > > How about not touching print.c at all with this patch, moving the > printQueryOptDisplayValue into crosstabview.c and leaving printQuery alone > since it is already correct? That was my reaction also, so I produced this POC patch. I think print.h needs some more work, and that'd be only for 20. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
-
Re: Fix \crosstabview to honor \pset display_true/display_false
David G. Johnston <david.g.johnston@gmail.com> — 2026-06-19T20:02:08Z
On Fri, Jun 19, 2026 at 10:24 AM Álvaro Herrera <alvherre@kurilemu.de> wrote: > On 2026-Jun-19, David G. Johnston wrote: > > > Looked a bit closer at printQuery this time; disliking that the if > > condition has an awareness of the implementation of > > printQueryOptDisplayValue since it lists !isnull and ftype != BOOLOID > > explicitly. > > > > How about not touching print.c at all with this patch, moving the > > printQueryOptDisplayValue into crosstabview.c and leaving printQuery > alone > > since it is already correct? > > That was my reaction also, so I produced this POC patch. I think > print.h needs some more work, and that'd be only for 20. > > Chao's decision to include the row and column headers makes more sense to me. The promise of this feature is that the visual output of booleans as t/f should go away if the user chooses to make use of these local variables. While we may not be able get them all (e.g., composites and arrays) doing so here is trivial to accomplish. David J.
-
Re: Fix \crosstabview to honor \pset display_true/display_false
Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-20T10:32:20Z
On 2026-Jun-19, David G. Johnston wrote: > Chao's decision to include the row and column headers makes more sense to > me. The promise of this feature is that the visual output of booleans as > t/f should go away if the user chooses to make use of these > local variables. While we may not be able get them all (e.g., > composites and arrays) doing so here is trivial to accomplish. OK. TBH that part seemed a bit over the top to me, but that wasn't a very strongly held opinion. I'll put that back and push soonish. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La gente vulgar sólo piensa en pasar el tiempo; el que tiene talento, en aprovecharlo"
-
Re: Fix \crosstabview to honor \pset display_true/display_false
Chao Li <li.evan.chao@gmail.com> — 2026-06-21T00:37:53Z
> On Jun 20, 2026, at 18:32, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > On 2026-Jun-19, David G. Johnston wrote: > >> Chao's decision to include the row and column headers makes more sense to >> me. The promise of this feature is that the visual output of booleans as >> t/f should go away if the user chooses to make use of these >> local variables. While we may not be able get them all (e.g., >> composites and arrays) doing so here is trivial to accomplish. > > OK. TBH that part seemed a bit over the top to me, but that wasn't a > very strongly held opinion. I'll put that back and push soonish. > > -- > Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ > "La gente vulgar sólo piensa en pasar el tiempo; > el que tiene talento, en aprovecharlo" Thank you both very much for reviewing and for the valuable comments. I have made the helper private to crosstabview.c and left print.c unchanged in v3. I will see if I can improve print.h/c for v20. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix \crosstabview to honor \pset display_true/display_false
Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-25T13:21:13Z
On 2026-Jun-21, Chao Li wrote: > Thank you both very much for reviewing and for the valuable comments. > > I have made the helper private to crosstabview.c and left print.c > unchanged in v3. I will see if I can improve print.h/c for v20. I mostly agree with this, but I don't think we need to make that function name that long. It's a private function after all. But it's definitely in the wrong spot in the file, so I propose to move it down. Now, your new test case doesn't exercise "\pset null" under \crosstabview, and I'm not sure that just leaving the cell empty for a NULL is really the thing to do, when \pset null has set it to some other value. Simply printing the \pset null string doesn't seem to cut it though (here implemented as the change in printCrosstab line 421ff), as seen in how this patch affects the test case prior to the new one. Should we have a separate array of bools to distinguish cells that must remain empty (because the query didn't provide a value for them) from cells that must have the \pset null value? Thanks diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c index aaa9b8a2451..75d5563b0b1 100644 --- a/src/bin/psql/crosstabview.c +++ b/src/bin/psql/crosstabview.c @@ -47,28 +47,6 @@ typedef struct _pivot_field int rank; } pivot_field; -/* - * Return the display representation of a crosstab value, following pset - * substitutions. The returned pointer should not be freed. - * - * TODO: In v20, consider whether this should become part of the public print.h - * interface, and with what name. - */ -static char * -printQueryOptDisplayValue(char *value, Oid ftype, const printQueryOpt *opt, - const char *default_null) -{ - if (value == NULL) - return opt->nullPrint ? opt->nullPrint : - unconstify(char *, default_null); - - if (ftype == BOOLOID) - return value[0] == 't' ? - (opt->truePrint ? opt->truePrint : "t") : - (opt->falsePrint ? opt->falsePrint : "f"); - - return value; -} /* Node in avl_tree */ typedef struct _avl_node @@ -106,6 +84,8 @@ static bool printCrosstab(const PGresult *result, int num_columns, pivot_field *piv_columns, int field_for_columns, int num_rows, pivot_field *piv_rows, int field_for_rows, int field_for_data); +static char *printDisplayValue(const printQueryOpt *opt, Oid ftype, char *value, + char *default_null); static void avlInit(avl_tree *tree); static void avlMergeValue(avl_tree *tree, char *name, char *sort_value); static int avlCollectFields(avl_tree *tree, avl_node *node, @@ -349,8 +329,8 @@ printCrosstab(const PGresult *result, { char *colname; - colname = printQueryOptDisplayValue(piv_columns[horiz_map[i]].name, - col_ftype, &popt, ""); + colname = printDisplayValue(&popt, col_ftype, + piv_columns[horiz_map[i]].name, ""); printTableAddHeader(&cont, colname, false, col_align); } @@ -363,8 +343,7 @@ printCrosstab(const PGresult *result, int idx = k * (num_columns + 1); cont.cells[idx] = - printQueryOptDisplayValue(piv_rows[i].name, row_ftype, &popt, - ""); + printDisplayValue(&popt, row_ftype, piv_rows[i].name, ""); } cont.cellsadded = num_rows * (num_columns + 1); @@ -421,17 +400,17 @@ printCrosstab(const PGresult *result, if (cont.cells[idx] != NULL) { pg_log_error("\\crosstabview: query result contains multiple data values for row \"%s\", column \"%s\"", - printQueryOptDisplayValue(rp->name, row_ftype, - &popt, "(null)"), - printQueryOptDisplayValue(cp->name, col_ftype, - &popt, "(null)")); + printDisplayValue(&popt, row_ftype, rp->name, + "(null)"), + printDisplayValue(&popt, col_ftype, cp->name, + "(null)")); goto error; } if (!PQgetisnull(result, rn, field_for_data)) value = PQgetvalue(result, rn, field_for_data); cont.cells[idx] = - printQueryOptDisplayValue(value, data_ftype, &popt, ""); + printDisplayValue(&popt, data_ftype, value, "ups"); } } @@ -442,7 +421,7 @@ printCrosstab(const PGresult *result, for (i = 0; i < cont.cellsadded; i++) { if (cont.cells[i] == NULL) - cont.cells[i] = ""; + cont.cells[i] = printDisplayValue(&popt, InvalidOid, NULL, ""); } printTable(&cont, pset.queryFout, false, pset.logfile); @@ -454,6 +433,27 @@ error: return retval; } +/* + * Return the display representation of one value in the query result, + * following pset substitutions. + * + * The returned pointer is not a copy. + */ +static char * +printDisplayValue(const printQueryOpt *opt, Oid ftype, char *value, + char *default_null) +{ + if (value == NULL) + return opt->nullPrint ? opt->nullPrint : default_null; + + if (ftype == BOOLOID) + return value[0] == 't' ? + (opt->truePrint ? opt->truePrint : "t") : + (opt->falsePrint ? opt->falsePrint : "f"); + + return value; +} + /* * The avl* functions below provide a minimalistic implementation of AVL binary * trees, to efficiently collect the distinct values that will form the horizontal diff --git a/src/test/regress/sql/psql_crosstab.sql b/src/test/regress/sql/psql_crosstab.sql index 00b395d7168..b9f8007b2d6 100644 --- a/src/test/regress/sql/psql_crosstab.sql +++ b/src/test/regress/sql/psql_crosstab.sql @@ -70,12 +70,21 @@ GROUP BY v, h ORDER BY h,v \pset null '' -- boolean display -\pset display_true 'true' -\pset display_false 'false' -SELECT false as row_key, true as col_key, true as val +\pset display_true 'Aye' +\pset display_false 'Nay' +\pset null 'Wut' +SELECT false as display_bools, false as col_key, false as val UNION ALL -SELECT true, false, false - \crosstabview row_key col_key val +SELECT true, true, true +UNION ALL +SELECT null, true, false +UNION ALL +SELECT null, false, true +UNION ALL +SELECT true, null, false +UNION ALL +SELECT false, null, true + \crosstabview display_bools col_key val \pset display_true 't' \pset display_false 'f' -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ Tom: There seems to be something broken here. Teodor: I'm in sackcloth and ashes... Fixed. http://postgr.es/m/482D1632.8010507@sigaev.ru -
Re: Fix \crosstabview to honor \pset display_true/display_false
Chao Li <li.evan.chao@gmail.com> — 2026-06-26T00:01:12Z
> On Jun 25, 2026, at 21:21, Álvaro Herrera <alvherre@kurilemu.de> wrote: Thank you very much for the comments. > > On 2026-Jun-21, Chao Li wrote: > >> Thank you both very much for reviewing and for the valuable comments. >> >> I have made the helper private to crosstabview.c and left print.c >> unchanged in v3. I will see if I can improve print.h/c for v20. > > I mostly agree with this, but I don't think we need to make that > function name that long. It's a private function after all. But it's > definitely in the wrong spot in the file, so I propose to move it down. Agreed. > > Now, your new test case doesn't exercise "\pset null" under > \crosstabview, and I'm not sure that just leaving the cell empty for a > NULL is really the thing to do, when \pset null has set it to some other > value. Simply printing the \pset null string doesn't seem to cut it > though (here implemented as the change in printCrosstab line 421ff), as > seen in how this patch affects the test case prior to the new one. > Should we have a separate array of bools to distinguish cells that must > remain empty (because the query didn't provide a value for them) from > cells that must have the \pset null value? > Yes, I didn’t include \pset null in the new test case because I didn’t intend this patch to change NULL display behavior, so I assumed other test cases already covered that. I didn’t check that carefully enough. I didn’t take this part from your proposal, so an empty cell still shows nothing: ``` @@ -442,7 +421,7 @@ printCrosstab(const PGresult *result, for (i = 0; i < cont.cellsadded; i++) { if (cont.cells[i] == NULL) - cont.cells[i] = ""; + cont.cells[i] = printDisplayValue(&popt, InvalidOid, NULL, ""); } ``` Your proposed test has NULL in the row and column headers, but not in the value column. I added a NULL value too, so the test case explicitly demonstrates the display difference between a NULL value and an empty cell. PFA v4: addressed Álvaro's comments. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix \crosstabview to honor \pset display_true/display_false
Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-26T18:07:35Z
Hello, On 2026-Jun-26, Chao Li wrote: > I didn’t take this part from your proposal, so an empty cell still shows nothing: > ``` > @@ -442,7 +421,7 @@ printCrosstab(const PGresult *result, > for (i = 0; i < cont.cellsadded; i++) > { > if (cont.cells[i] == NULL) > - cont.cells[i] = ""; > + cont.cells[i] = printDisplayValue(&popt, InvalidOid, NULL, ""); > } > ``` Yeah, this particular hunk wasn't intended for commit, so leaving it out was the right thing to do. > Your proposed test has NULL in the row and column headers, but not in > the value column. I added a NULL value too, so the test case > explicitly demonstrates the display difference between a NULL value > and an empty cell. Yeah, that's perfect, thanks. > PFA v4: addressed Álvaro's comments. Pushed now, thanks. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/