Re: WAL replay bugs

Hello, Let me apologize for continuing the discussion even though
the deadline is approaching.

At Fri, 11 Jul 2014 09:49:55 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqTJEzOz-FotibSEjyG0eaBngx2PLqywoDChYFXzFqYQkg(at)mail(dot)gmail(dot)com>
> 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.

That's awkward, but I agree with it. By the way, I found
PageHeaderData.pd_flags to have 9 bits room. It seems to be
usable if no other usage is in sight right now, if the formal
method to identify page types is worth the 3-4 bits there.

# This is a separate discussion from this patch itself.

> > - "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.

Although I doubt necessity of the flexibility seeing the current
testing framework, I don't have so strong objection about
that. Nevertheless, perhaps you are appreciated to put a notice
on.. README or somewhere.

> > - 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.

Agreed.

> > - 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.

Thank you.

regardes,

--
Kyotaro Horiguchi
NTT Open Source Software Center