Horton

Thread

Commits

  1. Don't try to read a multi-GB pg_stat_statements file in one call.

  1. BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    The Post Office <noreply@postgresql.org> — 2021-10-29T05:05:48Z

    The following bug has been logged on the website:

Bug reference:      17254
Logged by:          yusuke egashira
Email address:      egashira.yusuke@fujitsu.com
PostgreSQL version: 14.0
Operating system:   Windows Server 2019
Description:        

The database server was crashed with 0xC0000409 in pg_stat_statements.

2021-10-28 14:15:14.244 JST [4980] LOG:  server process (PID 2556) was
terminated by exception 0xC0000409
2021-10-28 14:15:14.244 JST [4980] DETAIL:  Failed process was running:
select count(1) from pg_stat_statements;
2021-10-28 14:15:14.244 JST [4980] HINT:  See C include file "ntstatus.h"
for a description of the hexadecimal value.

This crash happened with PostgreSQL 14.0 for Windows downloaded from
https://www.enterprisedb.com/download-postgresql-binaries.

According to our investigation, when the pg_stat_statements reads the
pg_stat_tmp\pgss_query_texts.stat, the server seems to crash with 0xC0000409
if the size of pgss_query_texts.stat exceeds about 2GB.
Our user was performing tasks that executed a lot of very long SQL
statements (INSERT statement with over 100,000 parameters), and the server
crashed whenever pgss_query_texts.stat grew to a size larger than 2GB.

Strangely, I had created the crashdumps directory under PGHOME, but a
minidump file was not output.
# Doesn't a crash with 0xC0000409 output a dump?
I retrieved a minidump from postgres.exe which was rebuilt with enabled
dumping by WindowsErrorReport.
It seems that an exception occurred due to an argument error of _read().


* CHANGED:
diff --git a/src/backend/main/main.c b/src/backend/main/main.c
index e58e24a646..319c6ab62e 100644
--- a/src/backend/main/main.c
+++ b/src/backend/main/main.c
@@ -64,9 +64,6 @@ main(int argc, char *argv[])
         * If supported on the current platform, set up a handler to be
called if
         * the backend/postmaster crashes with a fatal signal or
exception.
         */
-#if defined(WIN32) && defined(HAVE_MINIDUMP_TYPE)
-       pgwin32_install_crashdump_handler();
-#endif

        progname = get_progname(argv[0]);

@@ -247,8 +244,6 @@ startup_hacks(const char *progname)
                        exit(1);
                }

-               /* In case of general protection fault, don't show GUI popup
box */
-               SetErrorMode(SEM_FAILCRITICALERRORS |
SEM_NOGPFAULTERRORBOX);


* STACK_TEXT:
ucrtbase!invoke_watson+0x18
ucrtbase!invalid_parameter+0x81
ucrtbase!invalid_parameter_noinfo+0x9
ucrtbase!_read+0x3aedc
pg_stat_statements!qtext_load_file+0x112
pg_stat_statements!pg_stat_statements_internal+0x3ae
pg_stat_statements!pg_stat_statements_1_9+0x17
postgres!ExecMakeTableFunctionResult+0x205
postgres!FunctionNext+0x62
postgres!ExecScanFetch+0x101
postgres!fetch_input_tuple+0x91
postgres!agg_retrieve_direct+0x245
postgres!ExecAgg+0x156
postgres!standard_ExecutorRun+0x13a
pg_stat_statements!pgss_ExecutorRun+0xa1
postgres!PortalRunSelect+0x96
postgres!PortalRun+0x1ef
postgres!exec_simple_query+0x627
postgres!PostgresMain+0x6e9
postgres!BackendRun+0x44
postgres!SubPostmasterMain+0x254
postgres!main+0x43b
postgres!__scrt_common_main_seh+0x10c
kernel32!BaseThreadInitThunk+0x14
ntdll!RtlUserThreadStart+0x21



