Re: [PATCH] psql: add \dcs to list all constraints

Hi Chao-san,

Thanks for your comments!

On Thu, Jan 15, 2026 at 4:20 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>Thanks for the patch. I just reviewed the patch, and I think it is solid
already,
>just got a few small comments:
>
>1 - commit message
>```
>\dCN shows all kind of constraints by using pg_constraint.
>```
>
>All kind -> all kinds

Oh, good catch!
I will fix it. Thanks!

>2.
>```
>+ switch (cmd[3])
>+ {
>+ case '\0':
> ...
>+ success =
listConstraints(&cmd[3], pattern, show_verbose, show_system);
>+ break;
>+ default:
>+ status =
PSQL_CMD_UNKNOWN;
>+ break;
>```
>and
>```
>+ if (strlen(contypes) != strspn(contypes, dCN_options))
>+ {
>+ pg_log_error("\\dCN only takes [%s] as options",
dCN_options);
>+ return true;
>+ }
>```
>
>These two checks are redundant. Is it intentional to keep both?
>It might be simpler to rely solely on listConstraints() for validation.

The first check only verifies the next character following "CN".
For example, the following string would be passed to the listConstraints()
function:

\dCNcz

However, "z" is a non-existent option, so an error should be thrown.
This is why the second check is implemented: it reports an error if
the string contains anything other than the characters defined in
dCN_options.

This test case corresponds to the following regression test:
```
+---- Incorrect options will result in an error
+\dCNcz
+\dCN only takes [cfnptueSx+] as options
```

These two checks are also used in the existing \df, and \dCN follows
the same approach.

BTW, I just realized that this error message should be adjusted to align
with \? and the documentation. I will fix it later:
```
+\dCN only takes [cfnptue][Sx+] as options
```

>3
>```
>---- \dCN doesn't show constraints related to domain,
>---- since \dD can be used to check them
>```
>
>I saw this in the test script, should we mention that in the doc change?

Tom also commented on constraints related to domains, and I would like to
discuss and decide whether to include them in the \dCN command.

Depending on the decision, I am considering the following changes:

- If they are included:
- Remove the above test case from the regression tests
- If they are not included:
- Add the reason for excluding them to the documentation

Which do you think is better? Should domain constraints be covered by \dCN?
I would appreciate your feedback.

>4
>```
>+ <term><literal>\dCN[cfnptue][Sx+] [ <link
linkend="app-psql-patterns"><replaceable
class="parameter">pattern</replaceable></link> ]</literal></term>
>
>```
>
>Would it make sense to add a white space between [cfnptue] and [Sx+]? As
you do so for the help message.

Thanks for pointing that out.
I realized that this is an error in the help message, not the documentation.

$ psql -c '\?'|grep dCN
\dCN[cfnptue] [Sx+] [PATTERN]

I believe this syntax means that a space is required between the add-on
filter
[cfnptue] and [Sx+]. However, adding a space does not result in the
intended
behavior. Therefore, I will remove the space from the help message.

For example,
If there is a space before S, the pattern string will not be recognized.
# \dCNn S pg_const*
List of constraints
Schema | Name
--------+------
(0 rows)

If there is no space, the pattern string will be recognized.
# \dCNnS pg_const*
List of constraints
Schema | Name
------------+----------------------------
pg_catalog | pg_constraint_oid_not_null
(1 row)

>5
>```
>...
>+ pg_log_error("\\dCN only takes [%s] as options",
dCN_options);
>+ return true;
>...
>+ pg_log_error("The server (version %s) does not support
this meta-command on psql.",
>+
formatPGVersionNumber(pset.sversion, false,
>+
sverbuf, sizeof(sverbuf)));
>+ return true;
>```
>
>Why return true after pg_log_error?

I believe these functions return true because this is not considered
a fatal error for the psql command itself.
The only case where they seem to return false is when NULL is returned
after issuing a query with PSQLexec().

This behavior is consistent with other commands such as \df, so I do
not think there is a particular problem with keeping it as is.

>6
>```
>+ if (!(showCheck || showForeign || showNotnull || showPrimary ||
showTrigger || showUnique || showExclusion))
>+ showAllkinds = true;
>```
>
>Nit: this might be simplified as: showAllkinds = !(showCheck || …), then
you don’t need to initialize showAllkinds as false.

Personally, I do not think there is any major issue with the current code,
but since you suggested it, I will simplify it.

The next patch will include the following changes based on your comments:
- 1. Fix the commit message
- s/all kind/all kinds/
- 2. Fix the error message
- s/dCN only takes [cfnptueSx+]/dCN only takes [cfnptue][Sx+]/
- 4. Fix the help message
- s/\dCN[cfnptue] [Sx+] [PATTERN]/\dCN[cfnptue][Sx+] [PATTERN]/
- 6. Simplify the code that sets the value of showAllkinds

Thanks,
Tatsuro Yamada