From: Roman Khapov <rkhapov@yandex-team.ru>
Message-Id: <F97A21C3-5EBB-466F-B71D-14F30BD94CF7@yandex-team.ru>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_11278ED4-CE90-4189-816B-C311AB3B39FD"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.500.181.1.5\))
Subject: [PATCH v2] Re: Cancel problems of query to pg_stat_statements
Date: Mon, 12 May 2025 15:48:29 +0500
In-Reply-To: <26A1F7AF-810A-4D60-A6CB-8B749BCCE747@yandex-team.ru>
Cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
To: Andrey Borodin <x4mmm@yandex-team.ru>
References: <31431745515032@mail.yandex-team.ru>
 <26A1F7AF-810A-4D60-A6CB-8B749BCCE747@yandex-team.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/F97A21C3-5EBB-466F-B71D-14F30BD94CF7%40yandex-team.ru>
Precedence: bulk


--Apple-Mail=_11278ED4-CE90-4189-816B-C311AB3B39FD
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hi! Thanks for the review!

Its true that qtext_load_file() can be called with acquired lock in =
pg_stat_statements_internal()=E2=80=A6
So I perform INTERRUPTS_PENDING_CONDITION in v2 patch and call =
CHECK_FOR_INTERRUPTS later, after cycle ended and lock released.


--Apple-Mail=_11278ED4-CE90-4189-816B-C311AB3B39FD
Content-Disposition: attachment;
	filename=v2-cancelable-qtext_load_file.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v2-cancelable-qtext_load_file.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20f642c28124e1bd57764bd12296253b78fc9c083c=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20rkhapov=20<r.khapov@ya.ru>=0ADate:=20Mon,=2012=20=