The MSDN documentation says that the upper limit of the _read() argument is
INT_MAX (about 2GB), but the size gotten by fstat() exceeds this limit, so I
think we encountered server crash by an exception error.

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-160
> If buffer is NULL, or if buffer_size > INT_MAX, the invalid parameter
handler is invoked.

Until PostgreSQL 13, fstat() failed and returned ERROR when a file was
larger than 2GB, but as a result of improvements to fstat() in PostgreSQL
14, it appears that _read() has exceeded its limit and now causes a crash.

  2. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-10-29T19:43:35Z

    Thanks for the report.

On Fri, Oct 29, 2021 at 12:52 PM PG Bug reporting form <
noreply@postgresql.org> wrote:

>
> The MSDN documentation says that the upper limit of the _read() argument is
> INT_MAX (about 2GB), but the size gotten by fstat() exceeds this limit, so
> I
> think we encountered server crash by an exception error.
>
>
> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-160
> > If buffer is NULL, or if buffer_size > INT_MAX, the invalid parameter
> handler is invoked.
>
> Until PostgreSQL 13, fstat() failed and returned ERROR when a file was
> larger than 2GB, but as a result of improvements to fstat() in PostgreSQL
> 14, it appears that _read() has exceeded its limit and now causes a crash.
>
> The value of MaxAllocHugeSize is being oversized when _WIN64 is defined
[1]. Shouldn't the limit for a slurp be MaxAllocSize?

[1]
https://docs.microsoft.com/en-us/cpp/c-runtime-library/data-type-constants?view=msvc-160

Regards,

Juan José Santamaría Flecha

  3. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-10-30T08:24:46Z

    On Fri, Oct 29, 2021 at 9:43 PM Juan José Santamaría Flecha <
juanjo.santamaria@gmail.com> wrote:

>
> The value of MaxAllocHugeSize is being oversized when _WIN64 is defined
> [1]. Shouldn't the limit for a slurp be MaxAllocSize?
>
> Please find attached a patch for so.

Regards,

Juan José Santamaría Flecha

  4. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-10-30T08:27:29Z

    On Sat, Oct 30, 2021 at 10:24 AM Juan José Santamaría Flecha <
juanjo.santamaria@gmail.com> wrote:

>
> Please find attached a patch for so.
>>
>
> Regards,
>
> Juan José Santamaría Flecha
>
Now, with 100% more patch attached.

  5. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-30T16:26:43Z

    =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> Now, with 100% more patch attached.

That seems like a pretty poor solution.  It will cause pg_stat_statements
to fail altogether as soon as the stats file exceeds 1GB.  (Admittedly,
failing is better than crashing, but not by that much.)  Worse, it causes
that to happen on EVERY platform, not only Windows where the problem is.

I think instead, we need to turn the subsequent one-off read() call into a
loop that reads no more than INT_MAX bytes at a time.  It'd be possible
to restrict that to Windows, but probably no harm in doing it the same
way everywhere.

A different line of thought is that maybe we shouldn't be letting the
file get so big in the first place.  Letting every backend have its
own copy of a multi-gigabyte stats file is going to be problematic,
and not only on Windows.  It looks like the existing logic just considers
the number of hash table entries, not their size ... should we rearrange
things to keep a running count of the space used?

			regards, tom lane

  6. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-10-30T18:31:05Z

    On Sat, Oct 30, 2021 at 6:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> That seems like a pretty poor solution.  It will cause pg_stat_statements
> to fail altogether as soon as the stats file exceeds 1GB.  (Admittedly,
> failing is better than crashing, but not by that much.)  Worse, it causes
> that to happen on EVERY platform, not only Windows where the problem is.
>
> I don't think it is a Windows only problem, even on POSIX platforms it
might not be safe trying to read() over 2GB.


> I think instead, we need to turn the subsequent one-off read() call into a
> loop that reads no more than INT_MAX bytes at a time.  It'd be possible
> to restrict that to Windows, but probably no harm in doing it the same
> way everywhere.
>
> Seems reasonable to me, can such a change be back-patched?


