On 6 March 2018 at 16:07, Craig Ringer <cr...@2ndquadrant.com> wrote:
> On 6 March 2018 at 09:58, Craig Ringer <cr...@2ndquadrant.com> wrote: > >> On 5 March 2018 at 23:25, David Steele <da...@pgmasters.net> wrote: >> >>> Hi Craig, >>> >>> On 1/21/18 5:45 PM, Craig Ringer wrote: >>> > On 6 January 2018 at 08:28, Alvaro Herrera <alvhe...@alvh.no-ip.org >>> > <mailto:alvhe...@alvh.no-ip.org>> wrote: >>> > >>> > I think this should use ReadDirExtended with an elevel less than >>> ERROR, >>> > and do nothing. >>> > >>> > Why have strcmp(.) and strcmp(..)? These are going to be skipped >>> by the >>> > comparison to "xid" prefix anyway. Looks like strcmp processing >>> > power waste. >>> > >>> > Please don't use bare sprintf() -- upgrade to snprintf. >>> > >>> > With this coding, if I put a root-owned file "xidfoo" in a replslot >>> > directory, it will PANIC the server. Is that okay? Why not read >>> the >>> > file name with sscanf(), since we know the precise format it has? >>> Then >>> > we don't need to bother with random crap left around. Maybe a >>> good time >>> > to put the "xid-%u-lsn-%X-%X.snap" literal in a macro? I guess >>> the >>> > rationale is that if you let random people put "xidfoo" files in >>> your >>> > replication slot dirs, you deserve a PANIC anyway, but I'm not >>> sure. >>> > >>> > I'm happy to address those comments. >>> > >>> > The PANIC probably made sense when it was only done on startup, but not >>> > now it's at runtime. >>> > >>> > The rest is mainly retained from the prior code, but it's a good chance >>> > to make those changes. >>> This patch was marked Waiting on Author last December. Do you know when >>> you'll have a chance to provide an updated patch? >>> >>> Given that it's a bug fix it would be good to see a patch for this CF, >>> or very soon after. >>> >>> >> Thanks for the reminder. I'll address it today if I can. >> >> > Revised patch attached. > > I have _not_ rewritten to use sscanf yet. I'll do that next, so you can > choose the fewer-changes patch for backpatching if desired. > > ... and I'm not convinced it's really an improvement. uint32 xid, lsn_high, lsn_low; if (sscanf(spill_de->d_name, REORDERBUFFER_TXN_FILENAME_FORMAT, xid, lsn_high, lsn_low) == 3) { since we don't use the scanned-out information. I guess my answer to causing problems if you create a file named "xidfoo" in a slot directory is yeah, don't do that. Note that this patch changes the PANIC to ERROR. This will promote to FATAL during the startup process, which I think is fine. Objections? -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