May=202025=2015:29:17=20+0500=0ASubject:=20[PATCH=20v2]=20=
pg_stat_statements.c:=20cancelable=20qtext_load_file=0A=0AIn=20the=20=
case=20of=20a=20large=20PGSS_TEXT_FILE,=20the=20work=20time=20of=20the=20=
qtext_load_file=0Afunction=20will=20be=20quite=20long,=20and=20the=20=
query=20to=20the=20pg_stat_statements=20table=0Awill=20not=20be=20=
cancellable,=20as=20there=20is=20no=20CHECK_FOR_INTERRUPT=20in=20the=20=
function.=0A=0AAlso,=20the=20amount=20of=20bytes=20read=20can=20reach=20=
1=20GB,=20which=20leads=20to=20a=20slow=20read=0Asystem=20call=20that=20=
does=20not=20allow=20cancellation=20of=20the=20query.=20Testing=20the=20=
speed=0Aof=20sequential=20read=20using=20fio=20with=20different=20block=20=
sizes=20shows=20that=20there=20is=0Ano=20significant=20difference=20=
between=2016=20MB=20blocks=20and=201=20GB=20blocks.=0A=0ATherefore,=20=
this=20patch=20changes=20the=20maximum=20read=20value=20from=201=20GB=20=
to=2016=20MB=20and=0Aadds=20INTERRUPTS_PENDING_CONDITION()=20check=20in=20=
the=20read=20loop=20of=20qtext_load_file=20to=20make=20it=20cancellable.=0A=
For=20now,=20only=20statement=20execution=20is=20cancellable=20=
(fail_on_interrupt=20is=20true=20only=20for=20calls=20from=20=
pg_stat_statements_internal)=0A=0ASigned-off-by:=20rkhapov=20=
<r.khapov@ya.ru>=0A---=0A=20.../pg_stat_statements/pg_stat_statements.c=20=
=20=20|=2029=20++++++++++++++-----=0A=201=20file=20changed,=2022=20=
insertions(+),=207=20deletions(-)=0A=0Adiff=20--git=20=
a/contrib/pg_stat_statements/pg_stat_statements.c=20=
b/contrib/pg_stat_statements/pg_stat_statements.c=0Aindex=20=
9778407cba3..587d49f4330=20100644=0A---=20=
a/contrib/pg_stat_statements/pg_stat_statements.c=0A+++=20=
b/contrib/pg_stat_statements/pg_stat_statements.c=0A@@=20-365,7=20+365,7=20=
@@=20static=20pgssEntry=20*entry_alloc(pgssHashKey=20*key,=20Size=20=
query_offset,=20int=20query_len=0A=20static=20void=20=
entry_dealloc(void);=0A=20static=20bool=20qtext_store(const=20char=20=
*query,=20int=20query_len,=0A=20=09=09=09=09=09=09Size=20*query_offset,=20=
int=20*gc_count);=0A-static=20char=20*qtext_load_file(Size=20=
*buffer_size);=0A+static=20char=20*qtext_load_file(Size=20*buffer_size,=20=
bool=20fail_on_interrupts);=0A=20static=20char=20*qtext_fetch(Size=20=
query_offset,=20int=20query_len,=0A=20=09=09=09=09=09=09=20char=20=
*buffer,=20Size=20buffer_size);=0A=20static=20bool=20=
need_gc_qtexts(void);=0A@@=20-767,7=20+767,7=20@@=20=
pgss_shmem_shutdown(int=20code,=20Datum=20arg)=0A=20=09if=20=
(fwrite(&num_entries,=20sizeof(int32),=201,=20file)=20!=3D=201)=0A=20=09=09=
goto=20error;=0A=20=0A-=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size);=0A+=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size,=20false=20/*=20fail_on_interrupts=20*/=20=
);=0A=20=09if=20(qbuffer=20=3D=3D=20NULL)=0A=20=09=09goto=20error;=0A=20=0A=
@@=20-1767,7=20+1767,7=20@@=20=
pg_stat_statements_internal(FunctionCallInfo=20fcinfo,=0A=20=0A=20=09=09=
/*=20No=20point=20in=20loading=20file=20now=20if=20there=20are=20active=20=
writers=20*/=0A=20=09=09if=20(n_writers=20=3D=3D=200)=0A-=09=09=09=
qbuffer=20=3D=20qtext_load_file(&qbuffer_size);=0A+=09=09=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size,=20true=20/*=20fail_on_interrupts=20*/=20=
);=0A=20=09}=0A=20=0A=20=09/*=0A@@=20-1800,7=20+1800,7=20@@=20=
pg_stat_statements_internal(FunctionCallInfo=20fcinfo,=0A=20=09=09=09=
pgss->gc_count=20!=3D=20gc_count)=0A=20=09=09{=0A=20=09=09=09=
free(qbuffer);=0A-=09=09=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size);=0A+=09=09=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size,=20true=20/*=20fail_on_interrupts=20*/=20=
);=0A=20=09=09}=0A=20=09}=0A=20=0A@@=20-1819,6=20+1819,12=20@@=20=
pg_stat_statements_internal(FunctionCallInfo=20fcinfo,=0A=20=09=09=
memset(values,=200,=20sizeof(values));=0A=20=09=09memset(nulls,=200,=20=
sizeof(nulls));=0A=20=0A+=09=09/*=20Can't=20process=20interrupts=20here=20=
-=20pgss-lock=20is=20acquired=20*/=0A+=09=09if=20=
(INTERRUPTS_PENDING_CONDITION())=0A+=09=09{=0A+=09=09=09break;=0A+=09=09=
}=0A+=0A=20=09=09values[i++]=20=3D=20=
ObjectIdGetDatum(entry->key.userid);=0A=20=09=09values[i++]=20=3D=20=
ObjectIdGetDatum(entry->key.dbid);=0A=20=09=09if=20(api_version=20>=3D=20=
PGSS_V1_9)=0A@@=20-2014,6=20+2020,8=20@@=20=
pg_stat_statements_internal(FunctionCallInfo=20fcinfo,=0A=20=0A=20=09=
LWLockRelease(pgss->lock);=0A=20=0A+=09CHECK_FOR_INTERRUPTS();=0A+=0A=20=09=
free(qbuffer);=0A=20}=0A=20=0A@@=20-2312,7=20+2320,7=20@@=20error:=0A=20=20=
*=20the=20caller=20is=20responsible=20for=20verifying=20that=20the=20=
result=20is=20sane.=0A=20=20*/=0A=20static=20char=20*=0A=
-qtext_load_file(Size=20*buffer_size)=0A+qtext_load_file(Size=20=
*buffer_size,=20bool=20fail_on_interrupts)=0A=20{=0A=20=09char=09=20=20=20=
*buf;=0A=20=09int=09=09=09fd;=0A@@=20-2365,7=20+2373,14=20@@=20=
qtext_load_file(Size=20*buffer_size)=0A=20=09nread=20=3D=200;=0A=20=09=
while=20(nread=20<=20stat.st_size)=0A=20=09{=0A-=09=09int=09=09=09toread=20=
=3D=20Min(1024=20*=201024=20*=201024,=20stat.st_size=20-=20nread);=0A+=09=
=09int=09=09=09toread=20=3D=20Min(32=20*=201024=20*=201024,=20=
stat.st_size=20-=20nread);=0A+=0A+=09=09if=20(fail_on_interrupts=20&&=20=
INTERRUPTS_PENDING_CONDITION())=0A+=09=09{=0A+=09=09=09free(buf);=0A+=09=09=
=09CloseTransientFile(fd);=0A+=09=09=09return=20NULL;=0A+=09=09}=0A=20=0A=
=20=09=09/*=0A=20=09=09=20*=20If=20we=20get=20a=20short=20read=20and=20=
errno=20doesn't=20get=20set,=20the=20reason=20is=0A@@=20-2502,7=20=
+2517,7=20@@=20gc_qtexts(void)=0A=20=09=20*=20file=20is=20only=20going=20=
to=20get=20bigger;=20hoping=20for=20a=20future=20non-OOM=20result=20is=0A=
=20=09=20*=20risky=20and=20can=20easily=20lead=20to=20complete=20denial=20=
of=20service.=0A=20=09=20*/=0A-=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size);=0A+=09qbuffer=20=3D=20=
qtext_load_file(&qbuffer_size,=20false=20/*=20fail_on_interrupts=20*/=20=
);=0A=20=09if=20(qbuffer=20=3D=3D=20NULL)=0A=20=09=09goto=20gc_fail;=0A=20=
=0A--=20=0A2.39.5=20(Apple=20Git-154)=0A=0A=

--Apple-Mail=_11278ED4-CE90-4189-816B-C311AB3B39FD
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii


> On 28 Apr 2025, at 11:16, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>=20
>=20
>=20
>> On 24 Apr 2025, at 22:49, Roman Khapov <rkhapov@yandex-team.ru> =
wrote:
>>=20
>> Hi!
>> Recently we faced a problem in out production psql installation, =
which was that we had to cancel all requests to the db, including =
performance monitoring requests, that uses ps_stat_statements. But we =
could not cancel the request in usual way, and had to kill -9 the pg =
process of it.
>=20
> Interesting problem, thanks for raising it!
>=20
>> We've noticed that the the query execution stuck on PGSS_TEXT_FILE =
file reading in function qtext_load_file, which doesn't have =
CHECK_FOR_INTERRUPTS in the read cycle. In addition to our case with =
large PGSS_TEXT_FILE (and maybe the problems with virtual disk i/o) that =
can explain uncancellable pg_stat_statements queries.
>=20
> I'm afraid it might be not so easy to add CHECK_FOR_INTERRUPTS there. =
Most probably you was holding a LWLockAcquire(pgss->lock, LW_SHARED) =
somewhere (do you have a backtrace?), which prevent interrupts anyway.
>=20
> Thanks!
>=20
>=20
> Best regards, Andrey Borodin.


--Apple-Mail=_11278ED4-CE90-4189-816B-C311AB3B39FD--