| From: | Tatsuro Yamada <yamatattsu(at)gmail(dot)com> |
|---|---|
| To: | Jim Jones <jim(dot)jones(at)uni-muenster(dot)de> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Álvaro Herrera <alvherre(at)kurilemu(dot)de> |
| Subject: | Re: [PATCH] psql: add \dcs to list all constraints |
| Date: | 2026-01-15 01:48:43 |
| Message-ID: | CAOKkKFv3FvrqABum-4vfDcdj_8TodOXP6eraAKkh2wGjCbGnbw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Jim,
Thanks for your review comments!
On Tue, Jan 13, 2026 at 12:17 AM Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>
wrote:
> Here a few comments to v2:
>
== listConstraints() ==
It looks like that a WHERE condition can be potentially added to the "if
> (!showAllkinds)" block even if there is no WHERE clause at all. I'm not
> sure if this path is even possible, but perhaps a more defensive
> approach here wouldn't be a bad idea, e.g.
>
> ...
> bool have_where = false;
>
> if (!showSystem && !pattern)
> {
> appendPQExpBufferStr(&buf,
> "WHERE n.nspname <> 'pg_catalog' \n"
> " AND n.nspname <> 'information_schema' \n");
> have_where = true;
> }
>
> if (!validateSQLNamePattern(&buf, pattern,
> have_where, false,
> "n.nspname", "cst.conname", NULL,
> "pg_catalog.pg_table_is_visible(cst.conrelid)",
> &have_where, 3))
> {
>
> if (!showAllkinds)
> {
> appendPQExpBuffer(&buf, " %s cst.contype in (",
> have_where ? "AND" : "WHERE");
> ...
>
> What do you think?
>
Based on the value passed to validateSQLNamePattern(), one or more of
n.nspname, cst.conname, or pg_catalog.pg_table_is_visible(cst.conrelid) is
added to the query string together with either WHERE or AND.
Therefore, I believe there is no case in which the if (!showAllkinds) block
is
reached without an existing WHERE clause.
If you are aware of a specific test case where this can happen, I would
appreciate it if you could share it with me.
For now, my conclusion is that I would like to keep this part as it is. I
apologize
if I have missed something.
== Patch name ==
>
> It'd be better if you format your patch name with the version upfront, e.g.
>
> $ git format-patch -1 -v3
>
Thank you for the suggestion. From now on, I will generate patches using
the options you mentioned.
> I've tried a few more edge cases and so far everything is working as
expected
>
> postgres=# \set ECHO_HIDDEN on
>
> postgres=# \dcs 🐘*
> /******** QUERY *********/
> ...
FROM pg_catalog.pg_constraint cst
> JOIN pg_catalog.pg_namespace n ON n.oid = cst.connamespace
> JOIN pg_catalog.pg_class c on c.oid = cst.conrelid
>
Thank you for testing these additional edge cases.
I noticed that when the "+" (verbose option) is not used, the table name is
not needed. In that case, joining the pg_class table is unnecessary.
This will be fixed in the next patch.
Thanks,
Tatsuro Yamada
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-01-15 01:57:59 | Re: Can we change pg_rewind used without wal_log_hints and data_checksums |
| Previous Message | Michael Paquier | 2026-01-15 01:48:06 | Re: Add IS_INDEX macro to brin and gist index |