Re: pg14 psql broke \d datname.nspname.relname

On Fri, Mar 25, 2022 at 3:42 PM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> I think your change is fine, so I've rolled it into this next patch.

OK, cool. Here are some more comments.

In describe.c, why are the various describeWhatever functions
returning true when validateSQLNamePattern returns false? It seems to
me that they should return false. That would cause exec_command_d() to
set status = PSQL_CMD_ERROR, which seems appropriate. I wondered
whether we should return PSQL_CMD_ERROR only for database errors, but
that doesn't seem to be the case. For example, exec_command_a() sets
PSQL_CMD_ERROR for a failure in do_pset().

pg_dump's prohibit_crossdb_refs() has a special case for you are not
connected to a database, but psql's validateSQLNamePattern() treats it
as an invalid cross-database reference. Maybe that should be
consistent, or just the other way around. After all, I would expect
pg_dump to just bail out if we lose the database connection, but psql
may continue, because we can reconnect. Putting more code into the
tool where reconnecting doesn't really make sense seems odd.

processSQLNamePattern() documents that dotcnt can be NULL, and then
asserts that it isn't.

processSQLNamePattern() introduces new local variables schema and
name, which account for most of the notational churn in that function.
I can't see a reason why those changes are needed. You do test whether
the new variables are NULL in a couple of places, but you could
equally well test schemavar/namevar/altnamevar directly. Actually, I
don't really understand why this function needs any changes other than
passing dbnamebuf and dotcnt through to patternToSQLRegex(). Is there
a reason?

patternToSQLRegex() restructures the system of buffers as well, and I
don't understand the purpose of that either. It sort of looks like the
idea might be to relax the rule against dbname.relname patterns, but
why would we want to do that? If we don't want to do that, why remove
the assertion?

It is not very nice that patternToSQLRegex() ends up repeating the
locution "if (left && want_literal_dbname)
appendPQExpBufferChar(&left_literal, '"')" a whole bunch of times.
Suppose we remove all that. Then, in the if (!inquotes && ch == '.')
case, if left = true, we copy "cp - pattern" bytes starting at
"pattern" into the buffer. Wouldn't that accomplish the same thing
with less code?

--
Robert Haas
EDB: http://www.enterprisedb.com