From: | Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Peter Geoghegan <pg(at)bowt(dot)ie>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: pg14 psql broke \d datname.nspname.relname |
Date: | 2022-04-06 16:07:15 |
Message-ID: | ACE7E072-7AF9-4DE8-BE06-5462F32E629D@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> 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.
Attachment | Content-Type | Size |
---|---|---|
v8-0001-Reject-patterns-with-too-many-parts-or-wrong-db.patch | application/octet-stream | 76.5 KB |
unknown_filename | text/plain | 97 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Justin Pryzby | 2022-04-06 16:16:44 | Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) |
Previous Message | Tom Lane | 2022-04-06 16:04:35 | Re: SQL/JSON: JSON_TABLE |