Horton

Thread

Commits

  1. Avoid using %c printf format for potentially non-ASCII characters.

  1. bugfix: invalid bit/varbit input causes the log file to be unreadable

    Quan Zongliang <quanzongliang@gmail.com> — 2020-06-26T06:44:40Z

    The bit/varbit type input functions cause file_fdw to fail to read the 
logfile normally.

1. Server conf:
     server_encoding = UTF8
     locale = zh_CN.UTF-8

2. Create external tables using file_fdw

CREATE EXTENSION file_fdw;
CREATE SERVER pglog FOREIGN DATA WRAPPER file_fdw;

CREATE FOREIGN TABLE pglog (
      log_time timestamp(3) with time zone,
      user_name text,
      database_name text,
      process_id integer,
      connection_from text,
      session_id text,
      session_line_num bigint,
      command_tag text,
      session_start_time timestamp with time zone,
      virtual_transaction_id text,
      transaction_id bigint,
      error_severity text,
      sql_state_code text,
      message text,
      detail text,
      hint text,
      internal_query text,
      internal_query_pos integer,
      context text,
      query text,
      query_pos integer,
      location text,
      application_name text
) SERVER pglog
OPTIONS ( filename 'log/postgresql-2020-06-16_213409.csv',
      format 'csv');

It's normal to be here.

3. bit/varbit input
      select b'Ù';

The foreign table cannot be accessed. SELECT * FROM pglog will get:
invalid byte sequence for encoding "UTF8": 0xc3 0x22


The reason is that the error message in the bit_in / varbit_in function 
is output directly using %c. Causes the log file to not be decoded 
correctly.

The attachment is a patch.

  2. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-26T16:45:43Z

    Quan Zongliang <quanzongliang@gmail.com> writes:
> The reason is that the error message in the bit_in / varbit_in function 
> is output directly using %c. Causes the log file to not be decoded 
> correctly.

> The attachment is a patch.

I'm really quite skeptical of the premise here.  We do not guarantee that
the postmaster log file is valid in any particular encoding; it'd be
nearly impossible to do so if the cluster contains databases using
different encodings.  So I think you'd be way better off to reformulate
your log-reading code to be less fragile.

Even granting the premise, the proposed patch seems like a significant
decrease in user-friendliness for typical cases.  I'd rather see us
make an effort to print one valid-per-the-DB-encoding character.
Now that we can rely on snprintf to count %s restrictions in bytes,
I think something like this should work:

                         errmsg("\"%.*s\" is not a valid binary digit",
                                pg_mblen(sp), sp)));

But the real problem is that this is only the tip of the iceberg.
You didn't even hit all the %c usages in varbit.c.  A quick grep finds
these other spots that can doubtless be made to do the same thing:

acl.c:899:			elog(ERROR, "unrecognized objtype abbreviation: %c", objtypec);
arrayfuncs.c:507:								 errdetail("Unexpected \"%c\" character.",
arrayfuncs.c:554:									 errdetail("Unexpected \"%c\" character.",
arrayfuncs.c:584:									 errdetail("Unexpected \"%c\" character.",
arrayfuncs.c:591:									 errdetail("Unmatched \"%c\" character.", '}')));
arrayfuncs.c:633:										 errdetail("Unexpected \"%c\" character.",
encode.c:184:				 errmsg("invalid hexadecimal digit: \"%c\"", c)));
encode.c:341:						 errmsg("invalid symbol \"%c\" while decoding base64 sequence", (int) c)));
formatting.c:3298:										  errmsg("unmatched format separator \"%c\"",
jsonpath_gram.c:2390:						 errdetail("unrecognized flag character \"%c\" in LIKE_REGEX predicate",
regexp.c:426:							 errmsg("invalid regular expression option: \"%c\"",
tsvector_op.c:312:			elog(ERROR, "unrecognized weight: %c", char_weight);
tsvector_op.c:872:						 errmsg("unrecognized weight: \"%c\"", char_weight)));
varbit.c:233:						 errmsg("\"%c\" is not a valid binary digit",
varbit.c:258:						 errmsg("\"%c\" is not a valid hexadecimal digit",
varbit.c:534:						 errmsg("\"%c\" is not a valid binary digit",
varbit.c:559:						 errmsg("\"%c\" is not a valid hexadecimal digit",
varlena.c:5589:					 errmsg("unrecognized format() type specifier \"%c\"",
varlena.c:5710:						 errmsg("unrecognized format() type specifier \"%c\"",

and that's just in src/backend/utils/adt/.

			regards, tom lane

  3. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-28T17:10:59Z

    I wrote:
> Even granting the premise, the proposed patch seems like a significant
> decrease in user-friendliness for typical cases.  I'd rather see us
> make an effort to print one valid-per-the-DB-encoding character.
> Now that we can rely on snprintf to count %s restrictions in bytes,
> I think something like this should work:
>                          errmsg("\"%.*s\" is not a valid binary digit",
>                                 pg_mblen(sp), sp)));
> But the real problem is that this is only the tip of the iceberg.
> You didn't even hit all the %c usages in varbit.c.

I went through all the %c format sequences in the backend to see which
ones could use this type of fix.  There were not as many as I'd expected,
but still a fair number.  (I skipped cases where the input was coming from
the catalogs, as well as some non-user-facing debug printouts.)  That
leads to the attached patch, which seems to do the job without breaking
anything that works today.

			regards, tom lane

PS: I failed to resist the temptation to improve some shoddy error
messages nearby in pageinspect/heapfuncs.c.

  4. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Quan Zongliang <quanzongliang@gmail.com> — 2020-06-29T10:45:47Z

    Good.

I tested it, and it looks fine.

Thank you.


On 2020/6/29 1:10 上午, Tom Lane wrote:
> I wrote:
>> Even granting the premise, the proposed patch seems like a significant
>> decrease in user-friendliness for typical cases.  I'd rather see us
>> make an effort to print one valid-per-the-DB-encoding character.
>> Now that we can rely on snprintf to count %s restrictions in bytes,
>> I think something like this should work:
>>                           errmsg("\"%.*s\" is not a valid binary digit",
>>                                  pg_mblen(sp), sp)));
>> But the real problem is that this is only the tip of the iceberg.
>> You didn't even hit all the %c usages in varbit.c.
> I went through all the %c format sequences in the backend to see which
> ones could use this type of fix.  There were not as many as I'd expected,
> but still a fair number.  (I skipped cases where the input was coming from
> the catalogs, as well as some non-user-facing debug printouts.)  That
> leads to the attached patch, which seems to do the job without breaking
> anything that works today.
>
> 			regards, tom lane
>
> PS: I failed to resist the temptation to improve some shoddy error
> messages nearby in pageinspect/heapfuncs.c.
>

  5. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-29T15:42:23Z

    Quan Zongliang <quanzongliang@gmail.com> writes:
> I tested it, and it looks fine.

Pushed, thanks for reporting the issue!

			regards, tom lane

  6. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Huansong Fu <huansong.fu.info@gmail.com> — 2021-12-13T19:42:11Z

    Hi, 

We recently saw a similar issue in v12 and wondered why the corresponding fix for v14 (https://github.com/postgres/postgres/commit/16e3ad5d143) was not backported to v13 and before. The commit message did mention that this fix might have problem with translatable string messages - would you mind providing a bit more context about what is needed to backport this fix? Thank you.

Regards,
Huansong
https://vmware.com/ <https://vmware.com/>

> On Jun 29, 2020, at 11:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Quan Zongliang <quanzongliang@gmail.com> writes:
>> I tested it, and it looks fine.
> 
> Pushed, thanks for reporting the issue!
> 
> 			regards, tom lane
> 
> 
> 
>

  7. Re: bugfix: invalid bit/varbit input causes the log file to be unreadable

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-13T20:33:30Z

    Huansong Fu <huansong.fu.info@gmail.com> writes:
> We recently saw a similar issue in v12 and wondered why the corresponding fix for v14 (https://github.com/postgres/postgres/commit/16e3ad5d143) was not backported to v13 and before. The commit message did mention that this fix might have problem with translatable string messages - would you mind providing a bit more context about what is needed to backport this fix? Thank you.

Well, the commit message lists the reasons for not back-patching:

* we've seen few field complaints about such problems
* it'd add work for translators
* it wouldn't work reliably before v12.

Perhaps there's a case for back-patching as far as v12,
but I can't get very excited about it.

			regards, tom lane