Re: WAL replay bugs - Mailing list pgsql-hackers

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.paquier@gmail.com> wrote in
<CAB7nPqTJEzOz-FotibSEjyG0eaBngx2PLqywoDChYFXzFqYQkg@mail.gmail.com>
> Updated patches attached.
> 
> On Thu, Jul 10, 2014 at 7:13 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.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