Re: Re: BUGFIX: standby disconnect can corrupt serialized reorder buffers

On 6 March 2018 at 09:58, Craig Ringer <craig@2ndquadrant.com> wrote:

On 5 March 2018 at 23:25, David Steele <david@pgmasters.net> wrote:

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.