MIME-Version: 1.0
In-Reply-To: <552FA38F.9060005@iki.fi>
References: <548AF1CB.80702@vmware.com>
 <689EB259-44C2-4820-B901-4F6B1C55A1E4@simply.name>
 <549083D6.1000301@vmware.com>
 <CAHGQGwGJzp-QS7BODiv1uc291gAKtjzzCPb_nzUTxYKJhLsUCA@mail.gmail.com>
 <54949108.3030109@vmware.com>
 <CAEyp7J9Hy8Q__FbGeR5skjk7d0dvLC+KLXB3JUuWrXXdJ5O+Wg@mail.gmail.com>
 <552FA38F.9060005@iki.fi>
Date: Tue, 21 Apr 2015 15:53:22 +0900
Message-ID: 
 <CAB7nPqQE179yogtg+nKvdwt9KROxTyt-EjumKOMbuXQtea5r3w@mail.gmail.com>
Subject: Re: Streaming replication and WAL archive interactions
From: Michael Paquier <michael.paquier@gmail.com>
To: hlinnaka@iki.fi
Cc: Venkata Balaji N <nag1010@gmail.com>, Andres Freund <andres@anarazel.de>,
 Fujii Masao <masao.fujii@gmail.com>, Borodin Vladimir <root@simply.name>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary=001a11c128b0a96a4f0514368098
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--001a11c128b0a96a4f0514368098
Content-Type: text/plain; charset=ISO-8859-1

On Thu, Apr 16, 2015 at 8:57 PM, Heikki Linnakangas wrote:
> Oh, hang on, that's not necessarily true. On promotion, the standby
archives
> the last, partial WAL segment from the old timeline. That's just wrong
> (http://www.postgresql.org/message-id/52FCD37C.3070806@vmware.com), and in
> fact I somehow thought I changed that already, but apparently not. So
let's
> stop doing that.

Er. Are you planning to prevent the standby from archiving the last partial
segment from the old timeline at promotion? I thought from previous
discussions that we should do it as master (be it crashed, burned, burried
or dead) may not have the occasion to do it. By preventing its archiving
you close the door to the case where master did not have the occasion to
archive it.

+/* */
+static char primary_last_archived[MAX_XFN_CHARS + 1];
This is visibly missing a comment.

As primary_last_archived is used only by ProcessArchivalReport(), wouldn't
it be better to pass it as argument to this function?

+       /* Check that the filename the primary reported looks valid */
+       if (strlen(primary_last_archived) < 24 ||
+               strspn(primary_last_archived, "0123456789ABCDEF") != 24)
+               return;
Not related to this patch, but we had better have a macro doing this job I
think... It keeps spreading around.

People may be surprised that a base backup taken from a node that has
archive_mode = on set (that's the case in a very large number of cases)
will not be able to work as-is as node startup will fail as follows:
FATAL:  archive_mode='on' cannot be used in archive recovery
HINT:  Use 'shared' or 'always' mode instead.
One idea would be to simply ignore the fact that archive_mode = on on nodes
in recovery instead of dropping an error. Note that I like the fact that it
drops an error as that's clear, I just point the fact that people may be
surprised that base backups are not working anymore now in this case.

Are both WalSndArchivalReport() and WalSndArchivalReportIfNecessary()
really necessary? I think that for simplicity you could merge them and use
last_archival_report as a local variable.

Creating a dependency between the pgstat machinery and the WAL sender looks
weak to me. For example with this patch a master cannot stop, as it waits
indefinitely:
LOG:  using stale statistics instead of current ones because stats
collector is not responding
LOG:  sending archival report:
You could scan archive_status/ but that would be costly if there are many
entries to scan and I think that walsender should be highly responsive. Or
you could directly store the name of the lastly archived WAL segment marked
as .done in let's say archive_status/last_archived. An entry for that in
the control file does not seem the right place as a node may not have
archive_mode enabled that's why I am not mentioning it.

Regards,
-- 
Michael

--001a11c128b0a96a4f0514368098
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><br><br>On Thu, Apr 16, 2015 at 8:57 PM, Heikki Linnakanga=
s wrote:<br>&gt; Oh, hang on, that&#39;s not necessarily true. On promotion=
, the standby archives<br>&gt; the last, partial WAL segment from the old t=
imeline. That&#39;s just wrong<br>&gt; (<a href=3D"http://www.postgresql.or=
g/message-id/52FCD37C.3070806@vmware.com">http://www.postgresql.org/message=
-id/52FCD37C.3070806@vmware.com</a>), and in<br>&gt; fact I somehow thought=
 I changed that already, but apparently not. So let&#39;s<br>&gt; stop doin=
g that.<br><br>Er. Are you planning to prevent the standby from archiving t=
he last partial segment from the old timeline at promotion? I thought from =
previous discussions that we should do it as master (be it crashed, burned,=
 burried or dead) may not have the occasion to do it. By preventing its arc=
hiving you close the door to the case where master did not have the occasio=
n to archive it.<br><br>+/* */<br>+static char primary_last_archived[MAX_XF=
N_CHARS + 1];<br>This is visibly missing a comment.<br><br>As primary_last_=
archived is used only by ProcessArchivalReport(), wouldn&#39;t it be better=
 to pass it as argument to this function?<br><br>+ =A0 =A0 =A0 /* Check tha=
t the filename the primary reported looks valid */<br>+ =A0 =A0 =A0 if (str=
len(primary_last_archived) &lt; 24 ||<br>+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 strs=
pn(primary_last_archived, &quot;0123456789ABCDEF&quot;) !=3D 24)<br>+ =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 return;<br>Not related to this patch, but we had be=
tter have a macro doing this job I think... It keeps spreading around.<br><=
br>People may be surprised that a base backup taken from a node that has ar=
chive_mode =3D on set (that&#39;s the case in a very large number of cases)=
 will not be able to work as-is as node startup will fail as follows:<br>FA=
TAL: =A0archive_mode=3D&#39;on&#39; cannot be used in archive recovery<br>H=
INT: =A0Use &#39;shared&#39; or &#39;always&#39; mode instead.<br>One idea =
would be to simply ignore the fact that archive_mode =3D on on nodes in rec=
overy instead of dropping an error. Note that I like the fact that it drops=
 an error as that&#39;s clear, I just point the fact that people may be sur=
prised that base backups are not working anymore now in this case.<br><br>A=
re both WalSndArchivalReport() and WalSndArchivalReportIfNecessary() really=
 necessary? I think that for simplicity you could merge them and use last_a=
rchival_report as a local variable.<br><br>Creating a dependency between th=
e pgstat machinery and the WAL sender looks weak to me. For example with th=
is patch a master cannot stop, as it waits indefinitely:<br>LOG: =A0using s=
tale statistics instead of current ones because stats collector is not resp=
onding<br>LOG: =A0sending archival report: <br>You could scan archive_statu=
s/ but that would be costly if there are many entries to scan and I think t=
hat walsender should be highly responsive. Or you could directly store the =
name of the lastly archived WAL segment marked as .done in let&#39;s say ar=
chive_status/last_archived. An entry for that in the control file does not =
seem the right place as a node may not have archive_mode enabled that&#39;s=
 why I am not mentioning it.<br><br>Regards,<br>-- <br>Michael</div>

--001a11c128b0a96a4f0514368098--