Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitce161b1

Browse files
committed
aio: Stop using enum bitfields due to bad code generation
During an investigation into rather odd aio related errors on macos, observedby Alexander and Konstantin, we started to wonder if bitfield access isrelated to the error. At the moment it looks like it is related, we cannotreproduce the failures when replacing the bitfields. In addition, the problemcan only be reproduced with some compiler [versions] and not everyone has beenable to reproduce the issue.The observed problem is that, very rarely, PgAioHandle->{state,target} are inan inconsistent state, after having been checked to be in a valid state notlong before, triggering an assertion failure. Unfortunately, this could becaused by wrong compiler code generation or somehow of missing memory barriers- we don't really know. In theory there should not be any concurrent writeaccess to the handle in the state the bug is triggered, as the handle was idleand is just being initialized.Separately from the bug, we observed that at least gcc and clang generaterather terrible code for the bitfield access. Even if it's not clear if theobserved assertion failure is actually caused by the bitfield somehow, the badcode generation alone is sufficient reason to stop using bitfields.Therefore, replace the enum bitfields with uint8s and instead cast in eachswitch statement.Reported-by: Alexander Lakhin <exclusion@gmail.com>Reported-by: Konstantin Knizhnik <knizhnik@garret.ru>Discussion:https://postgr.es/m/1500090.1745443021@sss.pgh.pa.usBackpatch-through: 18
1 parentbaf45ba commitce161b1

File tree

5 files changed

+21
-15
lines changed

5 files changed

+21
-15
lines changed

‎src/backend/storage/aio/aio.c‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ pgaio_io_release_resowner(dlist_node *ioh_node, bool on_error)
275275
ResourceOwnerForgetAioHandle(ioh->resowner,&ioh->resowner_node);
276276
ioh->resowner=NULL;
277277

278-
switch (ioh->state)
278+
switch ((PgAioHandleState)ioh->state)
279279
{
280280
casePGAIO_HS_IDLE:
281281
elog(ERROR,"unexpected");
@@ -600,7 +600,7 @@ pgaio_io_wait(PgAioHandle *ioh, uint64 ref_generation)
600600
if (pgaio_io_was_recycled(ioh,ref_generation,&state))
601601
return;
602602

603-
switch (state)
603+
switch ((PgAioHandleState)state)
604604
{
605605
casePGAIO_HS_IDLE:
606606
casePGAIO_HS_HANDED_OUT:
@@ -825,7 +825,7 @@ pgaio_io_wait_for_free(void)
825825
&pgaio_my_backend->in_flight_ios);
826826
uint64generation=ioh->generation;
827827

828-
switch (ioh->state)
828+
switch ((PgAioHandleState)ioh->state)
829829
{
830830
/* should not be in in-flight list */
831831
casePGAIO_HS_IDLE:
@@ -905,7 +905,7 @@ static const char *
905905
pgaio_io_state_get_name(PgAioHandleStates)
906906
{
907907
#definePGAIO_HS_TOSTR_CASE(sym) case PGAIO_HS_##sym: return #sym
908-
switch (s)
908+
switch ((PgAioHandleState)s)
909909
{
910910
PGAIO_HS_TOSTR_CASE(IDLE);
911911
PGAIO_HS_TOSTR_CASE(HANDED_OUT);
@@ -930,7 +930,7 @@ pgaio_io_get_state_name(PgAioHandle *ioh)
930930
constchar*
931931
pgaio_result_status_string(PgAioResultStatusrs)
932932
{
933-
switch (rs)
933+
switch ((PgAioResultStatus)rs)
934934
{
935935
casePGAIO_RS_UNKNOWN:
936936
return"UNKNOWN";

‎src/backend/storage/aio/aio_funcs.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ pg_get_aios(PG_FUNCTION_ARGS)
175175
values[4]=CStringGetTextDatum(pgaio_io_get_op_name(&ioh_copy));
176176

177177
/* columns: details about the IO's operation (offset, length) */
178-
switch (ioh_copy.op)
178+
switch ((PgAioOp)ioh_copy.op)
179179
{
180180
casePGAIO_OP_INVALID:
181181
nulls[5]= true;

‎src/backend/storage/aio/aio_io.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ pgaio_io_perform_synchronously(PgAioHandle *ioh)
121121
START_CRIT_SECTION();
122122

123123
/* Perform IO. */
124-
switch (ioh->op)
124+
switch ((PgAioOp)ioh->op)
125125
{
126126
casePGAIO_OP_READV:
127127
pgstat_report_wait_start(WAIT_EVENT_DATA_FILE_READ);
@@ -176,7 +176,7 @@ pgaio_io_get_op_name(PgAioHandle *ioh)
176176
{
177177
Assert(ioh->op >=0&&ioh->op<PGAIO_OP_COUNT);
178178

179-
switch (ioh->op)
179+
switch ((PgAioOp)ioh->op)
180180
{
181181
casePGAIO_OP_INVALID:
182182
return"invalid";
@@ -198,7 +198,7 @@ pgaio_io_uses_fd(PgAioHandle *ioh, int fd)
198198
{
199199
Assert(ioh->state >=PGAIO_HS_DEFINED);
200200

201-
switch (ioh->op)
201+
switch ((PgAioOp)ioh->op)
202202
{
203203
casePGAIO_OP_READV:
204204
returnioh->op_data.read.fd==fd;
@@ -222,7 +222,7 @@ pgaio_io_get_iovec_length(PgAioHandle *ioh, struct iovec **iov)
222222

223223
*iov=&pgaio_ctl->iovecs[ioh->iovec_off];
224224

225-
switch (ioh->op)
225+
switch ((PgAioOp)ioh->op)
226226
{
227227
casePGAIO_OP_READV:
228228
returnioh->op_data.read.iov_length;

‎src/backend/storage/aio/method_io_uring.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,7 @@ pgaio_uring_sq_from_io(PgAioHandle *ioh, struct io_uring_sqe *sqe)
660660
{
661661
structiovec*iov;
662662

663-
switch (ioh->op)
663+
switch ((PgAioOp)ioh->op)
664664
{
665665
casePGAIO_OP_READV:
666666
iov=&pgaio_ctl->iovecs[ioh->iovec_off];

‎src/include/storage/aio_internal.h‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,17 +92,23 @@ typedef enum PgAioHandleState
9292

9393
structResourceOwnerData;
9494

95-
/* typedef is in aio_types.h */
95+
/*
96+
* Typedef is in aio_types.h
97+
*
98+
* We don't use the underlying enums for state, target and op to avoid wasting
99+
* space. We tried using bitfields, but several compilers generate rather
100+
* horrid code for that.
101+
*/
96102
structPgAioHandle
97103
{
98104
/* all state updates should go through pgaio_io_update_state() */
99-
PgAioHandleStatestate:8;
105+
uint8state;
100106

101107
/* what are we operating on */
102-
PgAioTargetIDtarget:8;
108+
uint8target;
103109

104110
/* which IO operation */
105-
PgAioOpop:8;
111+
uint8op;
106112

107113
/* bitfield of PgAioHandleFlags */
108114
uint8flags;

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp