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

On 6 March 2018 at 16:07, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:

> On 6 March 2018 at 09:58, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>
>> On 5 March 2018 at 23:25, David Steele <david(at)pgmasters(dot)net> wrote:
>>
>>> Hi Craig,
>>>
>>> On 1/21/18 5:45 PM, Craig Ringer wrote:
>>> > On 6 January 2018 at 08:28, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org
>>> > <mailto:alvherre(at)alvh(dot)no-ip(dot)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