Re: Proposal: SELECT * EXCLUDE (...) command

Hi Hunaid,

Thanks for v3. I applied it to current master without conflicts, and
three of the four problems from my previous mail are fixed: no crash
with dropped columns, no leaking into later stars, and qualified names
now follow the normal alias and schema rules. The remaining one,
(t2).*, is your note 7; more on that below. Peter's examples from
BCN-036R1 behave as expected, except example 6, which is your note 5.

> 4. The columns in the REPLACE and RENAME clauses are marked
> for SELECT privilege checks, whereas the columns in the
> EXCLUDE clause are not. (needs discussion)

+1 from me. Excluding a column means "I don't want it", so requiring
a privilege on it seems odd. And in PostgreSQL column names are
visible in pg_attribute anyway, so EXCLUDE doesn't reveal anything
about columns you can't read. See also the RETURNING example below.

> 5. No ambiguity error is raised, and operation is applied to all matching
> columns in case of unqualified column names. (needs discussion)

+1 here too. In my previous mail I only listed this as an open
question, but having thought about it more, I prefer this behavior.
If the accepted version still makes this an error, as BCN-036R1 does,
this would only add behavior where the standard raises an error.
Every unambiguous name behaves the same, and a qualified name is
still available when only one of the columns should go.

I don't think the error buys much. Plain * isn't very robust against
schema changes to begin with: if someone adds a "note" column to b
while a already has one, SELECT * FROM a JOIN b silently starts
returning two "note" columns, which a client reading columns by name
can easily mix up. Anyone using * already lives with that, EXCLUDE or
not. So the error would only guard against a small corner of the
problem, while making Christoph's case from upthread more cumbersome:
with JOIN ... ON t1.id = t2.id, excluding id would need
EXCLUDE (t1.id, t2.id). Robert's USING (id) variant avoids that, but
only for the join column itself. Columns that many tables share, like
created_at and updated_at, are ambiguous in any join: with
orders JOIN users ON orders.user_id = users.id,
EXCLUDE (created_at, updated_at) would need four qualified names.

> 7. The case: select (t2).* exclude (bar)
> It comes as A_Indirection node, maybe we can reject these in gram.y
> in the check_indirection function. I have not touched it yet.

This is wider than (t2).*. Star options are accepted anywhere a .* can
appear, but they are only applied in a plain select list. Everywhere
else they are silently ignored:

select (t2).* (exclude (bar)) from t2; -- foo, bar, baz
select row(t2.* (exclude (bar))) from t2; -- (10,20,30)
select to_jsonb(t2.* (exclude (bar))) from t2; -- includes "bar"
select * from t2 where t2.* (exclude (bar)) is not null;
select foo from t2 order by t2.* (exclude (bar));

The to_jsonb() case worries me most: someone writing
to_jsonb(u.* (exclude (password))) gets no error, and the password
ends up in the output.

They all come from the same place: star_options is attached in
indirection_el, but only transformTargetList() looks at it. ROW() goes
through transformExpressionList(), which passes NULL, and inside an
expression t.* becomes a whole-row Var. I think all of these should
raise an error. For (t2).*, BCN-036R1 says the same: an <all fields
reference> doesn't support an exclude list. Supporting some of them,
like to_jsonb(), could be a nice follow-up later, but an error is the
safer first step.

> - RETURNING with star options is rejected, as it made no sense there.

I think it makes sense there, and it comes for free. RETURNING goes
through transformTargetList() just like a select list, and the docs
say that "the syntax of the RETURNING list is identical to that of the
output list of SELECT". Since RETURNING is a PostgreSQL extension
anyway, keeping it consistent with SELECT seems natural.

I removed the check from gram.y, and everything I tried just works,
for INSERT, UPDATE, DELETE and MERGE, including REPLACE, RENAME and
the new OLD/NEW:

UPDATE r SET bar = bar + 1 WHERE id = 2
RETURNING old.* (EXCLUDE (secret) RENAME (bar AS old_bar)),
new.* (EXCLUDE (id, foo, secret)
RENAME (bar AS new_bar));

Excluding every column hits the existing "RETURNING must have at
least one column" error, and in select_staroptions only the RETURNING
test changes. It is also a good example for your note 4: a role with
UPDATE but without SELECT on "secret" can run

UPDATE r SET bar = bar + 1 WHERE id = 1
RETURNING * (EXCLUDE (secret));

while plain RETURNING * fails with "permission denied". The check in
gram.y only looked at top-level stars anyway, so RETURNING
row(t2.* (exclude (bar))) got through.

If RETURNING is allowed, one thing needs fixing. old and new share the
same range table index, so this is accepted and excludes secret from
new.*:

UPDATE r SET bar = bar + 1 RETURNING new.* (EXCLUDE (old.secret));

Comparing the p_returning_type of the qualifier's nsitem with the
varreturningtype of the Var should catch it, as in example 10.

Sorry for mixing the threads a bit again, but this shows a nice use
of both features together. The combination with INSERT ... BY NAME [1]
still works with v3, and RENAME makes it even better: an application
type whose field names differ from the table's columns can be mapped
directly.

INSERT INTO post BY NAME
SELECT * (EXCLUDE (id) RENAME (heading AS title, content AS body))
FROM unnest($1::post_dto[]);

Minor, since you mentioned refactoring: a few helpers would remove
most of the repetition, e.g. a makeStarOptions() in gram.y, one shared
rule for the column in exclude_item/replace_item/rename_item, and one
function for the three loops in check_star_options().

The new staroptions argument of ExpandColumnRefStar() is redundant,
because the options are already in the A_Star at the end of
cref->fields. transformExpressionList() passes NULL there even though
cref has options, which is the ROW() case above. If
ExpandColumnRefStar() read them from cref itself, that case could be
caught in one place.

Finally, when no column matches, check_star_options() reports a
missing column without checking the qualifier. For EXCLUDE
(nosuch.zzz) it reports

ERROR: column nosuch.zzz does not exist

where a select list reports

ERROR: missing FROM-clause entry for table "nosuch"

Do you plan to add it to the commitfest? PG20-3 is open for new
entries until the end of October, and cfbot would then also test
every new version on several platforms.

[1] https://www.postgresql.org/message-id/flat/CAJTYsWVDs7vkEN-eD1NAnZ7XqcfpXu3tP1wMGzJQ12G6eo1oRw%40mail.gmail.com

Regards,
Kacper Kuras