> On Mar 29, 2022, at 8:20 AM, Robert Haas <robertmh...@gmail.com> wrote: > > 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().
Yes, I believe you are right. For scripting, the following should echo, but doesn't under the version 7 patch. Fixed in version 8. % psql -c "\d a.b.c.d" || echo 'error' improper qualified name (too many dotted names): a.b.c.d > 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. Fixed psql in version 8 to issue the appropriate error message, either "You are currently not connected to a database." or "cross-database references are not implemented: %s". That matches the output for pg_dump. > processSQLNamePattern() documents that dotcnt can be NULL, and then > asserts that it isn't. That's ugly. Fixed the documentation in version 8. > 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? It looks like overeager optimization to me, to avoid passing buffers to patternToSQLRegex that aren't really wanted, consequently asking that function to parse things that the caller doesn't care about. But I don't think the optimization is worth the git history churn. Removed in version 8. > 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? This took a while to answer. I don't remember exactly what I was trying to do here, but it looks like I wanted callers who only want a (possibly database-qualified) schema name to pass that in the (dbnamebuf and) schemabuf, rather than using the (schemabuf and ) namebuf. I obviously didn't finish that conversion, because the clients never got the message. What remained was some rearrangement in patternToSQLRegex which worked but served no purpose. I've reverted the useless refactoring. > 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? We don't *quite* want the literal left string. If it is quoted, we still want the quotes removed. For example: \d "robert.haas".accounts.acme needs to return robert.haas (without the quotes) as the database name. Likewise, for embedded quotes: \d "robert""haas".accounts.acme needs to return robert"haas, and so forth. I was able to clean up the "if (left && want_literal_dbname)" stuff, though.
v8-0001-Reject-patterns-with-too-many-parts-or-wrong-db.patch
Description: Binary data
— Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
