Обсуждение: WaitLatchOrSocket seems to not count to 4 right...
Unless I'm misreading this code I think the nevents in WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls to AddWaitEventToSet in it and I think it's possible to trigger all 4. I guess it's based on knowing that nobody would actually set WL_EXIT_ON_PM_DEATH and WL_POSTMASTER_DEATH on the same waitset? -- greg
On 2022/02/08 7:00, Greg Stark wrote: > Unless I'm misreading this code I think the nevents in > WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls > to AddWaitEventToSet in it and I think it's possible to trigger all 4. Good catch! I think you're right. As the quick test, I confirmed that the assertion failure happened when I passed four possible events to WaitLatchOrSocket()in postgres_fdw. TRAP: FailedAssertion("set->nevents < set->nevents_space", File: "latch.c", Line: 868, PID: 54424) 0 postgres 0x0000000107efa49f ExceptionalCondition + 223 1 postgres 0x0000000107cbca0c AddWaitEventToSet + 76 2 postgres 0x0000000107cbd86e WaitLatchOrSocket + 430 3 postgres_fdw.so 0x000000010848b1aa pgfdw_get_result + 218 4 postgres_fdw.so 0x000000010848accb do_sql_command + 75 5 postgres_fdw.so 0x000000010848c6b8 configure_remote_session + 40 6 postgres_fdw.so 0x000000010848c32d connect_pg_server + 1629 7 postgres_fdw.so 0x000000010848aa06 make_new_connection + 566 8 postgres_fdw.so 0x0000000108489a06 GetConnection + 550 9 postgres_fdw.so 0x0000000108497ba4 postgresBeginForeignScan + 260 10 postgres 0x0000000107a7d79f ExecInitForeignScan + 943 11 postgres 0x0000000107a5c8ab ExecInitNode + 683 12 postgres 0x0000000107a5028a InitPlan + 1386 13 postgres 0x0000000107a4fb66 standard_ExecutorStart + 806 14 postgres 0x0000000107a4f833 ExecutorStart + 83 15 postgres 0x0000000107d0277f PortalStart + 735 16 postgres 0x0000000107cfe150 exec_simple_query + 1168 17 postgres 0x0000000107cfd39e PostgresMain + 2110 18 postgres 0x0000000107c07e72 BackendRun + 50 19 postgres 0x0000000107c07438 BackendStartup + 552 20 postgres 0x0000000107c0621c ServerLoop + 716 21 postgres 0x0000000107c039f9 PostmasterMain + 6441 22 postgres 0x0000000107ae20d9 main + 809 23 libdyld.dylib 0x00007fff2045cf3d start + 1 24 ??? 0x0000000000000003 0x0 + 3 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Tue, Feb 8, 2022 at 1:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > On 2022/02/08 7:00, Greg Stark wrote: > > Unless I'm misreading this code I think the nevents in > > WaitLatchOrSocket should really be 4 not 3. At least there are 4 calls > > to AddWaitEventToSet in it and I think it's possible to trigger all 4. > > Good catch! I think you're right. > > As the quick test, I confirmed that the assertion failure happened when I passed four possible events to WaitLatchOrSocket()in postgres_fdw. > > TRAP: FailedAssertion("set->nevents < set->nevents_space", File: "latch.c", Line: 868, PID: 54424) Yeah, the assertion shows there's no problem, but we should change it to 4 (and one day we should make it dynamic and change udata to hold an index instead of a pointer...) or, since it's a bit silly to add both of those events, maybe we should do: - if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) - AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, - NULL, NULL); - if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster) AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET, NULL, NULL); + else if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) + AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, +
On 2022/02/08 9:51, Thomas Munro wrote: > an index instead of a pointer...) or, since it's a bit silly to add > both of those events, maybe we should do: > > - if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) > - AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, > - NULL, NULL); > - > if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster) > AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET, > NULL, NULL); > + else if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster) > + AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET, > + +1 Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Tue, Feb 8, 2022 at 1:51 PM Thomas Munro <thomas.munro@gmail.com> wrote: > (and one day we should make it dynamic and change udata to hold > an index instead of a pointer...) Here's a patch like that. I'd originally sketched this out for another project, but I don't think I need it for that anymore. After this exchange I couldn't resist fleshing it out for a commitfest, just on useability grounds. Thoughts?
Вложения
Hi, On 2022-02-11 10:47:45 +1300, Thomas Munro wrote: > I'd originally sketched this out for another project, but I don't > think I need it for that anymore. After this exchange I couldn't > resist fleshing it out for a commitfest, just on useability grounds. > Thoughts? This currently doesn't apply: http://cfbot.cputube.org/patch_37_3533.log Marked as waiting-on-author for now. Are you aiming this for 15? Greetings, Andres Freund
This entry has been waiting on author input for a while (our current threshold is roughly two weeks), so I've marked it Returned with Feedback. Once you think the patchset is ready for review again, you (or any interested party) can resurrect the patch entry by visiting https://commitfest.postgresql.org/38/3533/ and changing the status to "Needs Review", and then changing the status again to "Move to next CF". (Don't forget the second step; hopefully we will have streamlined this in the near future!) Thanks, --Jacob