Re: ruleutils vs. empty targetlists

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane escribi�:
>> What I'm thinking about this today is that really the *right* solution
>> is to allow syntactically-empty SELECT lists; once we've bought into the
>> notion of zero-column tables, the notion that you can't have an empty
>> select list is just fundamentally at odds with that.  And since you can
>> already have semantically-empty SELECT lists, this should in theory not
>> create much risk of new bugs.  If we did that, the existing ruleutils
>> code is just fine, as are any existing dump files containing this sort
>> of query.

> Wow, as strange-sounding as that is, you're probably correct.

I experimented with this a bit, and found that things seem to pretty much
work, although unsurprisingly there are a couple of regression tests that
expected to get a syntax error and no longer do.

The only thing I've come across that arguably doesn't work is SELECT
DISTINCT:

regression=# select distinct from pg_database;
--
(8 rows)

The reason this says "8 rows" is that the produced plan is just a seqscan
of pg_database returning no columns.  The parser produced a Query with an
empty distinctClause, so the planner thinks no unique-ification is
required, so it doesn't produce any sort/uniq or hashagg node.  This seems
a bit bogus to me; mathematically, it seems like what this ought to mean
is that all rows are equivalent and so you get just one empty row out.
However, I don't see any practical use-case for that behavior, so I'm
disinclined to spend time on inventing a solution --- and any solution
would probably require a change in Query representation, making it not
back-patchable anyway.  Instead I suggest we add an error check in parse
analysis that throws an error for DISTINCT with no columns.  If anyone is
ever motivated to make this case do something sane, they can remove that
check and fix up whatever needs fixing up.  The attached first-draft patch
doesn't yet have such a prohibition, though.

Another point is that there is a second use of target_list in the grammar,
which is "RETURNING target_list".  I did not change that one into
"RETURNING opt_target_list", because if I had, it would have an issue
similar to DISTINCT's: RETURNING with no arguments would be represented
the same as no RETURNING at all, leading to probably not the behavior
the user expected.  We've already got that problem if you say "RETURNING
zero_column_table.*", making me wonder if a parse-analysis-time error
for that case wouldn't be a good thing.

Comments?

            regards, tom lane

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 8fced44..f9d4577 100644
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
*************** static Node *makeRecursiveViewSelect(cha
*** 334,340 ****
                  name_list from_clause from_list opt_array_bounds
                  qualified_name_list any_name any_name_list
                  any_operator expr_list attrs
!                 target_list insert_column_list set_target_list
                  set_clause_list set_clause multiple_set_clause
                  ctext_expr_list ctext_row def_list indirection opt_indirection
                  reloption_list group_clause TriggerFuncArgs select_limit
--- 334,340 ----
                  name_list from_clause from_list opt_array_bounds
                  qualified_name_list any_name any_name_list
                  any_operator expr_list attrs
!                 target_list opt_target_list insert_column_list set_target_list
                  set_clause_list set_clause multiple_set_clause
                  ctext_expr_list ctext_row def_list indirection opt_indirection
                  reloption_list group_clause TriggerFuncArgs select_limit
*************** select_clause:
*** 9259,9265 ****
   * However, this is not checked by the grammar; parse analysis must check it.
   */
  simple_select:
!             SELECT opt_distinct target_list
              into_clause from_clause where_clause
              group_clause having_clause window_clause
                  {
--- 9259,9265 ----
   * However, this is not checked by the grammar; parse analysis must check it.
   */
  simple_select:
!             SELECT opt_distinct opt_target_list
              into_clause from_clause where_clause
              group_clause having_clause window_clause
                  {
*************** ctext_row: '(' ctext_expr_list ')'                    {
*** 12215,12220 ****
--- 12215,12224 ----
   *
   *****************************************************************************/

+ opt_target_list: target_list                        { $$ = $1; }
+             | /* EMPTY */                            { $$ = NIL; }
+         ;
+
  target_list:
              target_el                                { $$ = list_make1($1); }
              | target_list ',' target_el                { $$ = lappend($1, $3); }
diff --git a/src/test/regress/expected/errors.out b/src/test/regress/expected/errors.out
index 4061512..b63cb54 100644
*** a/src/test/regress/expected/errors.out
--- b/src/test/regress/expected/errors.out
*************** select 1;
*** 15,35 ****
  --
  --
  -- SELECT
! -- missing relation name
  select;
! ERROR:  syntax error at or near ";"
! LINE 1: select;
!               ^
  -- no such relation
  select * from nonesuch;
  ERROR:  relation "nonesuch" does not exist
  LINE 1: select * from nonesuch;
                        ^
- -- missing target list
- select from pg_database;
- ERROR:  syntax error at or near "from"
- LINE 1: select from pg_database;
-                ^
  -- bad name in target list
  select nonesuch from pg_database;
  ERROR:  column "nonesuch" does not exist
--- 15,30 ----
  --
  --
  -- SELECT
! -- this used to be a syntax error, but now we allow an empty target list
  select;
! --
! (1 row)
!
  -- no such relation
  select * from nonesuch;
  ERROR:  relation "nonesuch" does not exist
  LINE 1: select * from nonesuch;
                        ^
  -- bad name in target list
  select nonesuch from pg_database;
  ERROR:  column "nonesuch" does not exist
*************** select * from pg_database where pg_datab
*** 45,56 ****
  ERROR:  column "nonesuch" does not exist
  LINE 1: ...ect * from pg_database where pg_database.datname = nonesuch;
                                                                ^
! -- bad select distinct on syntax, distinct attribute missing
! select distinct on (foobar) from pg_database;
! ERROR:  syntax error at or near "from"
! LINE 1: select distinct on (foobar) from pg_database;
!                                     ^
! -- bad select distinct on syntax, distinct attribute not in target list
  select distinct on (foobar) * from pg_database;
  ERROR:  column "foobar" does not exist
  LINE 1: select distinct on (foobar) * from pg_database;
--- 40,46 ----
  ERROR:  column "nonesuch" does not exist
  LINE 1: ...ect * from pg_database where pg_database.datname = nonesuch;
                                                                ^
! -- bad attribute name in select distinct on
  select distinct on (foobar) * from pg_database;
  ERROR:  column "foobar" does not exist
  LINE 1: select distinct on (foobar) * from pg_database;
diff --git a/src/test/regress/sql/errors.sql b/src/test/regress/sql/errors.sql
index 2ee707c..80d1b62 100644
*** a/src/test/regress/sql/errors.sql
--- b/src/test/regress/sql/errors.sql
*************** select 1;
*** 16,29 ****
  --
  -- SELECT

! -- missing relation name
  select;

  -- no such relation
  select * from nonesuch;

- -- missing target list
- select from pg_database;
  -- bad name in target list
  select nonesuch from pg_database;
  -- bad attribute name on lhs of operator
--- 16,27 ----
  --
  -- SELECT

! -- this used to be a syntax error, but now we allow an empty target list
  select;

  -- no such relation
  select * from nonesuch;

  -- bad name in target list
  select nonesuch from pg_database;
  -- bad attribute name on lhs of operator
*************** select * from pg_database where nonesuch
*** 32,43 ****
  -- bad attribute name on rhs of operator
  select * from pg_database where pg_database.datname = nonesuch;

!
! -- bad select distinct on syntax, distinct attribute missing
! select distinct on (foobar) from pg_database;
!
!
! -- bad select distinct on syntax, distinct attribute not in target list
  select distinct on (foobar) * from pg_database;


--- 30,36 ----
  -- bad attribute name on rhs of operator
  select * from pg_database where pg_database.datname = nonesuch;

! -- bad attribute name in select distinct on
  select distinct on (foobar) * from pg_database;