On Fri, Mar 25, 2022 at 3:42 PM Mark Dilger <mark.dil...@enterprisedb.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
