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

> On Mar 29, 2022, at 8:20 AM, Robert Haas <robertmhaas(at)gmail(dot)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.