Thread
Commits
-
Optimize fileset usage in apply worker.
- 31c389d8de91 15.0 landed
-
Refactor sharedfileset.c to separate out fileset implementation.
- dcac5e7ac157 15.0 landed
-
Move temporary file cleanup to before_shmem_exit().
- 675c945394b3 15.0 landed
-
pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV.
- ee3f8d3d3aec 15.0 cited
-
Add infrastructure for sharing temporary files between backends.
- dc6c4c9dc2a1 11.0 cited
-
pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T02:30:26Z
pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV. Previously pgstat_initialize() was called in InitPostgres() and AuxiliaryProcessMain(). As it turns out there was at least one case where we reported stats before pgstat_initialize() was called, see AutoVacWorkerMain()'s intentionally early call to pgstat_report_autovac(). This turns out to not be a problem with the current pgstat implementation as pgstat_initialize() only registers a shutdown callback. But in the shared memory based stats implementation we are working towards pgstat_initialize() has to do more work. After b406478b87e BaseInit() is a central place where initialization shared by normal backends and auxiliary backends can be put. Obviously BaseInit() is called before InitPostgres() registers ShutdownPostgres. Previously ShutdownPostgres was the first before_shmem_exit callback, now that's commonly pgstats. That should be fine. Previously pgstat_initialize() was not called in bootstrap mode, but there does not appear to be a need for that. It's now done unconditionally. To detect future issues like this, assertions are added to a few places verifying that the pgstat subsystem is initialized and not yet shut down. Author: Andres Freund <andres@anarazel.de> Discussion: https://postgr.es/m/20210405092914.mmxqe7j56lsjfsej@alap3.anarazel.de Discussion: https://postgr.es/m/20210802164124.ufo5buo4apl6yuvs@alap3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ee3f8d3d3aec0d7c961d6b398d31504bb272a450 Modified Files -------------- src/backend/postmaster/auxprocess.c | 2 -- src/backend/postmaster/pgstat.c | 53 +++++++++++++++++++++++++++++++++++-- src/backend/utils/init/postinit.c | 23 +++++++++------- 3 files changed, 65 insertions(+), 13 deletions(-)
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T02:44:07Z
Andres Freund <andres@anarazel.de> writes: > pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV. sifaka took exception to this, or at least I guess it was this one out of the four you pushed at once: TRAP: FailedAssertion("pgstat_is_initialized && !pgstat_is_shutdown", File: "pgstat.c", Line: 4810, PID: 74447) 0 postgres 0x0000000100e5a520 ExceptionalCondition + 124 1 postgres 0x0000000100ca1dec pgstat_reset_counters + 0 2 postgres 0x0000000100ca2548 pgstat_report_tempfile + 80 3 postgres 0x0000000100d07ca4 FileClose + 536 4 postgres 0x0000000100d09764 CleanupTempFiles + 160 5 postgres 0x0000000100d0f8c0 proc_exit_prepare + 228 6 postgres 0x0000000100d0f79c proc_exit + 24 7 postgres 0x0000000100e5ae04 errfinish + 856 8 postgres 0x0000000100f1a804 ProcessInterrupts.cold.9 + 88 9 postgres 0x0000000100d36a98 ProcessInterrupts + 604 10 postgres 0x0000000100cd93c4 sendDir + 1516 11 postgres 0x0000000100cd83a0 perform_base_backup + 3592 12 postgres 0x0000000100cd72c0 SendBaseBackup + 256 13 postgres 0x0000000100ce6a48 exec_replication_command + 1852 14 postgres 0x0000000100d39190 PostgresMain + 3260 15 postgres 0x0000000100ca9c78 process_startup_packet_die + 0 16 postgres 0x0000000100ca94ec ClosePostmasterPorts + 0 17 postgres 0x0000000100ca6a0c PostmasterMain + 4584 18 postgres 0x0000000100c0b798 help + 0 19 libdyld.dylib 0x0000000184391430 start + 4 regards, tom lane -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T03:06:52Z
Hi, On 2021-08-06 22:44:07 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > pgstat: Bring up pgstat in BaseInit() to fix uninitialized use of pgstat by AV. > > sifaka took exception to this, or at least I guess it was this one out > of the four you pushed at once: Longfin as well. It's the assertions from ee3f8d3d3ae, but possibly only exposed after fb2c5028e63. Not sure why it's your two animals that report this issue, but not others? Why would a backend doing SendBaseBackup() previously have allocated temp files? Don't get me wrong - it's good that they surfaced the issue, and it's an issue independent of the specific trigger. Glad I added the assertions... See also https://www.postgresql.org/message-id/20210803023612.iziacxk5syn2r4ut%40alap3.anarazel.de > TRAP: FailedAssertion("pgstat_is_initialized && !pgstat_is_shutdown", File: "pgstat.c", Line: 4810, PID: 74447) > 0 postgres 0x0000000100e5a520 ExceptionalCondition + 124 > 1 postgres 0x0000000100ca1dec pgstat_reset_counters + 0 > 2 postgres 0x0000000100ca2548 pgstat_report_tempfile + 80 > 3 postgres 0x0000000100d07ca4 FileClose + 536 > 4 postgres 0x0000000100d09764 CleanupTempFiles + 160 > 5 postgres 0x0000000100d0f8c0 proc_exit_prepare + 228 > 6 postgres 0x0000000100d0f79c proc_exit + 24 > 7 postgres 0x0000000100e5ae04 errfinish + 856 > 8 postgres 0x0000000100f1a804 ProcessInterrupts.cold.9 + 88 > 9 postgres 0x0000000100d36a98 ProcessInterrupts + 604 > 10 postgres 0x0000000100cd93c4 sendDir + 1516 > 11 postgres 0x0000000100cd83a0 perform_base_backup + 3592 > 12 postgres 0x0000000100cd72c0 SendBaseBackup + 256 > 13 postgres 0x0000000100ce6a48 exec_replication_command + 1852 > 14 postgres 0x0000000100d39190 PostgresMain + 3260 > 15 postgres 0x0000000100ca9c78 process_startup_packet_die + 0 > 16 postgres 0x0000000100ca94ec ClosePostmasterPorts + 0 > 17 postgres 0x0000000100ca6a0c PostmasterMain + 4584 > 18 postgres 0x0000000100c0b798 help + 0 > 19 libdyld.dylib 0x0000000184391430 start + 4 Greetings, Andres Freund -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T03:13:28Z
Andres Freund <andres@anarazel.de> writes: > Not sure why it's your two animals that report this issue, but not others? Why > would a backend doing SendBaseBackup() previously have allocated temp files? Guessing the common factor is "macOS", but that's just a guess. I can poke into it tomorrow. regards, tom lane
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T03:20:22Z
Hi, On 2021-08-06 23:13:28 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Not sure why it's your two animals that report this issue, but not others? Why > > would a backend doing SendBaseBackup() previously have allocated temp files? > > Guessing the common factor is "macOS", but that's just a guess. Probably not a bad one... > I can poke into it tomorrow. Thanks! Might be interesting to run the pg_basebackup tests with log_temp_files=0... Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T03:22:36Z
I wrote: > Guessing the common factor is "macOS", but that's just a guess. > I can poke into it tomorrow. I did try it real quick on my Mac laptop, and that fails too. Here's a more accurate backtrace, in case that helps. (lldb) bt * thread #1, stop reason = signal SIGSTOP * frame #0: 0x00007fff2033e92e libsystem_kernel.dylib`__pthread_kill + 10 frame #1: 0x00007fff2036d5bd libsystem_pthread.dylib`pthread_kill + 263 frame #2: 0x00007fff202c2406 libsystem_c.dylib`abort + 125 frame #3: 0x0000000108cdf87f postgres`ExceptionalCondition(conditionName=<unavailable>, errorType=<unavailable>, fileName=<unavailable>, lineNumber=<unavailable>) at assert.c:69:2 [opt] frame #4: 0x0000000108b0f551 postgres`pgstat_send [inlined] pgstat_assert_is_up at pgstat.c:4810:2 [opt] frame #5: 0x0000000108b0f532 postgres`pgstat_send(msg=<unavailable>, len=<unavailable>) at pgstat.c:3032 [opt] frame #6: 0x0000000108b0fbef postgres`pgstat_report_tempfile(filesize=<unavailable>) at pgstat.c:1812:2 [opt] frame #7: 0x0000000108b7abfe postgres`FileClose [inlined] ReportTemporaryFileUsage(path="base/pgsql_tmp/pgsql_tmp35840.0", size=0) at fd.c:1483:2 [opt] frame #8: 0x0000000108b7abf6 postgres`FileClose(file=1) at fd.c:1987 [opt] frame #9: 0x0000000108b7c3b8 postgres`CleanupTempFiles(isCommit=false, isProcExit=true) at fd.c:0 [opt] frame #10: 0x0000000108b82661 postgres`proc_exit_prepare(code=1) at ipc.c:209:3 [opt] frame #11: 0x0000000108b8253d postgres`proc_exit(code=1) at ipc.c:107:2 [opt] frame #12: 0x0000000108ce0201 postgres`errfinish(filename=<unavailable>, lineno=<unavailable>, funcname=<unavailable>) at elog.c:666:3 [opt] frame #13: 0x0000000108db362b postgres`ProcessInterrupts.cold.9 at postgres.c:3222:3 [opt] frame #14: 0x0000000108baa2a4 postgres`ProcessInterrupts at postgres.c:3218:22 [opt] frame #15: 0x0000000108b49f4a postgres`sendDir(path=".", basepathlen=1, sizeonly=false, tablespaces=0x00007fb47f0719c8, sendtblspclinks=true, manifest=<unavailable>, spcoid=0x0000000000000000) at basebackup.c:1277:3 [opt] frame #16: 0x0000000108b48dc8 postgres`perform_base_backup(opt=0x00007ffee73ae0a0) at basebackup.c:432:5 [opt] frame #17: 0x0000000108b47bf0 postgres`SendBaseBackup(cmd=<unavailable>) at basebackup.c:949:2 [opt] frame #18: 0x0000000108b58055 postgres`exec_replication_command(cmd_string="BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' ") at walsender.c:1625:4 [opt] frame #19: 0x0000000108bac8aa postgres`PostgresMain(argc=<unavailable>, argv=<unavailable>, dbname=<unavailable>, username=<unavailable>) at postgres.c:4484:12 [opt] frame #20: 0x0000000108b178db postgres`BackendRun(port=<unavailable>) at postmaster.c:4519:2 [opt] frame #21: 0x0000000108b17178 postgres`ServerLoop [inlined] BackendStartup(port=<unavailable>) at postmaster.c:4241:3 [opt] frame #22: 0x0000000108b17154 postgres`ServerLoop at postmaster.c:1758 [opt] frame #23: 0x0000000108b1421b postgres`PostmasterMain(argc=4, argv=0x00007fb47ec06640) at postmaster.c:1430:11 [opt] frame #24: 0x0000000108a6ac13 postgres`main(argc=<unavailable>, argv=<unavailable>) at main.c:199:3 [opt] frame #25: 0x00007fff20388f3d libdyld.dylib`start + 1 I see two core dumps with what seem to be this same trace after running pg_basebackup's tests. regards, tom lane -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T04:49:52Z
Hi, On 2021-08-06 23:22:36 -0400, Tom Lane wrote: > I wrote: > > Guessing the common factor is "macOS", but that's just a guess. > > I can poke into it tomorrow. > > I did try it real quick on my Mac laptop, and that fails too. > Here's a more accurate backtrace, in case that helps. Thanks! I now managed to reproduce it on a CI system (after some initial failed attempts due to pg_upgrade tests failing due to path length issues): https://api.cirrus-ci.com/v1/artifact/task/4863568911794176/log/src/bin/pg_basebackup/tmp_check/log/010_pg_basebackup_main.log 2021-08-06 21:18:40.096 PDT [22031] 010_pg_basebackup.pl LOG: received replication command: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 21:18:40.096 PDT [22031] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 21:18:42.210 PDT [22031] 010_pg_basebackup.pl LOG: could not send data to client: Broken pipe 2021-08-06 21:18:42.210 PDT [22031] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 21:18:42.210 PDT [22031] 010_pg_basebackup.pl FATAL: connection to client lost 2021-08-06 21:18:42.210 PDT [22031] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' TRAP: FailedAssertion("pgstat_is_initialized && !pgstat_is_shutdown", File: "pgstat.c", Line: 4810, PID: 22031) vs what I locally get: 2021-08-06 20:52:06.163 PDT [1397252] 010_pg_basebackup.pl LOG: received replication command: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 20:52:06.163 PDT [1397252] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 20:52:08.189 PDT [1397252] 010_pg_basebackup.pl LOG: could not send data to client: Broken pipe 2021-08-06 20:52:08.189 PDT [1397252] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 20:52:08.189 PDT [1397252] 010_pg_basebackup.pl ERROR: base backup could not send data, aborting backup 2021-08-06 20:52:08.189 PDT [1397252] 010_pg_basebackup.pl STATEMENT: BASE_BACKUP LABEL 'pg_basebackup base backup' PROGRESS NOWAIT MANIFEST 'yes' 2021-08-06 20:52:08.189 PDT [1397252] 010_pg_basebackup.pl FATAL: connection to client lost Note that OSX version doesn't have the "base backup could not send data, aborting backup" message. I guess that is what leads to the difference in behaviour: The temp file is created by InitializeBackupManifest(). In the !OSX case, we first abort via an ERROR, which triggers the cleanup via WalSndResourceCleanup(). On OSX however, we immediately error out with FATAL for some reason (timing? network buffering differences?), which will never reach WalSndErrorCleanup(). Therefore the temp file only gets deleted during proc_exit(), which triggers the issue... Not yet really sure what the best way to deal with this is. Presumably this issue would be fixed if AtProcExit_Files()/CleanupTempFiles() were scheduled via before_shmem_exit(). And perhaps it's not too off to schedule CleanupTempFiles() there - but it doesn't quite seem entirely right either. I'd kinda like to avoid having to overhaul the process exit infrastructure as a prerequisite to getting the shared memory stats patch in :(. Greetings, Andres Freund -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T05:11:49Z
Hi, On 2021-08-06 21:49:52 -0700, Andres Freund wrote: > The temp file is created by InitializeBackupManifest(). In the !OSX case, we > first abort via an ERROR, which triggers the cleanup via > WalSndResourceCleanup(). On OSX however, we immediately error out with FATAL > for some reason (timing? network buffering differences?), which will never > reach WalSndErrorCleanup(). Therefore the temp file only gets deleted during > proc_exit(), which triggers the issue... > > Not yet really sure what the best way to deal with this is. Presumably this > issue would be fixed if AtProcExit_Files()/CleanupTempFiles() were scheduled > via before_shmem_exit(). And perhaps it's not too off to schedule > CleanupTempFiles() there - but it doesn't quite seem entirely right either. Huh. I just noticed that AtProcExit_Files() is not even scheduled via on_shmem_exit() but on_proc_exit(). That means that even before fb2c5028e63 we sent pgstat messages *well* after pgstat_shutdown_hook() already ran. Crufty. Just hacking in an earlier CleanupTempFiles() does "fix" the OSX issue: https://cirrus-ci.com/task/5941265494704128?logs=macos_basebackup#L4 I'm inclined to leave things as-is until tomorrow to see if other things are shaken loose and then either commit a bandaid along those lines or revert the patch. Or something proper if we can figure it out till then. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T15:43:07Z
Andres Freund <andres@anarazel.de> writes: > On 2021-08-06 21:49:52 -0700, Andres Freund wrote: >> Not yet really sure what the best way to deal with this is. Presumably this >> issue would be fixed if AtProcExit_Files()/CleanupTempFiles() were scheduled >> via before_shmem_exit(). And perhaps it's not too off to schedule >> CleanupTempFiles() there - but it doesn't quite seem entirely right either. > Huh. I just noticed that AtProcExit_Files() is not even scheduled via > on_shmem_exit() but on_proc_exit(). That means that even before fb2c5028e63 > we sent pgstat messages *well* after pgstat_shutdown_hook() already > ran. Crufty. > Just hacking in an earlier CleanupTempFiles() does "fix" the OSX issue: > https://cirrus-ci.com/task/5941265494704128?logs=macos_basebackup#L4 So if I have the lay of the land correctly: 1. Somebody decided it'd be a great idea for temp file cleanup to send stats collector messages. 2. Temp file cleanup happens after shmem disconnection. 3. This accidentally worked, up to now, because stats transmission happens via a separate socket not shared memory. 4. We can't keep both #1 and #2 if we'd like to switch to shmem-based stats collection. Intuitively it seems like temp file management should be a low-level, backend-local function and therefore should be okay to run after shmem disconnection. I do not have a warm feeling about reversing that module layering --- what's to stop someone from breaking things by trying to use a temp file in their on_proc_exit or on_shmem_exit hook? Maybe what needs to go overboard is point 1. More generally, this points up the fact that we don't have a well-defined module hierarchy that would help us understand what code can safely do which. I'm not volunteering to design that, but maybe it needs to happen soon. regards, tom lane
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T16:48:50Z
Hi, On 2021-08-07 11:43:07 -0400, Tom Lane wrote: > So if I have the lay of the land correctly: > > 1. Somebody decided it'd be a great idea for temp file cleanup to send > stats collector messages. > > 2. Temp file cleanup happens after shmem disconnection. > > 3. This accidentally worked, up to now, because stats transmission happens > via a separate socket not shared memory. > > 4. We can't keep both #1 and #2 if we'd like to switch to shmem-based > stats collection. Sounds accurate to me. > Intuitively it seems like temp file management should be a low-level, > backend-local function and therefore should be okay to run after > shmem disconnection. I do not have a warm feeling about reversing that > module layering --- what's to stop someone from breaking things by > trying to use a temp file in their on_proc_exit or on_shmem_exit hook? We could just add an assert preventing that from happening. It's hard to believe that there could be a good reason to use temp files in those hook points. I'm somewhat inclined to split InitFileAccess() into two by separating out InitTemporaryFileAccess() or such. InitFileAccess() would continue to happen early and register a proc exit hook that errors out when there's a temp file (as a backstop for non cassert builds). The new InitTemporaryFileAccess() would happen a bit later and schedule CleanupTempFiles() to happen via before_shmem_access(). And we add a Assert(!proc_exit_inprogress) to the routines for opening a temp file. > Maybe what needs to go overboard is point 1. Keeping stats of temp files seems useful enough that I'm a bit hesitant to go there. I guess we could just prevent pgstats_report_tempfile() from being called when CleanupTempFiles() is called during proc exit, but that doesn't seem great. > More generally, this points up the fact that we don't have a well-defined > module hierarchy that would help us understand what code can safely do which. > I'm not volunteering to design that, but maybe it needs to happen soon. I agree. Part of the reason for whacking around process startup (in both pushed and still pending commits) was that previously it wasn't just poorly defined, it differed significantly between platforms. And I'm quite unhappy with the vagueness in which we defined the meaning of the various shutdown callbacks ([1]). I suspect to even get to the point of doing a useful redesign of the module hierarchy, we'd need to unify more of the process initialization between EXEC_BACKEND and normal builds. I've bitten by all this often enough to be motivated to propose something. However I want to get the basics of the shared memory stats stuff in first - it's a pain to keep it upated, and we'll need to find and solve all of the issues it has anyway, even if we go for a redesign of module / startup / shutdown layering. Greetings, Andres Freund [1] https://www.postgresql.org/message-id/20210803023612.iziacxk5syn2r4ut%40alap3.anarazel.de
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T17:06:47Z
Andres Freund <andres@anarazel.de> writes: > On 2021-08-07 11:43:07 -0400, Tom Lane wrote: >> Intuitively it seems like temp file management should be a low-level, >> backend-local function and therefore should be okay to run after >> shmem disconnection. I do not have a warm feeling about reversing that >> module layering --- what's to stop someone from breaking things by >> trying to use a temp file in their on_proc_exit or on_shmem_exit hook? > We could just add an assert preventing that from happening. It's hard to > believe that there could be a good reason to use temp files in those hook > points. The bigger picture here is that anyplace anybody ever wants to add stats collection in suddenly becomes a "must run before shmem disconnection" module. I think that way madness lies --- we can't have the entire backend shut down before shmem disconnection. So I feel like there's a serious design problem here, and it's not confined to temp files, or at least it won't stay confined to temp files. (There may indeed be other problems already, that we just haven't had the good luck to have buildfarm timing vagaries expose to us.) Maybe the solution is to acknowledge that we might lose some events during backend shutdown, and redefine the behavior as "we ignore event reports after pgstat shutdown", not "we assert that there never can be any such reports". > I'm somewhat inclined to split InitFileAccess() into two by separating out > InitTemporaryFileAccess() or such. InitFileAccess() would continue to happen > early and register a proc exit hook that errors out when there's a temp file > (as a backstop for non cassert builds). The new InitTemporaryFileAccess() > would happen a bit later and schedule CleanupTempFiles() to happen via > before_shmem_access(). And we add a Assert(!proc_exit_inprogress) to the > routines for opening a temp file. Maybe that would work, but after you multiply it by a bunch of different scenarios in different modules, it's going to get less and less attractive. > I've bitten by all this often enough to be motivated to propose > something. However I want to get the basics of the shared memory stats stuff > in first - it's a pain to keep it upated, and we'll need to find and solve all > of the issues it has anyway, even if we go for a redesign of module / startup > / shutdown layering. Fair. But I suggest that the first cut should look more like what I suggest above, ie just be willing to lose events during shutdown. The downsides of that are not so enormous that we should be willing to undertake major klugery to avoid it before we've even got a semi-working system. regards, tom lane
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T17:25:56Z
Hi, On 2021-08-07 13:06:47 -0400, Tom Lane wrote: > The bigger picture here is that anyplace anybody ever wants to add stats > collection in suddenly becomes a "must run before shmem disconnection" > module. I think that way madness lies --- we can't have the entire > backend shut down before shmem disconnection. So I feel like there's > a serious design problem here, and it's not confined to temp files, > or at least it won't stay confined to temp files. (There may indeed > be other problems already, that we just haven't had the good luck to > have buildfarm timing vagaries expose to us.) I think more often it should not end up as "must run before shmem disconnection" as a whole, but should be split into a portion running at that point. > Maybe the solution is to acknowledge that we might lose some events > during backend shutdown, and redefine the behavior as "we ignore > event reports after pgstat shutdown", not "we assert that there never > can be any such reports". I think it's fine to make such calls, but that it ought to reside in the stats emitting modules. Only it can decide whether needing to emit stats during shutdown is a rare edge case or a commonly expected path. E.g. the case of parallel worker shutdown sending stats too late is something common enough to be problematic, so I don't want to make it too hard to detect such cases. > > I'm somewhat inclined to split InitFileAccess() into two by separating out > > InitTemporaryFileAccess() or such. InitFileAccess() would continue to happen > > early and register a proc exit hook that errors out when there's a temp file > > (as a backstop for non cassert builds). The new InitTemporaryFileAccess() > > would happen a bit later and schedule CleanupTempFiles() to happen via > > before_shmem_access(). And we add a Assert(!proc_exit_inprogress) to the > > routines for opening a temp file. > > Maybe that would work, but after you multiply it by a bunch of different > scenarios in different modules, it's going to get less and less attractive. I'm not quite convinced. Even if we had a nicer ordering / layering of subsystems, we'd still have to deal with subsystems that don't fit nicely because they have conflicting needs. And we'd still need detection of use of subsystems that already have not been initialized / are already shut down. One example of that is imo fd.c being a very low level module that might be needed during shutdown of other modules but which also currently depends on the stats subsystem for temp file management. Which doesn't really make sense, because stats very well could depend on fd.c routines. I think needing to split the current fd.c subsystem into a lower-level (file access) and a higher level (temporary file management) module is precisely what a better designed module layering system will *force* us to do. > > I've bitten by all this often enough to be motivated to propose > > something. However I want to get the basics of the shared memory stats stuff > > in first - it's a pain to keep it upated, and we'll need to find and solve all > > of the issues it has anyway, even if we go for a redesign of module / startup > > / shutdown layering. > > Fair. But I suggest that the first cut should look more like what > I suggest above, ie just be willing to lose events during shutdown. > The downsides of that are not so enormous that we should be willing > to undertake major klugery to avoid it before we've even got a > semi-working system. I think that's more likely to hide bugs unfortunately. Consider fa91d4c91f2 - I might not have found that if we had just ignored "too late" pgstats activity in pgstats.c or fd.c, and that's not an edge case. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T17:37:16Z
Andres Freund <andres@anarazel.de> writes: > On 2021-08-07 13:06:47 -0400, Tom Lane wrote: >> Fair. But I suggest that the first cut should look more like what >> I suggest above, ie just be willing to lose events during shutdown. >> The downsides of that are not so enormous that we should be willing >> to undertake major klugery to avoid it before we've even got a >> semi-working system. > I think that's more likely to hide bugs unfortunately. Consider fa91d4c91f2 - > I might not have found that if we had just ignored "too late" pgstats activity > in pgstats.c or fd.c, and that's not an edge case. Depends what you want to define as a bug. What I am not happy about is the prospect of random assertion failures for the next six months while you finish redesigning half of the system. The rest of us have work we want to get done, too. I don't object to the idea of making no-lost-events an end goal, but we are clearly not ready for that today. regards, tom lane
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T18:51:41Z
Hi, On 2021-08-07 13:37:16 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2021-08-07 13:06:47 -0400, Tom Lane wrote: > >> Fair. But I suggest that the first cut should look more like what > >> I suggest above, ie just be willing to lose events during shutdown. > >> The downsides of that are not so enormous that we should be willing > >> to undertake major klugery to avoid it before we've even got a > >> semi-working system. > > > I think that's more likely to hide bugs unfortunately. Consider fa91d4c91f2 - > > I might not have found that if we had just ignored "too late" pgstats activity > > in pgstats.c or fd.c, and that's not an edge case. > > Depends what you want to define as a bug. What I am not happy about > is the prospect of random assertion failures for the next six months > while you finish redesigning half of the system. The rest of us > have work we want to get done, too. I don't object to the idea of > making no-lost-events an end goal, but we are clearly not ready > for that today. I don't know what to do about that. How would we even find these cases if they aren't hit during regression tests on my machine (nor on a lot of others)? Obviously I had ran the regression tests many times before pushing the earlier changes. The check for pgstat being up is in one central place and thus easily can be turned into a warning if problems around the shutdown sequence become a frequent issue on HEAD. If you think it's better to turn that into a WARNING now and then arm into an assert later, I can live with that as well, but I don't think it'll lead to a better outcome. The shared memory stats stuff isn't my personal project - it's Horiguchi-san's. I picked it up because it seems like an important thing to address and because it'd been in maybe a dozen CFs without a lot of progress. It just turns out that there's a lot of prerequisite changes :(. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T19:01:31Z
Hi, On 2021-08-07 09:48:50 -0700, Andres Freund wrote: > I'm somewhat inclined to split InitFileAccess() into two by separating out > InitTemporaryFileAccess() or such. InitFileAccess() would continue to happen > early and register a proc exit hook that errors out when there's a temp file > (as a backstop for non cassert builds). The new InitTemporaryFileAccess() > would happen a bit later and schedule CleanupTempFiles() to happen via > before_shmem_access(). And we add a Assert(!proc_exit_inprogress) to the > routines for opening a temp file. Attached is a patch showing how this could look like. Note that the PANIC should likely not be that but a WARNING, but the PANIC more useful for running some initial tests... I'm not sure whether we'd want to continue having the proc exit hook? It seems to me that asserts would provide a decent enough protection against introducing new temp files during shutdown. Alternatively we could make the asserts in OpenTemporaryFile et al elog(ERROR)s, and be pretty certain that no temp files would be open too late? Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-07T19:12:38Z
Andres Freund <andres@anarazel.de> writes: > On 2021-08-07 13:37:16 -0400, Tom Lane wrote: >> Depends what you want to define as a bug. What I am not happy about >> is the prospect of random assertion failures for the next six months >> while you finish redesigning half of the system. The rest of us >> have work we want to get done, too. I don't object to the idea of >> making no-lost-events an end goal, but we are clearly not ready >> for that today. > I don't know what to do about that. How would we even find these cases if they > aren't hit during regression tests on my machine (nor on a lot of others)? The regression tests really aren't that helpful for testing the problem scenario here, which basically is SIGTERM'ing a query-in-progress. I'm rather surprised that the buildfarm managed to exercise that at all. You might try setting up a test scaffold that runs the core regression tests and SIGINT's the postmaster, or alternatively SIGTERM's some individual session, at random times partway through. Obviously this will make the regression tests report failure, but what to look for is if anything dumps core on the way out. regards, tom lane
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-07T21:03:49Z
Hi, On 2021-08-07 15:12:38 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2021-08-07 13:37:16 -0400, Tom Lane wrote: > >> Depends what you want to define as a bug. What I am not happy about > >> is the prospect of random assertion failures for the next six months > >> while you finish redesigning half of the system. The rest of us > >> have work we want to get done, too. I don't object to the idea of > >> making no-lost-events an end goal, but we are clearly not ready > >> for that today. > > > I don't know what to do about that. How would we even find these cases if they > > aren't hit during regression tests on my machine (nor on a lot of others)? > > The regression tests really aren't that helpful for testing the problem > scenario here, which basically is SIGTERM'ing a query-in-progress. > I'm rather surprised that the buildfarm managed to exercise that at all. They're also not that helpful because this problem likely is unreachable for any tempfiles other than the one in InitializeBackupManifest(). Pretty much all, or even all, the other tempfiles are cleaned up either via transaction and/or resowner cleanup. I wonder if we should do something about WalSndResourceCleanup() not being reached for FATALs? I think at least a note in WalSndResourceCleanup() commenting on that fact seems like it might be a good idea? It seems like it could eventually be a problem that the resowners added in 0d8c9c1210c4 aren't ever cleaned up in case of a FATAL error. Most resowner cleanup actions are also backstopped with some form of on-exit hook, but I don't think it's all - e.g. buffer pins aren't. I guess I should start a thread about this on -hackers... > You might try setting up a test scaffold that runs the core regression > tests and SIGINT's the postmaster, or alternatively SIGTERM's some > individual session, at random times partway through. Obviously this > will make the regression tests report failure, but what to look for > is if anything dumps core on the way out. Worth trying. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-08T02:24:35Z
Hi, On 2021-08-07 12:01:31 -0700, Andres Freund wrote: > Attached is a patch showing how this could look like. Note that the PANIC > should likely not be that but a WARNING, but the PANIC more useful for running > some initial tests... I pushed a slightly evolved version of this. As the commit message noted, this may not be the best approach, but we can revise after further discussion. > I'm not sure whether we'd want to continue having the proc exit hook? It seems > to me that asserts would provide a decent enough protection against > introducing new temp files during shutdown. > Alternatively we could make the asserts in OpenTemporaryFile et al > elog(ERROR)s, and be pretty certain that no temp files would be open too late? I ended up removing the proc exit hook and not converting the asserts to an elog(). Happy to change either. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Robert Haas <robertmhaas@gmail.com> — 2021-08-09T15:46:30Z
On Sat, Aug 7, 2021 at 11:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Intuitively it seems like temp file management should be a low-level, > backend-local function and therefore should be okay to run after > shmem disconnection. I do not have a warm feeling about reversing that > module layering --- what's to stop someone from breaking things by > trying to use a temp file in their on_proc_exit or on_shmem_exit hook? > Maybe what needs to go overboard is point 1. > > More generally, this points up the fact that we don't have a well-defined > module hierarchy that would help us understand what code can safely do which. > I'm not volunteering to design that, but maybe it needs to happen soon. Yeah, I was quite surprised when I saw this commit, because my first reaction was - why in the world would temporary file shutdown properly precede DSM shutdown, given that temporary files are a low-level mechanism? The explanation that we're trying to send statistics at that point makes sense as far as it goes, but it seems to me that we cannot be far from having a circular dependency. All we need is to have DSM require the use of temporary files, and we'll end up needing DSM shutdown to happen both before and after temporary file cleanup. /me wonders idly about dynamic_shared_memory_type=file I think that subsystems like "memory" and "files" really ought to be the lowest-level things we have, and should be shut down last. Stuff like "send a message to the stats collector" seems like a higher level thing that may require those lower-level facilities in order to operate, and must therefore be shut down first. Maybe some subsystems need to be divided into upper and lower levels to make this work, or, well, I don't know, something else. But I'm deeply suspicious that lifting stuff like this to the front of the shutdown sequence is just papering over the problem, and not actually solving it. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-09T17:34:53Z
Hi, On 2021-08-09 11:46:30 -0400, Robert Haas wrote: > I think that subsystems like "memory" and "files" really ought to be > the lowest-level things we have, and should be shut down last. I don't disagree with that - but there's a difference between having that as an abstract goal, and having it a dependency of committing somebodies patch. > Stuff like "send a message to the stats collector" seems like a higher level > thing that may require those lower-level facilities in order to operate, and > must therefore be shut down first. Yep. > Maybe some subsystems need to be divided into upper and lower levels to make > this work, or, well, I don't know, something else. That's what I ended up doing, right? There's now InitFileAccess() and InitTemporaryFileAccess(). > But I'm deeply suspicious that lifting stuff like this to the front of the > shutdown sequence is just papering over the problem, and not actually > solving it. If you have a concrete proposal that you think makes sense to tie shared memory stats to, I'm happy to entertain it. One main motivator for b406478b87e etc was to allow rejiggering things like this more easily. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-10T11:06:45Z
Hi, On Sun, Aug 8, 2021 at 11:24 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-08-07 12:01:31 -0700, Andres Freund wrote: > > Attached is a patch showing how this could look like. Note that the PANIC > > should likely not be that but a WARNING, but the PANIC more useful for running > > some initial tests... > > I pushed a slightly evolved version of this. As the commit message noted, this > may not be the best approach, but we can revise after further discussion. While testing streaming logical replication, I got another assertion failure with the current HEAD (e2ce88b58f) when the apply worker raised an error during applying spooled changes: TRAP: FailedAssertion("pgstat_is_initialized && !pgstat_is_shutdown", File: "pgstat.c", Line: 4810, PID: 11084) 0 postgres 0x000000010704fd9a ExceptionalCondition + 234 1 postgres 0x0000000106d41e62 pgstat_assert_is_up + 66 2 postgres 0x0000000106d43854 pgstat_send + 20 3 postgres 0x0000000106d4433e pgstat_report_tempfile + 94 4 postgres 0x0000000106df9519 ReportTemporaryFileUsage + 25 5 postgres 0x0000000106df945a PathNameDeleteTemporaryFile + 282 6 postgres 0x0000000106df8c7e unlink_if_exists_fname + 174 7 postgres 0x0000000106df8b3a walkdir + 426 8 postgres 0x0000000106df8982 PathNameDeleteTemporaryDir + 82 9 postgres 0x0000000106dfe591 SharedFileSetDeleteAll + 113 10 postgres 0x0000000106dfdf62 SharedFileSetDeleteOnProcExit + 66 11 postgres 0x0000000106e05275 proc_exit_prepare + 325 12 postgres 0x0000000106e050a3 proc_exit + 19 13 postgres 0x0000000106d3ba99 StartBackgroundWorker + 649 14 postgres 0x0000000106d54e85 do_start_bgworker + 613 15 postgres 0x0000000106d4ef26 maybe_start_bgworkers + 486 16 postgres 0x0000000106d4d767 sigusr1_handler + 631 17 libsystem_platform.dylib 0x00007fff736705fd _sigtramp + 29 18 ??? 0x0000000000000000 0x0 + 0 19 postgres 0x0000000106d4c990 PostmasterMain + 6640 20 postgres 0x0000000106c24fa3 main + 819 21 libdyld.dylib 0x00007fff73477cc9 start + 1 The apply worker registers SharedFileSetDeleteOnProcExit() when creating a file set to serialize the changes. When it raises an error due to conflict during applying the change, the callback eventually reports the temp file statistics but pgstat already shut down, resulting in this assertion failure. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-11T13:47:07Z
On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > The apply worker registers SharedFileSetDeleteOnProcExit() when > creating a file set to serialize the changes. When it raises an error > due to conflict during applying the change, the callback eventually > reports the temp file statistics but pgstat already shut down, > resulting in this assertion failure. > I think we can try to fix this by registering to clean up these files via before_shmem_exit() as done by Andres in commit 675c945394. Similar to that commit, we can change the function name SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and register it via before_shmem_exit() instead of on_proc_exit(). Can you try that and see if it fixes the issue for you unless you have better ideas to try out? -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-12T02:09:10Z
On Wed, Aug 11, 2021 at 10:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > The apply worker registers SharedFileSetDeleteOnProcExit() when > > creating a file set to serialize the changes. When it raises an error > > due to conflict during applying the change, the callback eventually > > reports the temp file statistics but pgstat already shut down, > > resulting in this assertion failure. > > > > I think we can try to fix this by registering to clean up these files > via before_shmem_exit() as done by Andres in commit 675c945394. > Similar to that commit, we can change the function name > SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and > register it via before_shmem_exit() instead of on_proc_exit(). Can you > try that and see if it fixes the issue for you unless you have better > ideas to try out? It seems to me that moving the shared fileset cleanup to before_shmem_exit() is the right approach to fix this problem. The issue is fixed by the attached patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-12T06:08:03Z
On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Wed, Aug 11, 2021 at 10:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > The apply worker registers SharedFileSetDeleteOnProcExit() when > > > creating a file set to serialize the changes. When it raises an error > > > due to conflict during applying the change, the callback eventually > > > reports the temp file statistics but pgstat already shut down, > > > resulting in this assertion failure. > > > > > > > I think we can try to fix this by registering to clean up these files > > via before_shmem_exit() as done by Andres in commit 675c945394. > > Similar to that commit, we can change the function name > > SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and > > register it via before_shmem_exit() instead of on_proc_exit(). Can you > > try that and see if it fixes the issue for you unless you have better > > ideas to try out? > > It seems to me that moving the shared fileset cleanup to > before_shmem_exit() is the right approach to fix this problem. The > issue is fixed by the attached patch. +1, the fix makes sense to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-12T06:16:09Z
On Thu, Aug 12, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Wed, Aug 11, 2021 at 10:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > The apply worker registers SharedFileSetDeleteOnProcExit() when > > > > creating a file set to serialize the changes. When it raises an error > > > > due to conflict during applying the change, the callback eventually > > > > reports the temp file statistics but pgstat already shut down, > > > > resulting in this assertion failure. > > > > > > > > > > I think we can try to fix this by registering to clean up these files > > > via before_shmem_exit() as done by Andres in commit 675c945394. > > > Similar to that commit, we can change the function name > > > SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and > > > register it via before_shmem_exit() instead of on_proc_exit(). Can you > > > try that and see if it fixes the issue for you unless you have better > > > ideas to try out? > > > > It seems to me that moving the shared fileset cleanup to > > before_shmem_exit() is the right approach to fix this problem. The > > issue is fixed by the attached patch. > > +1, the fix makes sense to me. > I have also tested and fix works for me. The fix works because pgstat_initialize() is called before we register clean up in SharedFileSetInit(). I am not sure if we need an Assert to ensure that and if so how we can do that? Any suggestions? -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-12T07:43:11Z
On Thu, Aug 12, 2021 at 3:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > On Wed, Aug 11, 2021 at 10:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > The apply worker registers SharedFileSetDeleteOnProcExit() when > > > > > creating a file set to serialize the changes. When it raises an error > > > > > due to conflict during applying the change, the callback eventually > > > > > reports the temp file statistics but pgstat already shut down, > > > > > resulting in this assertion failure. > > > > > > > > > > > > > I think we can try to fix this by registering to clean up these files > > > > via before_shmem_exit() as done by Andres in commit 675c945394. > > > > Similar to that commit, we can change the function name > > > > SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and > > > > register it via before_shmem_exit() instead of on_proc_exit(). Can you > > > > try that and see if it fixes the issue for you unless you have better > > > > ideas to try out? > > > > > > It seems to me that moving the shared fileset cleanup to > > > before_shmem_exit() is the right approach to fix this problem. The > > > issue is fixed by the attached patch. > > > > +1, the fix makes sense to me. > > > > I have also tested and fix works for me. The fix works because > pgstat_initialize() is called before we register clean up in > SharedFileSetInit(). I am not sure if we need an Assert to ensure that > and if so how we can do that? Any suggestions? I think that the assertion added by ee3f8d3d3ae ensures that pgstat_initialize() is callbed before the callback for fileset cleanup is registered, no? Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-12T08:12:24Z
On Thu, Aug 12, 2021 at 1:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 3:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Aug 12, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > On Wed, Aug 11, 2021 at 10:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > > > > > On Tue, Aug 10, 2021 at 4:37 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > > > > > > > The apply worker registers SharedFileSetDeleteOnProcExit() when > > > > > > creating a file set to serialize the changes. When it raises an error > > > > > > due to conflict during applying the change, the callback eventually > > > > > > reports the temp file statistics but pgstat already shut down, > > > > > > resulting in this assertion failure. > > > > > > > > > > > > > > > > I think we can try to fix this by registering to clean up these files > > > > > via before_shmem_exit() as done by Andres in commit 675c945394. > > > > > Similar to that commit, we can change the function name > > > > > SharedFileSetDeleteOnProcExit to SharedFileSetDeleteOnShmExit and > > > > > register it via before_shmem_exit() instead of on_proc_exit(). Can you > > > > > try that and see if it fixes the issue for you unless you have better > > > > > ideas to try out? > > > > > > > > It seems to me that moving the shared fileset cleanup to > > > > before_shmem_exit() is the right approach to fix this problem. The > > > > issue is fixed by the attached patch. > > > > > > +1, the fix makes sense to me. > > > > > > > I have also tested and fix works for me. The fix works because > > pgstat_initialize() is called before we register clean up in > > SharedFileSetInit(). I am not sure if we need an Assert to ensure that > > and if so how we can do that? Any suggestions? > > I think that the assertion added by ee3f8d3d3ae ensures that > pgstat_initialize() is callbed before the callback for fileset cleanup > is registered, no? Right, it ensures that callback for fileset, is called after pgstat_initialize() and before pgstat_shutdown. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-12T08:18:23Z
On Thu, Aug 12, 2021 at 1:13 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 3:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > I have also tested and fix works for me. The fix works because > > pgstat_initialize() is called before we register clean up in > > SharedFileSetInit(). I am not sure if we need an Assert to ensure that > > and if so how we can do that? Any suggestions? > > I think that the assertion added by ee3f8d3d3ae ensures that > pgstat_initialize() is callbed before the callback for fileset cleanup > is registered, no? > I think I am missing something here, can you please explain? -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-12T08:22:37Z
Hi, On 2021-08-12 11:46:09 +0530, Amit Kapila wrote: > On Thu, Aug 12, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > It seems to me that moving the shared fileset cleanup to > > > before_shmem_exit() is the right approach to fix this problem. The > > > issue is fixed by the attached patch. > > > > +1, the fix makes sense to me. I'm not so sure. Why does sharedfileset have its own proc exit hook in the first place? ISTM that this should be dealt with using resowners, rathers than a sharedfileset specific mechanism? That said, I think it's fine to go for the ordering change in the short term. > I have also tested and fix works for me. The fix works because > pgstat_initialize() is called before we register clean up in > SharedFileSetInit(). I am not sure if we need an Assert to ensure that > and if so how we can do that? Any suggestions? I don't think we need to assert that - we'd see failures soon enough if that rule were violated... Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-12T09:36:23Z
On Thu, Aug 12, 2021 at 1:52 PM Andres Freund <andres@anarazel.de> wrote: > > On 2021-08-12 11:46:09 +0530, Amit Kapila wrote: > > On Thu, Aug 12, 2021 at 11:38 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > On Thu, Aug 12, 2021 at 7:39 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > It seems to me that moving the shared fileset cleanup to > > > > before_shmem_exit() is the right approach to fix this problem. The > > > > issue is fixed by the attached patch. > > > > > > +1, the fix makes sense to me. > > I'm not so sure. Why does sharedfileset have its own proc exit hook in the > first place? ISTM that this should be dealt with using resowners, rathers than > a sharedfileset specific mechanism? > The underlying temporary files need to be closed at xact end but need to survive across transactions. These are registered with the resource owner via PathNameOpenTemporaryFile/PathNameCreateTemporaryFile and then closed at xact end. So, we need a way to remove the files used by the process (apply worker in this particular case) before process exit and used this proc_exit hook (possibly on the lines of AtProcExit_Files). > That said, I think it's fine to go for the ordering change in the short term. > > > > I have also tested and fix works for me. The fix works because > > pgstat_initialize() is called before we register clean up in > > SharedFileSetInit(). I am not sure if we need an Assert to ensure that > > and if so how we can do that? Any suggestions? > > I don't think we need to assert that - we'd see failures soon enough if > that rule were violated... > Fair enough. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-12T12:48:19Z
Hi, On 2021-08-12 15:06:23 +0530, Amit Kapila wrote: > On Thu, Aug 12, 2021 at 1:52 PM Andres Freund <andres@anarazel.de> wrote: > > I'm not so sure. Why does sharedfileset have its own proc exit hook in the > > first place? ISTM that this should be dealt with using resowners, rathers than > > a sharedfileset specific mechanism? > > > The underlying temporary files need to be closed at xact end but need > to survive across transactions. Why do they need to be closed at xact end? To avoid wasting memory due to too many buffered files? > These are registered with the resource owner via > PathNameOpenTemporaryFile/PathNameCreateTemporaryFile and then closed > at xact end. So, we need a way to remove the files used by the process > (apply worker in this particular case) before process exit and used > this proc_exit hook (possibly on the lines of AtProcExit_Files). What I'm wondering is why it is a good idea to have a SharedFileSet specific cleanup mechanism. One that only operates on process lifetime level, rather than something more granular. I get that the of the files here needs to be longer than a transaction, but that can easily be addressed by having a longer lived resource owner. Process lifetime may work well for the current worker.c, but even there it doesn't seem optimal. One e.g. could easily imagine that we'd want to handle connection errors or configuration changes without restarting the worker, in which case process lifetime obviously isn't a good idea anymore. I think SharedFileSetInit() needs a comment explaining that it needs to be called in a process-lifetime memory context if used without dsm segments. Because otherwise SharedFileSetDeleteOnProcExit() will access already freed memory (both for filesetlist and the SharedFileSet itself). Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-12T12:54:21Z
Hi, On 2021-08-12 05:48:19 -0700, Andres Freund wrote: > I think SharedFileSetInit() needs a comment explaining that it needs to be > called in a process-lifetime memory context if used without dsm > segments. Because otherwise SharedFileSetDeleteOnProcExit() will access > already freed memory (both for filesetlist and the SharedFileSet itself). Oh. And I think it's not ok that SharedFileSetDeleteAll() unconditionally does SharedFileSetUnregister(). SharedFileSetUnregister() asserts out if there's no match, but DSM based sets are never entered into filesetlist. So one cannot have a non-DSM and DSM set coexisting. Which seems surprising. Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-13T09:08:37Z
On Thu, Aug 12, 2021 at 6:18 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-08-12 15:06:23 +0530, Amit Kapila wrote: > > On Thu, Aug 12, 2021 at 1:52 PM Andres Freund <andres@anarazel.de> wrote: > > > I'm not so sure. Why does sharedfileset have its own proc exit hook in the > > > first place? ISTM that this should be dealt with using resowners, rathers than > > > a sharedfileset specific mechanism? > > > > > > The underlying temporary files need to be closed at xact end but need > > to survive across transactions. > > Why do they need to be closed at xact end? To avoid wasting memory due to too > many buffered files? > Yes. > > > These are registered with the resource owner via > > PathNameOpenTemporaryFile/PathNameCreateTemporaryFile and then closed > > at xact end. So, we need a way to remove the files used by the process > > (apply worker in this particular case) before process exit and used > > this proc_exit hook (possibly on the lines of AtProcExit_Files). > > What I'm wondering is why it is a good idea to have a SharedFileSet specific > cleanup mechanism. One that only operates on process lifetime level, rather > than something more granular. I get that the of the files here needs to be > longer than a transaction, but that can easily be addressed by having a longer > lived resource owner. > > Process lifetime may work well for the current worker.c, but even there it > doesn't seem optimal. One e.g. could easily imagine that we'd want to handle > connection errors or configuration changes without restarting the worker, in > which case process lifetime obviously isn't a good idea anymore. > I don't deny that we can't make this at a more granular level. IIRC, at that time, we tried to follow AtProcExit_Files which cleans up temp files at proc exit and we needed something similar for temporary files used via SharedFileSet. I think we can extend this API but I guess it is better to then do it for dsm-based as well so that these get tracked via resowner. > > I think SharedFileSetInit() needs a comment explaining that it needs to be > called in a process-lifetime memory context if used without dsm > segments. > We already have a comment about proc_exit clean up of files but will extend that a bit about memory context. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-13T09:09:35Z
On Thu, Aug 12, 2021 at 6:24 PM Andres Freund <andres@anarazel.de> wrote: > > On 2021-08-12 05:48:19 -0700, Andres Freund wrote: > > I think SharedFileSetInit() needs a comment explaining that it needs to be > > called in a process-lifetime memory context if used without dsm > > segments. Because otherwise SharedFileSetDeleteOnProcExit() will access > > already freed memory (both for filesetlist and the SharedFileSet itself). > > Oh. And I think it's not ok that SharedFileSetDeleteAll() unconditionally does > SharedFileSetUnregister(). SharedFileSetUnregister() asserts out if there's no > match, but DSM based sets are never entered into filesetlist. So one cannot > have a non-DSM and DSM set coexisting. Which seems surprising. > Oops, it should be allowed to have both non-DSM and DSM set coexisting. I think we can remove Assert from SharedFileSetUnregister(). The other way could be to pass a parameter to SharedFileSetDeleteAll() to tell whether to unregister or not. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-13T15:59:40Z
Hi, (dropping -committers to avoid moderation stalls due xposting to multiple lists - I find that more annoying than helpful) On 2021-08-13 14:38:37 +0530, Amit Kapila wrote: > > What I'm wondering is why it is a good idea to have a SharedFileSet specific > > cleanup mechanism. One that only operates on process lifetime level, rather > > than something more granular. I get that the of the files here needs to be > > longer than a transaction, but that can easily be addressed by having a longer > > lived resource owner. > > > > Process lifetime may work well for the current worker.c, but even there it > > doesn't seem optimal. One e.g. could easily imagine that we'd want to handle > > connection errors or configuration changes without restarting the worker, in > > which case process lifetime obviously isn't a good idea anymore. > > > > I don't deny that we can't make this at a more granular level. IIRC, > at that time, we tried to follow AtProcExit_Files which cleans up temp > files at proc exit and we needed something similar for temporary files > used via SharedFileSet. The comparison to AtProcExit_Files isn't convincing to me - normally temp files are cleaned up long before that via resowner cleanup. To me the reuse of SharedFileSet for worker.c as executed seems like a bad design. As things stand there's little code shared between dsm/non-dsm shared file sets. The cleanup semantics are entirely different. Several functions don't work if used on the "wrong kind" of set (e.g. SharedFileSetAttach()). > I think we can extend this API but I guess it is better to then do it > for dsm-based as well so that these get tracked via resowner. DSM segments are resowner managed already, so it's not obvious that that'd buy us much? Although I guess there could be a few edge cases that'd look cleaner, because we could reliably trigger cleanup in the leader instead of relying on dsm detach hooks + refcounts to manage when a set is physically deleted? Greetings, Andres Freund
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-16T14:47:47Z
On Fri, Aug 13, 2021 at 9:29 PM Andres Freund <andres@anarazel.de> wrote: > > I think we can extend this API but I guess it is better to then do it > > for dsm-based as well so that these get tracked via resowner. > > DSM segments are resowner managed already, so it's not obvious that that'd buy > us much? Although I guess there could be a few edge cases that'd look cleaner, > because we could reliably trigger cleanup in the leader instead of relying on > dsm detach hooks + refcounts to manage when a set is physically deleted? > In an off-list discussion with Thomas and Amit, we tried to discuss how to clean up the shared files set in the current use case. Thomas suggested that instead of using individual shared fileset for storing the data for each XID why don't we just create a single shared fileset for complete worker lifetime and when the worker is exiting we can just remove that shared fileset. And for storing XID data, we can just create the files under the same shared fileset and delete those files when we longer need them. I like this idea and it looks much cleaner, after this, we can get rid of the special cleanup mechanism using 'filesetlist'. I have attached a patch for the same. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-17T05:24:30Z
On Mon, Aug 16, 2021 at 8:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Aug 13, 2021 at 9:29 PM Andres Freund <andres@anarazel.de> wrote: > > > > I think we can extend this API but I guess it is better to then do it > > > for dsm-based as well so that these get tracked via resowner. > > > > DSM segments are resowner managed already, so it's not obvious that that'd buy > > us much? Although I guess there could be a few edge cases that'd look cleaner, > > because we could reliably trigger cleanup in the leader instead of relying on > > dsm detach hooks + refcounts to manage when a set is physically deleted? > > > > In an off-list discussion with Thomas and Amit, we tried to discuss > how to clean up the shared files set in the current use case. Thomas > suggested that instead of using individual shared fileset for storing > the data for each XID why don't we just create a single shared fileset > for complete worker lifetime and when the worker is exiting we can > just remove that shared fileset. And for storing XID data, we can > just create the files under the same shared fileset and delete those > files when we longer need them. I like this idea and it looks much > cleaner, after this, we can get rid of the special cleanup mechanism > using 'filesetlist'. I have attached a patch for the same. > It seems to me that this idea would obviate any need for resource owners as we will have only one fileset now. I have a few initial comments on the patch: 1. + /* do cleanup on worker exit (e.g. after DROP SUBSCRIPTION) */ + on_shmem_exit(worker_cleanup, (Datum) 0); It should be registered with before_shmem_exit() hook to allow sending stats for file removal. 2. After these changes, the comments atop stream_open_file and SharedFileSetInit need to be changed. 3. In function subxact_info_write(), we are computing subxact file path twice which doesn't seem to be required. 4. + if (missing_ok) + return NULL; ereport(ERROR, (errcode_for_file_access(), - errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m", + errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m", segment_name, name))); There seems to be a formatting issue with errmsg. Also, it is better to keep an empty line before ereport. 5. How can we provide a strict mechanism to not allow to use dsm APIs for non-dsm FileSet? One idea could be that we can have a variable (probably bool) in SharedFileSet structure which will be initialized in SharedFileSetInit based on whether the caller has provided dsm segment. Then in other DSM-based APIs, we can check if it is used for the wrong type. -- With Regards, Amit Kapila. -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-17T06:36:37Z
On Tue, Aug 17, 2021 at 10:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Aug 16, 2021 at 8:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Aug 13, 2021 at 9:29 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > I think we can extend this API but I guess it is better to then do it > > > > for dsm-based as well so that these get tracked via resowner. > > > > > > DSM segments are resowner managed already, so it's not obvious that that'd buy > > > us much? Although I guess there could be a few edge cases that'd look cleaner, > > > because we could reliably trigger cleanup in the leader instead of relying on > > > dsm detach hooks + refcounts to manage when a set is physically deleted? > > > > > > > In an off-list discussion with Thomas and Amit, we tried to discuss > > how to clean up the shared files set in the current use case. Thomas > > suggested that instead of using individual shared fileset for storing > > the data for each XID why don't we just create a single shared fileset > > for complete worker lifetime and when the worker is exiting we can > > just remove that shared fileset. And for storing XID data, we can > > just create the files under the same shared fileset and delete those > > files when we longer need them. I like this idea and it looks much > > cleaner, after this, we can get rid of the special cleanup mechanism > > using 'filesetlist'. I have attached a patch for the same. > > > > It seems to me that this idea would obviate any need for resource > owners as we will have only one fileset now. I have a few initial > comments on the patch: > One more comment: @@ -2976,39 +2952,17 @@ subxact_info_write(Oid subid, TransactionId xid) .. + /* Try to open the subxact file, if it doesn't exist then create it */ + fd = BufFileOpenShared(xidfileset, path, O_RDWR, true); + if (fd == NULL) + fd = BufFileCreateShared(xidfileset, path); .. Instead of trying to create the file here based on whether it exists or not, can't we create it in subxact_info_add where we are first time allocating memory for subxacts? If that works then in the above code, the file should always exist. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-17T07:37:45Z
On Tue, Aug 17, 2021 at 12:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > One more comment: > @@ -2976,39 +2952,17 @@ subxact_info_write(Oid subid, TransactionId xid) > .. > + /* Try to open the subxact file, if it doesn't exist then create it */ > + fd = BufFileOpenShared(xidfileset, path, O_RDWR, true); > + if (fd == NULL) > + fd = BufFileCreateShared(xidfileset, path); > .. > > Instead of trying to create the file here based on whether it exists > or not, can't we create it in subxact_info_add where we are first time > allocating memory for subxacts? If that works then in the above code, > the file should always exist. One problem with this approach is that for now we delay creating the subxact file till the end of the stream and if by end of the stream all the subtransactions got aborted within the same stream then we don't even create that file. But with this suggestion, we will always create the file as soon as we get the first subtransaction. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-17T08:00:06Z
On Tue, Aug 17, 2021 at 10:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Aug 16, 2021 at 8:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Aug 13, 2021 at 9:29 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > I think we can extend this API but I guess it is better to then do it > > > > for dsm-based as well so that these get tracked via resowner. > > > > > > DSM segments are resowner managed already, so it's not obvious that that'd buy > > > us much? Although I guess there could be a few edge cases that'd look cleaner, > > > because we could reliably trigger cleanup in the leader instead of relying on > > > dsm detach hooks + refcounts to manage when a set is physically deleted? > > > > > > > In an off-list discussion with Thomas and Amit, we tried to discuss > > how to clean up the shared files set in the current use case. Thomas > > suggested that instead of using individual shared fileset for storing > > the data for each XID why don't we just create a single shared fileset > > for complete worker lifetime and when the worker is exiting we can > > just remove that shared fileset. And for storing XID data, we can > > just create the files under the same shared fileset and delete those > > files when we longer need them. I like this idea and it looks much > > cleaner, after this, we can get rid of the special cleanup mechanism > > using 'filesetlist'. I have attached a patch for the same. > > > > It seems to me that this idea would obviate any need for resource > owners as we will have only one fileset now. I have a few initial > comments on the patch: > > 1. > + /* do cleanup on worker exit (e.g. after DROP SUBSCRIPTION) */ > + on_shmem_exit(worker_cleanup, (Datum) 0); > > It should be registered with before_shmem_exit() hook to allow sending > stats for file removal. Done > 2. After these changes, the comments atop stream_open_file and > SharedFileSetInit need to be changed. Done > 3. In function subxact_info_write(), we are computing subxact file > path twice which doesn't seem to be required. Fixed > 4. > + if (missing_ok) > + return NULL; > ereport(ERROR, > (errcode_for_file_access(), > - errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m", > + errmsg("could not open temporary file \"%s\" from BufFile \"%s\": %m", > segment_name, name))); > > There seems to be a formatting issue with errmsg. Also, it is better > to keep an empty line before ereport. Done > 5. How can we provide a strict mechanism to not allow to use dsm APIs > for non-dsm FileSet? One idea could be that we can have a variable > (probably bool) in SharedFileSet structure which will be initialized > in SharedFileSetInit based on whether the caller has provided dsm > segment. Then in other DSM-based APIs, we can check if it is used for > the wrong type. Yeah, we can do something like that, can't we just use an existing variable instead of adding new, e.g. refcnt is required only when multiple processes are attached, so maybe if dsm segment is not passed then we can keep refcnt as 0 and based on we can give an error. For example, if we try to call SharedFileSetAttach for the SharedFileSet which has refcnt as 0 then we error out? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-17T08:56:37Z
On Tue, Aug 17, 2021 at 1:30 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Aug 17, 2021 at 10:54 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > for non-dsm FileSet? One idea could be that we can have a variable > > (probably bool) in SharedFileSet structure which will be initialized > > in SharedFileSetInit based on whether the caller has provided dsm > > segment. Then in other DSM-based APIs, we can check if it is used for > > the wrong type. > > Yeah, we can do something like that, can't we just use an existing > variable instead of adding new, e.g. refcnt is required only when > multiple processes are attached, so maybe if dsm segment is not passed > then we can keep refcnt as 0 and based on we can give an error. For > example, if we try to call SharedFileSetAttach for the SharedFileSet > which has refcnt as 0 then we error out? > But as of now, we treat refcnt as 0 for SharedFileSet that is already destroyed. See SharedFileSetAttach. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Andres Freund <andres@anarazel.de> — 2021-08-17T11:04:27Z
Hi, On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > 5. How can we provide a strict mechanism to not allow to use dsm APIs > for non-dsm FileSet? One idea could be that we can have a variable > (probably bool) in SharedFileSet structure which will be initialized > in SharedFileSetInit based on whether the caller has provided dsm > segment. Then in other DSM-based APIs, we can check if it is used for > the wrong type. Well, isn't the issue here that it's not a shared file set in case you explicitly don't want to share it? ISTM that the proper way to address this would be to split out a FileSet from SharedFileSet that's then used for worker.c and sharedfileset.c. Rather than making sharedfileset.c support a non-shared mode. Greetings, Andres Freund
-
RE: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-18T01:16:50Z
Hi, I took a quick look at the v2 patch and noticed a typo. + * backends and render it read-only. If missing_ok is true then it will return + * NULL if file doesn not exist otherwise error. */ doesn not=> doesn't Best regards, Houzj
-
RE: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-18T02:52:57Z
On Wed, Aug 18, 2021 9:17 AM houzj.fnst@fujitsu.com wrote: > Hi, > > I took a quick look at the v2 patch and noticed a typo. > > + * backends and render it read-only. If missing_ok is true then it will return > + * NULL if file doesn not exist otherwise error. > */ > doesn not=> doesn't > Here are some other comments: 1). +BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode, + bool missing_ok) { BufFile *file; char segment_name[MAXPGPATH]; ... files = palloc(sizeof(File) * capacity); ... @@ -318,10 +320,15 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode) * name. */ if (nfiles == 0) + { + if (missing_ok) + return NULL; + I think it might be better to pfree(files) when return NULL. 2). /* Delete the subxact file and release the memory, if it exist */ - if (ent->subxact_fileset) - { - subxact_filename(path, subid, xid); - SharedFileSetDeleteAll(ent->subxact_fileset); - pfree(ent->subxact_fileset); - ent->subxact_fileset = NULL; - } - - /* Remove the xid entry from the stream xid hash */ - hash_search(xidhash, (void *) &xid, HASH_REMOVE, NULL); + subxact_filename(path, subid, xid); + SharedFileSetDelete(xidfileset, path, true); Without the patch it doesn't throw an error if not exist, But with the patch, it pass error_on_failure=true to SharedFileSetDelete(). Was it intentional ? Best regards, Houzj -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-18T04:00:09Z
On Wed, Aug 18, 2021 at 8:23 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > > On Wed, Aug 18, 2021 9:17 AM houzj.fnst@fujitsu.com wrote: > > Hi, > > > > I took a quick look at the v2 patch and noticed a typo. > > > > + * backends and render it read-only. If missing_ok is true then it will return > > + * NULL if file doesn not exist otherwise error. > > */ > > doesn not=> doesn't > > > > Here are some other comments: > > 1). > +BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode, > + bool missing_ok) > { > BufFile *file; > char segment_name[MAXPGPATH]; > ... > files = palloc(sizeof(File) * capacity); > ... > @@ -318,10 +320,15 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode) > * name. > */ > if (nfiles == 0) > + { > + if (missing_ok) > + return NULL; > + > > I think it might be better to pfree(files) when return NULL. > > > 2). > /* Delete the subxact file and release the memory, if it exist */ > - if (ent->subxact_fileset) > - { > - subxact_filename(path, subid, xid); > - SharedFileSetDeleteAll(ent->subxact_fileset); > - pfree(ent->subxact_fileset); > - ent->subxact_fileset = NULL; > - } > - > - /* Remove the xid entry from the stream xid hash */ > - hash_search(xidhash, (void *) &xid, HASH_REMOVE, NULL); > + subxact_filename(path, subid, xid); > + SharedFileSetDelete(xidfileset, path, true); > > Without the patch it doesn't throw an error if not exist, > But with the patch, it pass error_on_failure=true to SharedFileSetDelete(). > Don't we need to use BufFileDeleteShared instead of SharedFileSetDelete as you have used to remove the changes file? -- With Regards, Amit Kapila. -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-18T04:17:40Z
On Wed, Aug 18, 2021 at 9:30 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Aug 18, 2021 at 8:23 AM houzj.fnst@fujitsu.com > <houzj.fnst@fujitsu.com> wrote: > > > > On Wed, Aug 18, 2021 9:17 AM houzj.fnst@fujitsu.com wrote: > > > Hi, > > > > > > I took a quick look at the v2 patch and noticed a typo. > > > > > > + * backends and render it read-only. If missing_ok is true then it will return > > > + * NULL if file doesn not exist otherwise error. > > > */ > > > doesn not=> doesn't > > > > > > > Here are some other comments: > > > > 1). > > +BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode, > > + bool missing_ok) > > { > > BufFile *file; > > char segment_name[MAXPGPATH]; > > ... > > files = palloc(sizeof(File) * capacity); > > ... > > @@ -318,10 +320,15 @@ BufFileOpenShared(SharedFileSet *fileset, const char *name, int mode) > > * name. > > */ > > if (nfiles == 0) > > + { > > + if (missing_ok) > > + return NULL; > > + > > > > I think it might be better to pfree(files) when return NULL. > > > > > > 2). > > /* Delete the subxact file and release the memory, if it exist */ > > - if (ent->subxact_fileset) > > - { > > - subxact_filename(path, subid, xid); > > - SharedFileSetDeleteAll(ent->subxact_fileset); > > - pfree(ent->subxact_fileset); > > - ent->subxact_fileset = NULL; > > - } > > - > > - /* Remove the xid entry from the stream xid hash */ > > - hash_search(xidhash, (void *) &xid, HASH_REMOVE, NULL); > > + subxact_filename(path, subid, xid); > > + SharedFileSetDelete(xidfileset, path, true); > > > > Without the patch it doesn't throw an error if not exist, > > But with the patch, it pass error_on_failure=true to SharedFileSetDelete(). > > > > Don't we need to use BufFileDeleteShared instead of > SharedFileSetDelete as you have used to remove the changes file? Yeah, it should be BufFileDeleteShared. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-18T05:54:01Z
On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres@anarazel.de> wrote: > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > for non-dsm FileSet? One idea could be that we can have a variable > > (probably bool) in SharedFileSet structure which will be initialized > > in SharedFileSetInit based on whether the caller has provided dsm > > segment. Then in other DSM-based APIs, we can check if it is used for > > the wrong type. > > Well, isn't the issue here that it's not a shared file set in case you > explicitly don't want to share it? ISTM that the proper way to address > this would be to split out a FileSet from SharedFileSet that's then used > for worker.c and sharedfileset.c. > Okay, but note that to accomplish the same, we need to tweak the BufFile (buffile.c) APIs as well so that they can work with FileSet. As per the initial analysis, there doesn't seem to be any problem with that though. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-18T10:15:14Z
On Wed, Aug 18, 2021 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres@anarazel.de> wrote: > > > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > > for non-dsm FileSet? One idea could be that we can have a variable > > > (probably bool) in SharedFileSet structure which will be initialized > > > in SharedFileSetInit based on whether the caller has provided dsm > > > segment. Then in other DSM-based APIs, we can check if it is used for > > > the wrong type. > > > > Well, isn't the issue here that it's not a shared file set in case you > > explicitly don't want to share it? ISTM that the proper way to address > > this would be to split out a FileSet from SharedFileSet that's then used > > for worker.c and sharedfileset.c. > > > > Okay, but note that to accomplish the same, we need to tweak the > BufFile (buffile.c) APIs as well so that they can work with FileSet. > As per the initial analysis, there doesn't seem to be any problem with > that though. I was looking into this, so if we want to do that I think the outline will look like this - There will be a fileset.c and fileset.h files, and we will expose a new structure FileSet, which will be the same as SharedFileSet, except mutext and refcount. The fileset.c will expose FileSetInit(), FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() interfaces. - sharefileset.c will internally call the fileset.c's interfaces. The SharedFileSet structure will also contain FileSet and other members i.e. mutex and refcount. - the buffile.c's interfaces which are ending with Shared e.g. BufFileCreateShared, BufFileOpenShared, should be converted to BufFileCreate and BufFileOpen respectively. And the input to these interfaces can be converted to FileSet instead of SharedFileSet. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-19T04:23:43Z
On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Aug 18, 2021 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > > > for non-dsm FileSet? One idea could be that we can have a variable > > > > (probably bool) in SharedFileSet structure which will be initialized > > > > in SharedFileSetInit based on whether the caller has provided dsm > > > > segment. Then in other DSM-based APIs, we can check if it is used for > > > > the wrong type. > > > > > > Well, isn't the issue here that it's not a shared file set in case you > > > explicitly don't want to share it? ISTM that the proper way to address > > > this would be to split out a FileSet from SharedFileSet that's then used > > > for worker.c and sharedfileset.c. > > > > > > > Okay, but note that to accomplish the same, we need to tweak the > > BufFile (buffile.c) APIs as well so that they can work with FileSet. > > As per the initial analysis, there doesn't seem to be any problem with > > that though. > > I was looking into this, so if we want to do that I think the outline > will look like this > > - There will be a fileset.c and fileset.h files, and we will expose a > new structure FileSet, which will be the same as SharedFileSet, except > mutext and refcount. The fileset.c will expose FileSetInit(), > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() > interfaces. > > - sharefileset.c will internally call the fileset.c's interfaces. The > SharedFileSet structure will also contain FileSet and other members > i.e. mutex and refcount. > > - the buffile.c's interfaces which are ending with Shared e.g. > BufFileCreateShared, BufFileOpenShared, should be converted to > BufFileCreate and > BufFileOpen respectively. > The other alternative to name buffile APIs could be BufFileCreateFileSet, BufFileOpenFileSet, etc. -- With Regards, Amit Kapila.
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-21T12:38:17Z
On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Aug 18, 2021 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > > > for non-dsm FileSet? One idea could be that we can have a variable > > > > (probably bool) in SharedFileSet structure which will be initialized > > > > in SharedFileSetInit based on whether the caller has provided dsm > > > > segment. Then in other DSM-based APIs, we can check if it is used for > > > > the wrong type. > > > > > > Well, isn't the issue here that it's not a shared file set in case you > > > explicitly don't want to share it? ISTM that the proper way to address > > > this would be to split out a FileSet from SharedFileSet that's then used > > > for worker.c and sharedfileset.c. > > > > > > > Okay, but note that to accomplish the same, we need to tweak the > > BufFile (buffile.c) APIs as well so that they can work with FileSet. > > As per the initial analysis, there doesn't seem to be any problem with > > that though. > > I was looking into this, so if we want to do that I think the outline > will look like this > > - There will be a fileset.c and fileset.h files, and we will expose a > new structure FileSet, which will be the same as SharedFileSet, except > mutext and refcount. The fileset.c will expose FileSetInit(), > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() > interfaces. > > - sharefileset.c will internally call the fileset.c's interfaces. The > SharedFileSet structure will also contain FileSet and other members > i.e. mutex and refcount. > > - the buffile.c's interfaces which are ending with Shared e.g. > BufFileCreateShared, BufFileOpenShared, should be converted to > BufFileCreate and > BufFileOpen respectively. And the input to these interfaces can be > converted to FileSet instead of SharedFileSet. Here is the first draft based on the idea we discussed, 0001, splits sharedfileset.c in sharedfileset.c and fileset.c and 0002 is same patch I submitted earlier(use single fileset throughout the worker), just it is rebased on top of 0001. Please let me know your thoughts. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
RE: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-23T03:41:22Z
On Sat, Aug 21, 2021 8:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > I was looking into this, so if we want to do that I think the outline > > will look like this > > > > - There will be a fileset.c and fileset.h files, and we will expose a > > new structure FileSet, which will be the same as SharedFileSet, except > > mutext and refcount. The fileset.c will expose FileSetInit(), > > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() > > interfaces. > > > > - sharefileset.c will internally call the fileset.c's interfaces. The > > SharedFileSet structure will also contain FileSet and other members > > i.e. mutex and refcount. > > > > - the buffile.c's interfaces which are ending with Shared e.g. > > BufFileCreateShared, BufFileOpenShared, should be converted to > > BufFileCreate and BufFileOpen respectively. And the input to these > > interfaces can be converted to FileSet instead of SharedFileSet. > > Here is the first draft based on the idea we discussed, 0001, splits > sharedfileset.c in sharedfileset.c and fileset.c and 0002 is same patch I > submitted earlier(use single fileset throughout the worker), just it is rebased on > top of 0001. Please let me know your thoughts. Hi, Here are some comments for the new version patches. 1) + TempTablespacePath(tempdirpath, tablespace); + snprintf(path, MAXPGPATH, "%s/%s%lu.%u.sharedfileset", + tempdirpath, PG_TEMP_FILE_PREFIX, + (unsigned long) fileset->creator_pid, fileset->number); do we need to use different filename for shared and un-shared fileset ? 2) I think we can remove or adjust the following comments in sharedfileset.c. ---- * SharedFileSets can be used by backends when the temporary files need to be * opened/closed multiple times and the underlying files need to survive across * transactions. ---- * We can also use this interface if the temporary files are used only by * single backend but the files need to be opened and closed multiple times * and also the underlying files need to survive across transactions. For ---- 3) The 0002 patch still used the word "shared fileset" in some places, I think we should change it to "fileset". 4) -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, - int mode); -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, - bool error_on_failure); extern void SharedFileSetDeleteAll(SharedFileSet *fileset); -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); I noticed the patch delete part of public api, is it better to keep the old api and let them invoke new api internally ? Having said that, I didn’t find any open source extension use these old api, so it might be fine to delete them. Best regards, Hou zj
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-23T04:22:50Z
On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > 4) > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > - int mode); > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > - bool error_on_failure); > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > I noticed the patch delete part of public api, is it better to keep the old api and > let them invoke new api internally ? Having said that, I didn’t find any open source > extension use these old api, so it might be fine to delete them. Right, those were internally used by buffile.c but now we have changed buffile.c to directly use the fileset interfaces, so we better remove them. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-23T06:13:10Z
On Mon, Aug 23, 2021 at 9:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com > <houzj.fnst@fujitsu.com> wrote: > > > 4) > > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > > - int mode); > > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > > - bool error_on_failure); > > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > > > I noticed the patch delete part of public api, is it better to keep the old api and > > let them invoke new api internally ? Having said that, I didn’t find any open source > > extension use these old api, so it might be fine to delete them. > > Right, those were internally used by buffile.c but now we have changed > buffile.c to directly use the fileset interfaces, so we better remove > them. > I also don't see any reason to keep those exposed from sharedfileset.h. I see that even in the original commit dc6c4c9dc2, these APIs seem to be introduced to be used by buffile. Andres/Thomas, do let us know if you think otherwise? One more comment: I think v1-0001-Sharedfileset-refactoring doesn't have a way for cleaning up worker.c temporary files on error/exit. It is better to have that to make it an independent patch. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-23T07:22:04Z
On Mon, Aug 23, 2021 at 11:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Aug 23, 2021 at 9:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com > > <houzj.fnst@fujitsu.com> wrote: > > > > > 4) > > > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > > > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > > > - int mode); > > > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > > > - bool error_on_failure); > > > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > > > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > > > > > I noticed the patch delete part of public api, is it better to keep the old api and > > > let them invoke new api internally ? Having said that, I didn’t find any open source > > > extension use these old api, so it might be fine to delete them. > > > > Right, those were internally used by buffile.c but now we have changed > > buffile.c to directly use the fileset interfaces, so we better remove > > them. > > > > I also don't see any reason to keep those exposed from > sharedfileset.h. I see that even in the original commit dc6c4c9dc2, > these APIs seem to be introduced to be used by buffile. Andres/Thomas, > do let us know if you think otherwise? > > One more comment: > I think v1-0001-Sharedfileset-refactoring doesn't have a way for > cleaning up worker.c temporary files on error/exit. It is better to > have that to make it an independent patch. I think we should handle that in worker.c itself, by adding a before_dsm_detach function before_shmem_exit right? Or you are thinking that in FileSetInit, we keep the mechanism of filesetlist like we were doing in SharedFileSetInit? I think that will just add unnecessary complexity in the first patch which will eventually go away in the second patch. And if we do that then SharedFileSetInit can not directly use the FileSetInit, otherwise, the dsm based fileset will also get registered for cleanup in filesetlist so for that we might need to pass one parameter to the FileSetInit() that whether to register for cleanup or not and that will again not look clean because now we are again adding the conditional cleanup, IMHO that is the same problem what we are trying to cleanup in SharedFileSetInit by introducing a new FileSetInit. I think what we can do is, introduce a new function FileSetInitInternal(), that will do what FileSetInit() is doing today and now both SharedFileSetInit() and the FileSetInit() will call this function, and along with that SharedFileSetInit(), will register the dsm based cleanup and FileSetInit() will do the filesetlist based cleanup. But IMHO, we should try to go in this direction only if we are sure that we want to commit the first patch and not the second. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-23T07:37:27Z
On Sat, Aug 21, 2021 at 9:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Aug 18, 2021 at 3:45 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Aug 18, 2021 at 11:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > > > On Tue, Aug 17, 2021 at 4:34 PM Andres Freund <andres@anarazel.de> wrote: > > > > > > > > On 2021-08-17 10:54:30 +0530, Amit Kapila wrote: > > > > > 5. How can we provide a strict mechanism to not allow to use dsm APIs > > > > > for non-dsm FileSet? One idea could be that we can have a variable > > > > > (probably bool) in SharedFileSet structure which will be initialized > > > > > in SharedFileSetInit based on whether the caller has provided dsm > > > > > segment. Then in other DSM-based APIs, we can check if it is used for > > > > > the wrong type. > > > > > > > > Well, isn't the issue here that it's not a shared file set in case you > > > > explicitly don't want to share it? ISTM that the proper way to address > > > > this would be to split out a FileSet from SharedFileSet that's then used > > > > for worker.c and sharedfileset.c. > > > > > > > > > > Okay, but note that to accomplish the same, we need to tweak the > > > BufFile (buffile.c) APIs as well so that they can work with FileSet. > > > As per the initial analysis, there doesn't seem to be any problem with > > > that though. > > > > I was looking into this, so if we want to do that I think the outline > > will look like this > > > > - There will be a fileset.c and fileset.h files, and we will expose a > > new structure FileSet, which will be the same as SharedFileSet, except > > mutext and refcount. The fileset.c will expose FileSetInit(), > > FileSetCreate(), FileSetOpen(), FileSetDelete() and FileSetDeleteAll() > > interfaces. > > > > - sharefileset.c will internally call the fileset.c's interfaces. The > > SharedFileSet structure will also contain FileSet and other members > > i.e. mutex and refcount. > > > > - the buffile.c's interfaces which are ending with Shared e.g. > > BufFileCreateShared, BufFileOpenShared, should be converted to > > BufFileCreate and > > BufFileOpen respectively. And the input to these interfaces can be > > converted to FileSet instead of SharedFileSet. > > Here is the first draft based on the idea we discussed, 0001, splits > sharedfileset.c in sharedfileset.c and fileset.c and 0002 is same > patch I submitted earlier(use single fileset throughout the worker), > just it is rebased on top of 0001. Please let me know your thoughts. Here are some comments on 0001 patch: +/* + * Initialize a space for temporary files. This API can be used by shared + * fileset as well as if the temporary files are used only by single backend + * but the files need to be opened and closed multiple times and also the + * underlying files need to survive across transactions. + * + * Files will be distributed over the tablespaces configured in + * temp_tablespaces. + * + * Under the covers the set is one or more directories which will eventually + * be deleted. + */ I think it's better to mention cleaning up here like we do in the comment of SharedFileSetInit(). --- I think we need to clean up both stream_fileset and subxact_fileset on proc exit. In 0002 patch cleans up the fileset but I think we need to do that also in 0001 patch. --- There still are some comments using "shared fileset" in both buffile.c and worker.c. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-23T08:13:01Z
On Mon, Aug 23, 2021 at 12:52 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Aug 23, 2021 at 11:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Mon, Aug 23, 2021 at 9:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com > > > <houzj.fnst@fujitsu.com> wrote: > > > > > > > 4) > > > > -extern File SharedFileSetCreate(SharedFileSet *fileset, const char *name); > > > > -extern File SharedFileSetOpen(SharedFileSet *fileset, const char *name, > > > > - int mode); > > > > -extern bool SharedFileSetDelete(SharedFileSet *fileset, const char *name, > > > > - bool error_on_failure); > > > > extern void SharedFileSetDeleteAll(SharedFileSet *fileset); > > > > -extern void SharedFileSetUnregister(SharedFileSet *input_fileset); > > > > > > > > I noticed the patch delete part of public api, is it better to keep the old api and > > > > let them invoke new api internally ? Having said that, I didn’t find any open source > > > > extension use these old api, so it might be fine to delete them. > > > > > > Right, those were internally used by buffile.c but now we have changed > > > buffile.c to directly use the fileset interfaces, so we better remove > > > them. > > > > > > > I also don't see any reason to keep those exposed from > > sharedfileset.h. I see that even in the original commit dc6c4c9dc2, > > these APIs seem to be introduced to be used by buffile. Andres/Thomas, > > do let us know if you think otherwise? > > > > One more comment: > > I think v1-0001-Sharedfileset-refactoring doesn't have a way for > > cleaning up worker.c temporary files on error/exit. It is better to > > have that to make it an independent patch. > > I think we should handle that in worker.c itself, by adding a > before_dsm_detach function before_shmem_exit right? > Yeah, I thought of handling it in worker.c similar to what you've in 0002 patch. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-23T09:43:19Z
On Mon, Aug 23, 2021 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote: Note: merge comments from multiple mails > > I think we should handle that in worker.c itself, by adding a > > before_dsm_detach function before_shmem_exit right? > > > > Yeah, I thought of handling it in worker.c similar to what you've in 0002 patch. > I have done handling in worker.c On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > > On Sat, Aug 21, 2021 8:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > 1) > + TempTablespacePath(tempdirpath, tablespace); > + snprintf(path, MAXPGPATH, "%s/%s%lu.%u.sharedfileset", > + tempdirpath, PG_TEMP_FILE_PREFIX, > + (unsigned long) fileset->creator_pid, fileset->number); > > do we need to use different filename for shared and un-shared fileset ? I was also thinking about the same, does it make sense to name it just ""%s/%s%lu.%u.fileset"? On Mon, Aug 23, 2021 at 1:08 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > Here are some comments on 0001 patch: > > +/* > + * Initialize a space for temporary files. This API can be used by shared > + * fileset as well as if the temporary files are used only by single backend > + * but the files need to be opened and closed multiple times and also the > + * underlying files need to survive across transactions. > + * > + * Files will be distributed over the tablespaces configured in > + * temp_tablespaces. > + * > + * Under the covers the set is one or more directories which will eventually > + * be deleted. > + */ > > I think it's better to mention cleaning up here like we do in the > comment of SharedFileSetInit(). Right, done > > --- > I think we need to clean up both stream_fileset and subxact_fileset on > proc exit. In 0002 patch cleans up the fileset but I think we need to > do that also in 0001 patch. Done > --- > There still are some comments using "shared fileset" in both buffile.c > and worker.c. Done -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-24T06:56:02Z
On Mon, Aug 23, 2021 at 3:13 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Mon, Aug 23, 2021 at 1:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > Note: merge comments from multiple mails > > > > I think we should handle that in worker.c itself, by adding a > > > before_dsm_detach function before_shmem_exit right? > > > > > > > Yeah, I thought of handling it in worker.c similar to what you've in 0002 patch. > > > > I have done handling in worker.c > > On Mon, Aug 23, 2021 at 9:11 AM houzj.fnst@fujitsu.com > <houzj.fnst@fujitsu.com> wrote: > > > > On Sat, Aug 21, 2021 8:38 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > 1) > > + TempTablespacePath(tempdirpath, tablespace); > > + snprintf(path, MAXPGPATH, "%s/%s%lu.%u.sharedfileset", > > + tempdirpath, PG_TEMP_FILE_PREFIX, > > + (unsigned long) fileset->creator_pid, fileset->number); > > > > do we need to use different filename for shared and un-shared fileset ? > > I was also thinking about the same, does it make sense to name it just > ""%s/%s%lu.%u.fileset"? > I think it is reasonable to use .fileset as proposed by you. Few other comments: ================= 1. + /* + * Register before-shmem-exit hook to ensure filesets are dropped while we + * can still report stats for underlying temporary files. + */ + before_shmem_exit(worker_cleanup, (Datum) 0); + Do we really need to register a new callback here? Won't the existing logical replication worker exit routine (logicalrep_worker_onexit) be sufficient for this patch's purpose? 2. - SharedFileSet *fileset; /* space for segment files if shared */ - const char *name; /* name of this BufFile if shared */ + FileSet *fileset; /* space for fileset for fileset based file */ + const char *name; /* name of this BufFile */ The comments for the above two variables can be written as (a) space for fileset based segment files, (b) name of fileset based BufFile. 3. /* - * Create a new segment file backing a shared BufFile. + * Create a new segment file backing a fileset BufFile. */ static File -MakeNewSharedSegment(BufFile *buffile, int segment) +MakeNewSegment(BufFile *buffile, int segment) I think it is better to name this function as MakeNewFileSetSegment. You can slightly change the comment as: "Create a new segment file backing a fileset based BufFile." 4. /* * It is possible that there are files left over from before a crash - * restart with the same name. In order for BufFileOpenShared() not to + * restart with the same name. In order for BufFileOpen() not to * get confused about how many segments there are, we'll unlink the next Typo. /BufFileOpen/BufFileOpenFileSet 5. static void -SharedSegmentName(char *name, const char *buffile_name, int segment) +SegmentName(char *name, const char *buffile_name, int segment) Can we name this as FileSetSegmentName? 6. * - * The naming scheme for shared BufFiles is left up to the calling code. The + * The naming scheme for fileset BufFiles is left up to the calling code. Isn't it better to say "... fileset based BufFiles .."? 7. + * FileSets provide a temporary namespace (think directory) so that files can + * be discovered by name A full stop is missing at the end of the statement. 8. + * + * The callers are expected to explicitly remove such files by using + * SharedFileSetDelete/ SharedFileSetDeleteAll. + * + * Files will be distributed over the tablespaces configured in + * temp_tablespaces. + * + * Under the covers the set is one or more directories which will eventually + * be deleted. + */ +void +FileSetInit(FileSet *fileset) Is there a need to mention 'Shared' in API names (SharedFileSetDelete/ SharedFileSetDeleteAll) in the comments? Also, there doesn't seem to be a need for extra space before *DeleteAll API in comments. 9. * Files will be distributed over the tablespaces configured in * temp_tablespaces. * * Under the covers the set is one or more directories which will eventually * be deleted. */ void SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg) I think we can remove the part of the above comment where it says "Files will be distributed over ..." as that is already mentioned atop FileSetInit. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-24T10:25:26Z
On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > I was also thinking about the same, does it make sense to name it just > > ""%s/%s%lu.%u.fileset"? Done > I think it is reasonable to use .fileset as proposed by you. > > Few other comments: > ================= > 1. > + /* > + * Register before-shmem-exit hook to ensure filesets are dropped while we > + * can still report stats for underlying temporary files. > + */ > + before_shmem_exit(worker_cleanup, (Datum) 0); > + > > Do we really need to register a new callback here? Won't the existing > logical replication worker exit routine (logicalrep_worker_onexit) be > sufficient for this patch's purpose? Right, we don't need an extra function for this. > 2. > - SharedFileSet *fileset; /* space for segment files if shared */ > - const char *name; /* name of this BufFile if shared */ > + FileSet *fileset; /* space for fileset for fileset based file */ > + const char *name; /* name of this BufFile */ > > The comments for the above two variables can be written as (a) space > for fileset based segment files, (b) name of fileset based BufFile. Done > 3. > /* > - * Create a new segment file backing a shared BufFile. > + * Create a new segment file backing a fileset BufFile. > */ > static File > -MakeNewSharedSegment(BufFile *buffile, int segment) > +MakeNewSegment(BufFile *buffile, int segment) > > I think it is better to name this function as MakeNewFileSetSegment. > You can slightly change the comment as: "Create a new segment file > backing a fileset based BufFile." Make sense > 4. > /* > * It is possible that there are files left over from before a crash > - * restart with the same name. In order for BufFileOpenShared() not to > + * restart with the same name. In order for BufFileOpen() not to > * get confused about how many segments there are, we'll unlink the next > > Typo. /BufFileOpen/BufFileOpenFileSet Fixed > 5. > static void > -SharedSegmentName(char *name, const char *buffile_name, int segment) > +SegmentName(char *name, const char *buffile_name, int segment) > > Can we name this as FileSetSegmentName? Done > 6. > * > - * The naming scheme for shared BufFiles is left up to the calling code. The > + * The naming scheme for fileset BufFiles is left up to the calling code. > > Isn't it better to say "... fileset based BufFiles .."? Done > 7. > + * FileSets provide a temporary namespace (think directory) so that files can > + * be discovered by name > > A full stop is missing at the end of the statement. Fixed > 8. > + * > + * The callers are expected to explicitly remove such files by using > + * SharedFileSetDelete/ SharedFileSetDeleteAll. > + * > + * Files will be distributed over the tablespaces configured in > + * temp_tablespaces. > + * > + * Under the covers the set is one or more directories which will eventually > + * be deleted. > + */ > +void > +FileSetInit(FileSet *fileset) > > Is there a need to mention 'Shared' in API names (SharedFileSetDelete/ > SharedFileSetDeleteAll) in the comments? Also, there doesn't seem to > be a need for extra space before *DeleteAll API in comments. Fixed > 9. > * Files will be distributed over the tablespaces configured in > * temp_tablespaces. > * > * Under the covers the set is one or more directories which will eventually > * be deleted. > */ > void > SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg) > > I think we can remove the part of the above comment where it says > "Files will be distributed over ..." as that is already mentioned atop > FileSetInit. Done -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-25T01:50:42Z
On Tuesday, August 24, 2021 6:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > I was also thinking about the same, does it make sense to name it > > > just ""%s/%s%lu.%u.fileset"? > > Done > > > I think it is reasonable to use .fileset as proposed by you. > > > > Few other comments: > Done After applying the patch, I tested the impacted features (parallel hashjoin/btree build) with different settings (log_temp_files/maintenance_work_mem/parallel_leader_participation/workers) , and the patch works well. One thing I noticed is that when enable log_temp_files, the pathname in log changed from "xxx.sharedfileset" to "xxx.fileset", and I also noticed some blogs[1] reference the old path name. Although, I think it doesn't matter, just share the information here in case someone has different opinions. Some minor thing I noticed in the patch: 1) it seems we can add "FileSet" to typedefs.list 2) The commit message in 0002 used "shared fileset" which should be "fileset", --- Instead of using a separate shared fileset for each xid, use one shared fileset for whole lifetime of the worker... --- [1] https://blog.dbi-services.com/about-temp_tablespaces-in-postgresql/ Best regards, Hou zj
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-25T12:19:20Z
On Tue, Aug 24, 2021 at 3:55 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > The first patch looks good to me. I have made minor changes to the attached patch. The changes include: fixing compilation warning, made some comment changes, ran pgindent, and few other cosmetic changes. If you are fine with the attached, then kindly rebase the second patch atop it. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-26T01:46:49Z
On Wed, Aug 25, 2021 at 9:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Aug 24, 2021 at 3:55 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > The first patch looks good to me. I have made minor changes to the > attached patch. The changes include: fixing compilation warning, made > some comment changes, ran pgindent, and few other cosmetic changes. If > you are fine with the attached, then kindly rebase the second patch > atop it. The patch looks good to me. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-26T06:17:34Z
On Wed, Aug 25, 2021 at 5:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, Aug 24, 2021 at 3:55 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> > wrote: > > > > The first patch looks good to me. I have made minor changes to the > attached patch. The changes include: fixing compilation warning, made > some comment changes, ran pgindent, and few other cosmetic changes. If > you are fine with the attached, then kindly rebase the second patch > atop it. > > The patch looks good to me, I have rebased 0002 atop this patch and also done some cosmetic fixes in 0002. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-26T08:40:00Z
On Thu, Aug 26, 2021 at 11:47 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Aug 25, 2021 at 5:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Tue, Aug 24, 2021 at 3:55 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: >> > >> > On Tue, Aug 24, 2021 at 12:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> > >> >> The first patch looks good to me. I have made minor changes to the >> attached patch. The changes include: fixing compilation warning, made >> some comment changes, ran pgindent, and few other cosmetic changes. If >> you are fine with the attached, then kindly rebase the second patch >> atop it. >> > > The patch looks good to me, > Thanks, Sawada-San and Dilip for confirmation. I would like to commit this and the second patch (the second one still needs some more testing and review) for PG-15 as there is no bug per-se related to this work in PG-14 but I see an argument to commit this for PG-14 to keep the code (APIs) consistent. What do you think? Does anybody else have any opinion on this? Below is a summary of each of the patches for those who are not following this closely: Patch-1: The purpose of this patch is to refactor sharedfileset.c to separate out fileset implementation. Basically, this moves the fileset related implementation out of sharedfileset.c to allow its usage by backends that don't want to share filesets among different processes. After this split, fileset infrastructure is used by both sharedfileset.c and worker.c for the named temporary files that survive across transactions. This is suggested by Andres to have a cleaner API and its usage. Patch-2: Allow to use single fileset in worker.c (for the lifetime of worker) instead of using a separate fileset for each remote transaction. After this, the files to record changes for each remote transaction will be created under the same fileset and the files will be deleted after the transaction is completed. This is suggested by Thomas to allow better resource usage. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-27T05:24:46Z
On Thu, Aug 26, 2021 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > Thanks, Sawada-San and Dilip for confirmation. I would like to commit > this and the second patch (the second one still needs some more > testing and review) for PG-15 as there is no bug per-se related to > this work in PG-14 but I see an argument to commit this for PG-14 to > keep the code (APIs) consistent. What do you think? Does anybody else > have any opinion on this? > > IMHO, this is a fair amount of refactoring and this is actually an improvement patch so we should push it to v15. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-27T05:26:17Z
On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > The patch looks good to me, I have rebased 0002 atop > this patch and also done some cosmetic fixes in 0002. Here are some comments for the 0002 patch. 1) - * toplevel transaction. Each subscription has a separate set of files. + * toplevel transaction. Each subscription has a separate files. a separate files => a separate file 2) + * Otherwise, just open it file for writing, in append mode. */ open it file => open the file 3) if (subxact_data.nsubxacts == 0) { - if (ent->subxact_fileset) - { - cleanup_subxact_info(); - FileSetDeleteAll(ent->subxact_fileset); - pfree(ent->subxact_fileset); - ent->subxact_fileset = NULL; - } + cleanup_subxact_info(); + BufFileDeleteFileSet(stream_fileset, path, true); + Before applying the patch, the code only invoke cleanup_subxact_info() when the file exists. After applying the patch, it will invoke cleanup_subxact_info() either the file exists or not. Is it correct ? 4) /* - * If this is the first streamed segment, the file must not exist, so make - * sure we're the ones creating it. Otherwise just open the file for - * writing, in append mode. + * If this is the first streamed segment, create the changes file. + * Otherwise, just open it file for writing, in append mode. */ if (first_segment) - { ... - if (found) - ereport(ERROR, - (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg_internal("incorrect first-segment flag for streamed replication transaction"))); ... - } + stream_fd = BufFileCreateFileSet(stream_fileset, path); Since the function BufFileCreateFileSet() doesn't check the file's existence, the change here seems remove the file existence check which the old code did. Best regards, Hou zj -
RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-27T05:28:48Z
On Fri, Aug 27, 2021 1:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Thu, Aug 26, 2021 at 2:10 PM Amit Kapila <mailto:amit.kapila16@gmail.com> wrote: > >> Thanks, Sawada-San and Dilip for confirmation. I would like to commit >> this and the second patch (the second one still needs some more >> testing and review) for PG-15 as there is no bug per-se related to >> this work in PG-14 but I see an argument to commit this for PG-14 to >> keep the code (APIs) consistent. What do you think? Does anybody else >> have any opinion on this? > IMHO, this is a fair amount of refactoring and this is actually an > improvement patch so we should push it to v15. +1 Best regards, Hou zj
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-27T06:34:31Z
On Fri, Aug 27, 2021 at 10:56 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > > On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > The patch looks good to me, I have rebased 0002 atop > > this patch and also done some cosmetic fixes in 0002. > > Here are some comments for the 0002 patch. > > 1) > > - * toplevel transaction. Each subscription has a separate set of files. > + * toplevel transaction. Each subscription has a separate files. > > a separate files => a separate file Done > 2) > + * Otherwise, just open it file for writing, in append mode. > */ > > open it file => open the file Done > > 3) > if (subxact_data.nsubxacts == 0) > { > - if (ent->subxact_fileset) > - { > - cleanup_subxact_info(); > - FileSetDeleteAll(ent->subxact_fileset); > - pfree(ent->subxact_fileset); > - ent->subxact_fileset = NULL; > - } > + cleanup_subxact_info(); > + BufFileDeleteFileSet(stream_fileset, path, true); > + > > Before applying the patch, the code only invoke cleanup_subxact_info() when the > file exists. After applying the patch, it will invoke cleanup_subxact_info() > either the file exists or not. Is it correct ? I think this is just structure resetting at the end of the stream. Earlier the hash was telling us whether we have ever dirtied that structure or not but now we are not maintaining that hash so we just reset it at the end of the stream. I don't think its any bad, in fact I think this is much cheaper compared to maining the hash. > > 4) > /* > - * If this is the first streamed segment, the file must not exist, so make > - * sure we're the ones creating it. Otherwise just open the file for > - * writing, in append mode. > + * If this is the first streamed segment, create the changes file. > + * Otherwise, just open it file for writing, in append mode. > */ > if (first_segment) > - { > ... > - if (found) > - ereport(ERROR, > - (errcode(ERRCODE_PROTOCOL_VIOLATION), > - errmsg_internal("incorrect first-segment flag for streamed replication transaction"))); > ... > - } > + stream_fd = BufFileCreateFileSet(stream_fileset, path); > > > Since the function BufFileCreateFileSet() doesn't check the file's existence, > the change here seems remove the file existence check which the old code did. Not really, we were just doing a sanity check of the in memory hash entry, now we don't maintain that so we don't need to do that. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-30T01:12:07Z
On Fri, Aug 27, 2021 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Aug 26, 2021 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> >> Thanks, Sawada-San and Dilip for confirmation. I would like to commit >> this and the second patch (the second one still needs some more >> testing and review) for PG-15 as there is no bug per-se related to >> this work in PG-14 but I see an argument to commit this for PG-14 to >> keep the code (APIs) consistent. What do you think? Does anybody else >> have any opinion on this? >> > > IMHO, this is a fair amount of refactoring and this is actually an improvement patch so we should push it to v15. I think so too. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-30T06:15:27Z
On Fri, Aug 27, 2021 at 3:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Aug 27, 2021 at 10:56 AM houzj.fnst@fujitsu.com > <houzj.fnst@fujitsu.com> wrote: > > > > On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > The patch looks good to me, I have rebased 0002 atop > > > this patch and also done some cosmetic fixes in 0002. > > > > Here are some comments for the 0002 patch. > > > > 1) > > > > - * toplevel transaction. Each subscription has a separate set of files. > > + * toplevel transaction. Each subscription has a separate files. > > > > a separate files => a separate file > > Done > > > 2) > > + * Otherwise, just open it file for writing, in append mode. > > */ > > > > open it file => open the file > > Done > > > > > 3) > > if (subxact_data.nsubxacts == 0) > > { > > - if (ent->subxact_fileset) > > - { > > - cleanup_subxact_info(); > > - FileSetDeleteAll(ent->subxact_fileset); > > - pfree(ent->subxact_fileset); > > - ent->subxact_fileset = NULL; > > - } > > + cleanup_subxact_info(); > > + BufFileDeleteFileSet(stream_fileset, path, true); > > + > > > > Before applying the patch, the code only invoke cleanup_subxact_info() when the > > file exists. After applying the patch, it will invoke cleanup_subxact_info() > > either the file exists or not. Is it correct ? > > I think this is just structure resetting at the end of the stream. > Earlier the hash was telling us whether we have ever dirtied that > structure or not but now we are not maintaining that hash so we just > reset it at the end of the stream. I don't think its any bad, in fact > I think this is much cheaper compared to maining the hash. > > > > > 4) > > /* > > - * If this is the first streamed segment, the file must not exist, so make > > - * sure we're the ones creating it. Otherwise just open the file for > > - * writing, in append mode. > > + * If this is the first streamed segment, create the changes file. > > + * Otherwise, just open it file for writing, in append mode. > > */ > > if (first_segment) > > - { > > ... > > - if (found) > > - ereport(ERROR, > > - (errcode(ERRCODE_PROTOCOL_VIOLATION), > > - errmsg_internal("incorrect first-segment flag for streamed replication transaction"))); > > ... > > - } > > + stream_fd = BufFileCreateFileSet(stream_fileset, path); > > > > > > Since the function BufFileCreateFileSet() doesn't check the file's existence, > > the change here seems remove the file existence check which the old code did. > > Not really, we were just doing a sanity check of the in memory hash > entry, now we don't maintain that so we don't need to do that. Thank you for updating the patch! The patch looks good to me except for the below comment: + /* Delete the subxact file, if it exist. */ + subxact_filename(path, subid, xid); + BufFileDeleteFileSet(stream_fileset, path, true); s/it exist/it exists/ Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/ -
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-30T06:44:09Z
On Mon, Aug 30, 2021 at 6:42 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > On Fri, Aug 27, 2021 at 2:25 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Thu, Aug 26, 2021 at 2:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > >> > >> > >> Thanks, Sawada-San and Dilip for confirmation. I would like to commit > >> this and the second patch (the second one still needs some more > >> testing and review) for PG-15 as there is no bug per-se related to > >> this work in PG-14 but I see an argument to commit this for PG-14 to > >> keep the code (APIs) consistent. What do you think? Does anybody else > >> have any opinion on this? > >> > > > > IMHO, this is a fair amount of refactoring and this is actually an improvement patch so we should push it to v15. > > I think so too. > I have pushed the first patch in this series. -- With Regards, Amit Kapila.
-
RE: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2021-08-31T02:08:59Z
From Mon, Aug 30, 2021 2:15 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Fri, Aug 27, 2021 at 3:34 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Aug 27, 2021 at 10:56 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote: > > > > > > On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > > The patch looks good to me, I have rebased 0002 atop this patch > > > > and also done some cosmetic fixes in 0002. > > Thank you for updating the patch! > > The patch looks good to me except for the below comment: > > + /* Delete the subxact file, if it exist. */ > + subxact_filename(path, subid, xid); > + BufFileDeleteFileSet(stream_fileset, path, true); > > s/it exist/it exists/ Except for Sawada-san's comment, the v6-0002 patch looks good to me. Best regards, Hou zj
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-08-31T05:53:16Z
On Tue, Aug 31, 2021 at 7:39 AM houzj.fnst@fujitsu.com < houzj.fnst@fujitsu.com> wrote: > From Mon, Aug 30, 2021 2:15 PM Masahiko Sawada <sawada.mshk@gmail.com> > wrote: > > On Fri, Aug 27, 2021 at 3:34 PM Dilip Kumar <dilipbalaut@gmail.com> > wrote: > > > > > > On Fri, Aug 27, 2021 at 10:56 AM houzj.fnst@fujitsu.com < > houzj.fnst@fujitsu.com> wrote: > > > > > > > > On Thu, Aug 26, 2021 2:18 PM Dilip Kumar <dilipbalaut@gmail.com> > wrote: > > > > > > > > > The patch looks good to me, I have rebased 0002 atop this patch > > > > > and also done some cosmetic fixes in 0002. > > > > Thank you for updating the patch! > > > > The patch looks good to me except for the below comment: > > > > + /* Delete the subxact file, if it exist. */ > > + subxact_filename(path, subid, xid); > > + BufFileDeleteFileSet(stream_fileset, path, true); > > > > s/it exist/it exists/ > > Except for Sawada-san's comment, the v6-0002 patch looks good to me. > Thanks, I will wait for a day to see if there are any other comments on this, after that I will fix this one issue and post the updated patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-08-31T09:50:04Z
On Fri, Aug 27, 2021 at 12:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > Few comments on v6-0002* ========================= 1. -BufFileDeleteFileSet(FileSet *fileset, const char *name) +BufFileDeleteFileSet(FileSet *fileset, const char *name, bool missing_ok) { char segment_name[MAXPGPATH]; int segment = 0; @@ -358,7 +369,7 @@ BufFileDeleteFileSet(FileSet *fileset, const char *name) for (;;) { FileSetSegmentName(segment_name, name, segment); - if (!FileSetDelete(fileset, segment_name, true)) + if (!FileSetDelete(fileset, segment_name, !missing_ok)) I don't think the usage of missing_ok is correct here. If you see FileSetDelete->PathNameDeleteTemporaryFile, it already tolerates that the file doesn't exist but gives an error only when it is unable to link. So, with this missing_ok users (say like worker.c) won't even get errors when they are not able to remove files whereas I think the need for the patch is to not get an error when the file doesn't exist. I think you don't need to change anything in the way we invoke FileSetDelete. 2. -static HTAB *xidhash = NULL; +static FileSet *stream_fileset = NULL; Can we keep this in LogicalRepWorker and initialize it accordingly? 3. + /* Open the subxact file, if it does not exist, create it. */ + fd = BufFileOpenFileSet(stream_fileset, path, O_RDWR, true); + if (fd == NULL) + fd = BufFileCreateFileSet(stream_fileset, path); I think retaining the existing comment: "Create the subxact file if it not already created, otherwise open the existing file." seems better here. 4. /* - * If there is no subtransaction then nothing to do, but if already have - * subxact file then delete that. + * If there are no subtransactions, there is nothing to be done, but if + * subxacts already exist, delete it. */ How about changing the above comment to something like: "Delete the subxacts file, if exists"? 5. Can we slightly change the commit message as: Optimize fileset usage in apply worker. Use one fileset for the entire worker lifetime instead of using separate filesets for each streaming transaction. Now, the changes/subxacts files for every streaming transaction will be created under the same fileset and the files will be deleted after the transaction is completed. This patch extends the BufFileOpenFileSet and BufFileDeleteFileSet APIs to allow users to specify whether to give an error on missing files. -- With Regards, Amit Kapila. -
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-09-01T08:23:05Z
On Tue, Aug 31, 2021 at 3:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Fri, Aug 27, 2021 at 12:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > Few comments on v6-0002* > ========================= > 1. > -BufFileDeleteFileSet(FileSet *fileset, const char *name) > +BufFileDeleteFileSet(FileSet *fileset, const char *name, bool missing_ok) > { > char segment_name[MAXPGPATH]; > int segment = 0; > @@ -358,7 +369,7 @@ BufFileDeleteFileSet(FileSet *fileset, const char *name) > for (;;) > { > FileSetSegmentName(segment_name, name, segment); > - if (!FileSetDelete(fileset, segment_name, true)) > + if (!FileSetDelete(fileset, segment_name, !missing_ok)) > > I don't think the usage of missing_ok is correct here. If you see > FileSetDelete->PathNameDeleteTemporaryFile, it already tolerates that > the file doesn't exist but gives an error only when it is unable to > link. So, with this missing_ok users (say like worker.c) won't even > get errors when they are not able to remove files whereas I think the > need for the patch is to not get an error when the file doesn't exist. > I think you don't need to change anything in the way we invoke > FileSetDelete. Right, fixed. > 2. > -static HTAB *xidhash = NULL; > +static FileSet *stream_fileset = NULL; > > Can we keep this in LogicalRepWorker and initialize it accordingly? Done > 3. > + /* Open the subxact file, if it does not exist, create it. */ > + fd = BufFileOpenFileSet(stream_fileset, path, O_RDWR, true); > + if (fd == NULL) > + fd = BufFileCreateFileSet(stream_fileset, path); > > I think retaining the existing comment: "Create the subxact file if it > not already created, otherwise open the existing file." seems better > here. Done > 4. > /* > - * If there is no subtransaction then nothing to do, but if already have > - * subxact file then delete that. > + * If there are no subtransactions, there is nothing to be done, but if > + * subxacts already exist, delete it. > */ > > How about changing the above comment to something like: "Delete the > subxacts file, if exists"? Done > 5. Can we slightly change the commit message as: > Optimize fileset usage in apply worker. > > Use one fileset for the entire worker lifetime instead of using > separate filesets for each streaming transaction. Now, the > changes/subxacts files for every streaming transaction will be created > under the same fileset and the files will be deleted after the > transaction is completed. > > This patch extends the BufFileOpenFileSet and BufFileDeleteFileSet > APIs to allow users to specify whether to give an error on missing > files. Done -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com -
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-09-01T11:53:13Z
On Wed, Sep 1, 2021 at 1:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > The latest patch looks good to me. I have made some changes in the comments, see attached. I am planning to push this tomorrow unless you or others have any comments on it. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Dilip Kumar <dilipbalaut@gmail.com> — 2021-09-01T12:02:51Z
On Wed, Sep 1, 2021 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Sep 1, 2021 at 1:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > The latest patch looks good to me. I have made some changes in the > comments, see attached. I am planning to push this tomorrow unless you > or others have any comments on it. > These comments changes look good to me. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Amit Kapila <amit.kapila16@gmail.com> — 2021-09-02T05:36:28Z
On Wed, Sep 1, 2021 at 5:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Sep 1, 2021 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Wed, Sep 1, 2021 at 1:53 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: >> > >> >> The latest patch looks good to me. I have made some changes in the >> comments, see attached. I am planning to push this tomorrow unless you >> or others have any comments on it. > > > These comments changes look good to me. > Pushed. -- With Regards, Amit Kapila.
-
Re: Separate out FileSet from SharedFileSet (was Re: pgsql: pgstat: Bring up pgstat in BaseInit() to fix uninitialized use o)
Andres Freund <andres@anarazel.de> — 2021-11-29T18:10:51Z
Hi, Thank for all the work on this topic - I'd somehow accidentally marked this thread as read when coming back from vacation... Greetings, Andres Freund