Re: thinko in basic_archive.c

2022-11-09 Thread Bharath Rupireddy
On Tue, Nov 8, 2022 at 3:18 AM Nathan Bossart wrote: > > The call to snprintf() should take care of adding a terminating null byte > in the right place. Ah, my bad. MemSet is avoided in v5 patch setting only the first byte. > > So, IIUC, your point here is what if the copy_file fails to create t

Re: thinko in basic_archive.c

2022-11-07 Thread Nathan Bossart
On Mon, Nov 07, 2022 at 04:53:35PM +0530, Bharath Rupireddy wrote: > The tempfile name can vary in size for the simple reason that a pid > can be of varying digits - for instance, tempfile name is 'foo1234' > (pid being 1234) and it becomes '\'\0\'oo1234' if we just reset the > first char to '\0' a

Re: thinko in basic_archive.c

2022-11-07 Thread Bharath Rupireddy
On Sun, Nov 6, 2022 at 3:17 AM Nathan Bossart wrote: > > On Fri, Oct 21, 2022 at 09:30:16PM +0530, Bharath Rupireddy wrote: > > On Fri, Oct 21, 2022 at 10:43 AM Kyotaro Horiguchi > > wrote: > >> Thanks, but we don't need to wipe out the all bytes. Just putting \0 > >> at the beginning of the buff

Re: thinko in basic_archive.c

2022-11-05 Thread Nathan Bossart
On Fri, Oct 21, 2022 at 09:30:16PM +0530, Bharath Rupireddy wrote: > On Fri, Oct 21, 2022 at 10:43 AM Kyotaro Horiguchi > wrote: >> Thanks, but we don't need to wipe out the all bytes. Just putting \0 >> at the beginning of the buffer is sufficient. > > Nah, that's not a clean way IMO. Why not?

Re: thinko in basic_archive.c

2022-10-21 Thread Bharath Rupireddy
On Fri, Oct 21, 2022 at 10:43 AM Kyotaro Horiguchi wrote: > > Thanks, but we don't need to wipe out the all bytes. Just putting \0 > at the beginning of the buffer is sufficient. Nah, that's not a clean way IMO. > And the Memset() at the > beginning of basic_archive_file_internal is not needed s

Re: thinko in basic_archive.c

2022-10-20 Thread Kyotaro Horiguchi
Sorry, the previous mail are sent inadvertently.. At Fri, 21 Oct 2022 14:13:46 +0900 (JST), Kyotaro Horiguchi wrote in > + expectation that a value will soon be provided. Care must be taken when > + multiple servers are archiving to the same > + basic_archive.archive_library dire

Re: thinko in basic_archive.c

2022-10-20 Thread Kyotaro Horiguchi
Hi. Anyway, on second thought, lager picture than just adding the post-process-end callback would out of the scope of this patch. So I write some comments on the patch first, then discussion the rest. Thu, 20 Oct 2022 13:29:12 +0530, Bharath Rupireddy wrote in > > No. I didn't mean that, If s

Re: thinko in basic_archive.c

2022-10-20 Thread Bharath Rupireddy
On Thu, Oct 20, 2022 at 6:57 AM Kyotaro Horiguchi wrote: > > > The archive module must be responsible for cleaning up the temp file > > that it creates. One way to do it is in the archive module's shutdown > > callback, this covers most of the cases, but not all. > > True. But I agree to Robert th

Re: thinko in basic_archive.c