> A different line of thought is that maybe we shouldn't be letting the
> file get so big in the first place.  Letting every backend have its
> own copy of a multi-gigabyte stats file is going to be problematic,
> and not only on Windows.  It looks like the existing logic just considers
> the number of hash table entries, not their size ... should we rearrange
> things to keep a running count of the space used?
>
> +1. There should be a mechanism to limit the effective memory size.

Regards,

Juan José Santamaría Flecha

  7. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-30T20:50:48Z

    =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sat, Oct 30, 2021 at 6:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think instead, we need to turn the subsequent one-off read() call into a
>> loop that reads no more than INT_MAX bytes at a time.  It'd be possible
>> to restrict that to Windows, but probably no harm in doing it the same
>> way everywhere.

> Seems reasonable to me, can such a change be back-patched?

Don't see why not.

>> A different line of thought is that maybe we shouldn't be letting the
>> file get so big in the first place.  Letting every backend have its
>> own copy of a multi-gigabyte stats file is going to be problematic,
>> and not only on Windows.  It looks like the existing logic just considers
>> the number of hash table entries, not their size ... should we rearrange
>> things to keep a running count of the space used?

> +1. There should be a mechanism to limit the effective memory size.

This, on the other hand, would likely be something for HEAD only.
But now that we've seen a field complaint, it seems like a good
thing to pursue.

			regards, tom lane

  8. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-31T17:07:42Z

    I wrote:
> =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
>> On Sat, Oct 30, 2021 at 6:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think instead, we need to turn the subsequent one-off read() call into a
>>> loop that reads no more than INT_MAX bytes at a time.  It'd be possible
>>> to restrict that to Windows, but probably no harm in doing it the same
>>> way everywhere.

>> Seems reasonable to me, can such a change be back-patched?

> Don't see why not.

Here's a quick patch for that.  I don't have any ability to check it
on Windows, but the logic is easy to verify by reducing the arbitrary
constant to something small.  (I used 1GB, not INT_MAX, because I figured
we ought to read in multiples of a filesystem block if possible.)

			regards, tom lane

  9. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-31T22:46:34Z

    =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sun, Oct 31, 2021 at 6:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a quick patch for that.  I don't have any ability to check it
>> on Windows, but the logic is easy to verify by reducing the arbitrary
>> constant to something small.  (I used 1GB, not INT_MAX, because I figured
>> we ought to read in multiples of a filesystem block if possible.)

> I have tested the patch in Windows and it works as expected.

Great, thanks for testing!

			regards, tom lane

  10. Re: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Juan José Santamaría Flecha <juanjo.santamaria@gmail.com> — 2021-10-31T23:34:15Z

    On Sun, Oct 31, 2021 at 6:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> Here's a quick patch for that.  I don't have any ability to check it
> on Windows, but the logic is easy to verify by reducing the arbitrary
> constant to something small.  (I used 1GB, not INT_MAX, because I figured
> we ought to read in multiples of a filesystem block if possible.)
>
>  I have tested the patch in Windows and it works as expected.

Regards,

Juan José Santamaría Flecha

  11. RE: BUG #17254: Crash with 0xC0000409 in pg_stat_statements when pg_stat_tmp\pgss_query_texts.stat exceeded 2GB.

    Yusuke Egashira (Fujitsu) <egashira.yusuke@fujitsu.com> — 2021-11-02T02:56:57Z

    =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= <juanjo.santamaria@gmail.com> writes:
> On Sun, Oct 31, 2021 at 6:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a quick patch for that.  I don't have any ability to check it
>> on Windows, but the logic is easy to verify by reducing the arbitrary
>> constant to something small.  (I used 1GB, not INT_MAX, because I figured
>> we ought to read in multiples of a filesystem block if possible.)

> I have tested the patch in Windows and it works as expected.

I have also patched our environment to make sure it does not crash.
Thank you for your prompt work.

Regards.
Yusuke Egashira.