On 2025/06/19 11:38, jian he wrote:
> On Wed, Jun 18, 2025 at 11:05 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>>
>> I agree that this is roughly the right approach, but I think you're
>> doing it harder than it needs to be -- it might be easier to add a JOIN
>> to pg_description to the big query in getTableAttrs(), and add a pointer
>> to the returned string in tbinfo->notnull_comments[j] (for versions
>> earlier than 18, don't add the join and have it return constant NULL).
>> Then in dumpTableSchema, in the place where you added the new query,
>> just scan that array and print COMMENT ON commands for each valid
>> constraint where that's not a null pointer.
>>
>
> Previously I was worried about print_notnull, shouldPrintColumn.
> if there is a not-null constraint that is not dumped separately, it has comments
> then we should dump these comments, then no need to worry about print_notnull.
>
> using notnull_comments saves us one more query.
> however, in determineNotNullFlags we have:
>
> char *default_name;
> /* XXX should match ChooseConstraintName better */
> default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
> tbinfo->attnames[j]);
> if (strcmp(default_name,
> PQgetvalue(res, r, i_notnull_name)) == 0)
> tbinfo->notnull_constrs[j] = "";
Do we really need this part? With the patch, due to this part,
pg_dump might emit output like:
CREATE TABLE t (i int NOT NULL);
COMMENT ON CONSTRAINT t_i_not_null ON public.t IS '.......';
This seems to rely on the assumption that the naming convention for
NOT NULL constraints (e.g., t_i_not_null) will remain stable across versions.
If that ever changes, the dumped DDL could fail.
Wouldn't it be safer to always print the constraint name explicitly,
even if it's the default name? For example:
CREATE TABLE t (i int CONSTRAINT t_i_not_null NOT NULL);
COMMENT ON CONSTRAINT t_i_not_null ON public.t IS '.......';
This is consistent with how check constraints are handled.
Regarding the patch:
+ if (!fout->dopt->no_comments &&
+ dopt->dumpSchema &&
+ fout->remoteVersion >= 180000)
The dopt->dumpSchema check is redundant, since dumpTableSchema() is
only called when that flag is already true?
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
