Re: WAL replay bugs

Updated patches attached.

On Thu, Jul 10, 2014 at 7:13 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> The usage of this is a bit irregular as a (extension) module but
> it is the nature of this 'contrib'. The rearranged page type
> detection logic seems neater and keeps to work as intended. This
> logic will be changed after the new page type detection scheme
> becomes ready by the another patch.

No disk format changes will be allowed just to make page detection
easier (Tom mentioned that earlier in this thread btw). We'll have to
live with what current code offers, especially considering that adding
new bytes for page detection for gin pages would double the size of
its special area after applying MAXALIGN to it.

> - A question mark seems missing from the end of the message "Has
> build been done with -DBUFFER_CAPTURE included in CFLAGS" in
> test.sh.
Fixed.

> - "make check" runs "regress --use-existing" but IMHO the make
> target which is expected to do that is installcheck. I had
> fooled to convince that it should run the postgres which is
> built dedicatedly:(

Yes, the patch is abusing of that. --use-existing is specified in this
case because the test itself is controlling Postgres servers to build
and fetch the buffer captures. This allows more flexible machinery
IMO.

> - postgres processes are left running after
> test_default(custom).sh has stopped halfway. This can be fixed
> with the attached patch, but, to be honest, this seems too
> much. I'll follow your decision whether or not to do this.
> (bufcapt_test_sh_20140710.patch)

I had considered that first, thinking that it was the user
responsibility if things are screwed up with his custom scripts. I
guess that the way you are doing it is a safeguard simple enough
though, so included with some editing, particularly reporting to the
user the error code returned by the test script.

> - test_default.sh is not only an example script which will run
> while utilize this facility, but the test script for this
> facility itself.
> So I think it would be better be added some queries so that all
> possible page types available for the default testing. What do
> you think about the attached patch? (hash index is unlogged
> but I dared to put it for clarity.)

Yeah, having a default set of queries run just to let the user get an
idea of how it works improves things.
Once again thanks for taking the time to look at that.

Regards,
--
Michael