Hi Craig, On 1/21/18 5:45 PM, Craig Ringer wrote: > On 6 January 2018 at 08:28, Alvaro Herrera <alvherre@alvh.no-ip.org > <mailto:alvherre@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?