Re: Immediate standby promotion

On Mon, Sep 1, 2014 at 4:44 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Sep 1, 2014 at 3:23 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > I think there is one downside as well for this proposal that
> > apart from data loss, it can lead to uncommitted data occupying
> > space in database which needs to be later cleaned by vacuum.
> > This can happen with non-immediate promote as well, but the
> > chances with immediate are more. So the gain we got by doing
> > immediate promotion can lead to slow down of operations in some
> > cases. It might be useful if we mention this in docs.
>
> Yep, the immediate promotion might be more likely to cause
> the recovery to end before replaying WAL data of VACUUM. But, OTOH,
> I think that the immediate promotion might be more likely to cause
> the recovery to end before replaying WAL data which will generate
> garbage data.

This seems arguable, because immediate promotion won't allow
WAL data to be replayed completely which means more chance
that only partial data of transactions will be replayed and commit
for those transactions won't get replayed, so it can lead to garbage
data.

> So I'm not sure if it's worth adding that note to the doc.

No issues, I just want to bring this point to your notice so that if
you think it is important enough that we can mention it then we
can update the docs else leave it.

> >
> > Few comments about patch:
> >
> > 1.
> > On standby we will see below message:
> >
> > LOG: received promote request
> >
> > User will always see above message irrespective of whether it
> > is immediate promote or any other mode of promote. I think it will
> > be better to distinguish between different modes and display the
> > appropriate message.
>
> Agreed. So I'm thinking to change the code as follows.
>
> if (immediate_promote)
> ereport(LOG, (errmsg("received immediate promote request")));
> else
> ereport(LOG, (errmsg("received promote request")));

This seems fine to me.

> Or we should name the normal promotion?

No need.

> >
> > 2.
> > StartupXLOG()
> > {
> > ..
> > + if (immediate_promote)
> > + break;
> > ..
> > }
> >
> > Why are you doing this check after pause
> > (recoveryApplyDelay/recoveryPausesHere) for recovery?
> >
> > Why can't we do it after ReadRecord()?
>
> We can do that check either after ReadRecord() or after pause.
> I preferred to add the check after pause because immediate promotion
> would be likely to be requested while recovery is being paused.
> In this case, if we do that check after ReadRecord(), we need to read
> one more WAL record that actually we don't need.

Okay, but for that you need to make sure that pause can detect
promotion request.

> BTW, in the current patch, when immediate promotion is requested while
> recovery is being paused, the recovery keeps being paused until it's
> manually resumed. But immediate promotion should cause even paused
> recovery to end immediately?

Yeap, I also think so.

Another issue with immediate promotion is that currently if primary server
is continuously sending the data, then standby could not detect --immediate
promote request and the reason seems to be below code:
WaitForWALToBecomeAvailable()
{
...
{
/* just make sure source info is correct... */
readSource = XLOG_FROM_STREAM;
XLogReceiptSource = XLOG_FROM_STREAM;
return true;
}
..
if (CheckForStandbyTrigger())
}

Basically we won't check for promote request if the data is available.

I have even reproduced this by below test case:
Primary (session-1) -
1. Create table t1 (c1 int, c2 char(500)) with (fillfactor = 10);

Standby -
2. Configure and start standby
3. Just connect with one client

Primary (session-1) -
4. insert into t1 values (generate_series(1,100000), 'aaaaa');

From another window, run command:
5. pg_ctl promote --immediate -D ..\..\Database1

Run step-4 and step-5 at the same time.

Currently standby is promoted only after insert operation
in step-4 is finished which seems to be wrong.

Apart from above issue, I have one question for you regarding
this feature, currently the patch supports immediate promotion
via pg_ctl promote, however we have another mechanism (trigger_file)
which you have not enhanced to support this new feature. Is there
any reason for same?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com