2022-10-19 Thread Kyotaro Horiguchi
At Wed, 19 Oct 2022 10:21:12 +0530, Bharath Rupireddy wrote in > On Wed, Oct 19, 2022 at 8:58 AM Kyotaro Horiguchi > wrote: > > An archive module could clean up them at startup or at the first call > > but that might be dangerous (since archive directory is I think > > thought as external resou

Re: thinko in basic_archive.c

2022-10-19 Thread Kyotaro Horiguchi
At Wed, 19 Oct 2022 10:48:03 -0400, Robert Haas wrote in > On Tue, Oct 18, 2022 at 11:28 PM Kyotaro Horiguchi > wrote: > > > > Yeah, leaving a potentially unbounded number of files around after > > > > system crashes seems pretty unfriendly. I'm not sure how to fix that, > > > > exactly. > > >

Re: thinko in basic_archive.c

2022-10-19 Thread Robert Haas
On Tue, Oct 18, 2022 at 11:28 PM Kyotaro Horiguchi wrote: > > > Yeah, leaving a potentially unbounded number of files around after > > > system crashes seems pretty unfriendly. I'm not sure how to fix that, > > > exactly. > > Unbounded number of sequential crash-restarts itself is a more serious >

Re: thinko in basic_archive.c

2022-10-18 Thread Bharath Rupireddy
On Wed, Oct 19, 2022 at 8:58 AM Kyotaro Horiguchi wrote: > > Unbounded number of sequential crash-restarts itself is a more serious > problem.. Agree. The basic_archive module currently leaves temp files around even for normal restarts of the cluster, which is bad IMO. > An archive module could

Re: thinko in basic_archive.c

2022-10-18 Thread Kyotaro Horiguchi
+0530, Bharath Rupireddy wrote in > On Mon, Oct 17, 2022 at 6:45 PM Robert Haas wrote: > > > > On Fri, Oct 14, 2022 at 4:45 AM Bharath Rupireddy > > wrote: > > > What happens to the left-over temp files after a server crash? Will > > > they be lying around in the archive directory? I understan

Re: thinko in basic_archive.c

2022-10-18 Thread Bharath Rupireddy
On Mon, Oct 17, 2022 at 6:45 PM Robert Haas wrote: > > On Fri, Oct 14, 2022 at 4:45 AM Bharath Rupireddy > wrote: > > What happens to the left-over temp files after a server crash? Will > > they be lying around in the archive directory? I understand that we > > can't remove such files because we

Re: thinko in basic_archive.c

2022-10-17 Thread Robert Haas
On Fri, Oct 14, 2022 at 4:45 AM Bharath Rupireddy wrote: > What happens to the left-over temp files after a server crash? Will > they be lying around in the archive directory? I understand that we > can't remove such files because we can't distinguish left-over files > from a crash and the temp fi

Re: thinko in basic_archive.c

2022-10-16 Thread Michael Paquier
On Sat, Oct 15, 2022 at 02:10:26PM -0700, Nathan Bossart wrote: > On Sat, Oct 15, 2022 at 10:19:05AM +0530, Bharath Rupireddy wrote: >> Can you please help me understand how name collisions can happen with >> temp file names including WAL file name, timestamp to millisecond >> scale, and PID? Havin

Re: thinko in basic_archive.c

2022-10-15 Thread Nathan Bossart
On Sat, Oct 15, 2022 at 10:19:05AM +0530, Bharath Rupireddy wrote: > Can you please help me understand how name collisions can happen with > temp file names including WAL file name, timestamp to millisecond > scale, and PID? Having the timestamp is enough to provide a non-unique > temp file name wh

Re: thinko in basic_archive.c

2022-10-14 Thread Bharath Rupireddy
On Sat, Oct 15, 2022 at 12:03 AM Nathan Bossart wrote: > > On Fri, Oct 14, 2022 at 02:15:19PM +0530, Bharath Rupireddy wrote: > > Given that temp file name includes WAL file name, epoch to > > milliseconds scale and MyProcPid, can there be name collisions after a > > server crash or even when mult

Re: thinko in basic_archive.c

2022-10-14 Thread Nathan Bossart
On Fri, Oct 14, 2022 at 02:15:19PM +0530, Bharath Rupireddy wrote: > Given that temp file name includes WAL file name, epoch to > milliseconds scale and MyProcPid, can there be name collisions after a > server crash or even when multiple servers with different pids are > archiving/copying the same

Re: thinko in basic_archive.c

2022-10-14 Thread Bharath Rupireddy
On Fri, Oct 14, 2022 at 10:11 AM Nathan Bossart wrote: > > I intended for the temporary file name generated by basic_archive.c to I'm trying to understand this a bit: /* * Pick a sufficiently unique name for the temporary file so that a * collision is unlikely. This helps avoid pr

thinko in basic_archive.c

2022-10-13 Thread Nathan Bossart
Hi hackers, I intended for the temporary file name generated by basic_archive.c to include the current timestamp so that the name was "sufficiently unique." Of course, this could also be used to determine the creation time, but you can just as easily use stat(1) for that. In any case, I forgot to