Обсуждение: pgsql: Fix pg_dump assertion failure when dumping pg_catalog.

Fix pg_dump assertion failure when dumping pg_catalog.

Commit 396d348b04 did not account for the default collation.

Also, use pg_log_warning() instead of Assert().

Discussion: https://postgr.es/m/ce071503fee88334aa70f360e6e4ea14d48305ee.camel%40j-davis.com
Reviewed-by: Michael Paquier
Backpatch-through: 15

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/37188cea0c16f3f27ec65b526ffe12f6469142fc

Modified Files
--------------
src/bin/pg_dump/pg_dump.c        | 69 ++++++++++++++++++++++++++++++----------
src/bin/pg_dump/t/002_pg_dump.pl |  8 +++++
2 files changed, 61 insertions(+), 16 deletions(-)

On 22.08.23 22:08, Jeff Davis wrote:
> Fix pg_dump assertion failure when dumping pg_catalog.
> 
> Commit 396d348b04 did not account for the default collation.
> 
> Also, use pg_log_warning() instead of Assert().

Looking at this:

     pg_log_warning("invalid collation \"%s\"", qcollname);

qcollname is already a quoted identifier, so this message would be 
doubly quoted.  Maybe you should use collinfo->dobj.name in the messages.

Also, for

     pg_fatal("unrecognized collation provider '%c'", collprovider[0]);

use double quotes.

On 22.08.23 22:08, Jeff Davis wrote:
> Fix pg_dump assertion failure when dumping pg_catalog.
> 
> Commit 396d348b04 did not account for the default collation.
> 
> Also, use pg_log_warning() instead of Assert().
> 
> Discussion: https://postgr.es/m/ce071503fee88334aa70f360e6e4ea14d48305ee.camel%40j-davis.com
> Reviewed-by: Michael Paquier
> Backpatch-through: 15

I have another question about this patch.  The original issue was that 
it would trigger an Assert() inside pg_dump when some columns in 
pg_collation were null that were not expected.  This patch now contains 
code like

             appendPQExpBufferStr(q, ", lc_collate = ");
-           appendStringLiteralAH(q, collcollate, fout);
+           appendStringLiteralAH(q, collcollate ? collcollate : "", fout);
             appendPQExpBufferStr(q, ", lc_ctype = ");
-           appendStringLiteralAH(q, collctype, fout);
+           appendStringLiteralAH(q, collctype ? collctype : "", fout);

which would produce pg_dump output like

CREATE COLLATION ... (provider = libc, lc_collate = , lc_ctype = );

which is not valid syntax.

Peter Eisentraut <peter@eisentraut.org> writes:
> I have another question about this patch.  The original issue was that 
> it would trigger an Assert() inside pg_dump when some columns in 
> pg_collation were null that were not expected.  This patch now contains 
> code like

>              appendPQExpBufferStr(q, ", lc_collate = ");
> -           appendStringLiteralAH(q, collcollate, fout);
> +           appendStringLiteralAH(q, collcollate ? collcollate : "", fout);
>              appendPQExpBufferStr(q, ", lc_ctype = ");
> -           appendStringLiteralAH(q, collctype, fout);
> +           appendStringLiteralAH(q, collctype ? collctype : "", fout);

> which would produce pg_dump output like

> CREATE COLLATION ... (provider = libc, lc_collate = , lc_ctype = );

> which is not valid syntax.

How so?  appendStringLiteral adds quotes around what it's given,
empty string or no.

The receiving server might or might not like those parameters
semantically, but the syntax should be ok.

            regards, tom lane