Обсуждение: initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>   * Launch a VS x86 command prompt
>   * devenv /debugexe bin\initdb.exe -D test
>   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>   * Run
>   * When it traps at get_restricted_token(), manually move the execution
> pointer over the setup of the restricted execution token by dragging &
> dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
> comment it out and rebuild, but I was working with a supplied binary.
>   * Continue until next breakpoint
>   * Launch process explorer and find the pid of the postgres child process
>   * Debug->attach to process, attach to the child postgres. This doesn't
> detach the parent, VS does multiprocess debugging.
>   * Continue execution
>   * vs will trap on the child when it crashes

Do you think a crash dump could have been created by creating
crashdumps/ in PGDATA as part of initdb before this query is run?
-- 
Michael

On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Yes, quite possibly, actually. I should've just got Haroon to build me a new
> initdb without the priv setting and with creation of crashdumps/ .
>
> It might be worth testing that out and adding an initdb startup flag to
> create the directory, since initdb is such a PITA to debug.

I was more thinking about putting that under -DDEBUG for example.
-- 
Michael

> From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
> Sent: Friday, June 24, 2016 11:37 AM
> On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer <craig@2ndquadrant.com>
> wrote:
> It might be worth testing that out and adding an initdb startup flag
> > to create the directory, since initdb is such a PITA to debug.
> 
> I was more thinking about putting that under -DDEBUG for example.
> 

I think just the existing option -d (--debug) and/or -n (--no-clean) would be OK.

Regards
Takayuki Tsunakawa

On Fri, Jun 24, 2016 at 11:51 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
>> From: pgsql-hackers-owner@postgresql.org
>> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
>> Sent: Friday, June 24, 2016 11:37 AM
>> On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer <craig@2ndquadrant.com>
>> wrote:
>> It might be worth testing that out and adding an initdb startup flag
>> > to create the directory, since initdb is such a PITA to debug.
>>
>> I was more thinking about putting that under -DDEBUG for example.
>>
>
> I think just the existing option -d (--debug) and/or -n (--no-clean) would be OK.

If the majority thinks that an option switch is more adapted, I won't
fight it strongly. Just please let's not mess up with the behavior of
the existing options.
-- 
Michael

On Fri, Jun 24, 2016 at 1:28 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Given that it's only been seen in VS 2013, it's particularly odd that it's
> not biting woodlouse.
>
> I'd like more details from those whose installs are crashing. What exact
> vcvars env did you run under, with which exact cl.exe version?

Which OS did you use for the compilation? I don't think that this
matters much but woodloose is using Win7.
-- 
Michael

On Fri, Jun 24, 2016 at 3:22 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>
>
> On 24 June 2016 at 10:28, Michael Paquier <michael.paquier@gmail.com> wrote:
>>
>> On Fri, Jun 24, 2016 at 11:21 AM, Craig Ringer <craig@2ndquadrant.com>
>> wrote:
>> >   * Launch a VS x86 command prompt
>> >   * devenv /debugexe bin\initdb.exe -D test
>> >   * Set a breakpoint in initdb.c:3557 and initdb.c:3307
>> >   * Run
>> >   * When it traps at get_restricted_token(), manually move the execution
>> > pointer over the setup of the restricted execution token by dragging &
>> > dropping the yellow instruction pointer arrow. Yes, really. Or, y'know,
>> > comment it out and rebuild, but I was working with a supplied binary.
>> >   * Continue until next breakpoint
>> >   * Launch process explorer and find the pid of the postgres child
>> > process
>> >   * Debug->attach to process, attach to the child postgres. This doesn't
>> > detach the parent, VS does multiprocess debugging.
>> >   * Continue execution
>> >   * vs will trap on the child when it crashes
>>
>> Do you think a crash dump could have been created by creating
>> crashdumps/ in PGDATA as part of initdb before this query is run?
>
>
>
> The answer is "yes" btw. Add "crashdumps" to the static array of directories
> created by initdb and it works great.

As simple as attached..

> Sigh. It'd be less annoying if I hadn't written most of the original patch.

You mean the patch that created the crashdumps/ trick? This has saved
me a couple of months back to analyze a problem TBH.
--
Michael

Craig Ringer <craig@2ndquadrant.com> writes:
> I have absolutely no idea why it's trying to access memory at what looks
> like   (uint64)(-1) though.  Nothing in the auto vars list:

> + &restrictlist 0x000000000043f7b0 {0x0000000009e32600 {type=T_List (656)
> length=1 head=0x0000000009e325e0 {data={ptr_value=...} ...} ...}} List * *
> + inner_rel 0x0000000009e7ad68 {type=T_EquivalenceClass (537)
> reloptkind=RELOPT_BASEREL (0) relids=0x0000000009e30520 {...} ...} RelOptInfo
> *
> + inner_rel->relids 0x0000000009e30520 {nwords=658 words=0x0000000009e30524
> {...} } Bitmapset *
> + outer_rel 0x00000001401dec98
> {postgres.exe!build_joinrel_tlist(PlannerInfo * root, RelOptInfo * joinrel,
> RelOptInfo * input_rel), Line 646} {...} RelOptInfo *
> + outer_rel->relids 0xe808498b48d78b48 {nwords=??? words=0xe808498b48d78b4c
> {...} } Bitmapset *
> + sjinfo 0x000000000043f870 {type=T_SpecialJoinInfo (543)
> min_lefthand=0x0000000009e7abd0 {nwords=1 words=0x0000000009e7abd4 {...} }
> ...} SpecialJoinInfo *


inner_rel seems to be pointing at garbage, or at least why is the
referenced object tag T_EquivalenceClass not T_RelOptInfo?  And
why aren't we being given anything for outer_rel?  The value for
outer_rel->relids isn't inspiring any confidence either, and
for that matter inner_rel->relids couldn't possibly have more than
nwords==1 given how simple the query is.  In short, either the
debugger is totally confused or the code is, because most of these
pointers aren't pointing at anything sane.

TBH, this looks more like a compiler bug than anything else.  I wonder
whether it's getting confused by taking the address of a parameter
(although surely we do that elsewhere).

It would be worth recompiling at -O0, or whatever the local equivalent
of that is, to see if (1) the crash goes away or (2) the debugger's
printouts get any more reliable.
        regards, tom lane

Craig Ringer <craig@2ndquadrant.com> writes:
> On 24 June 2016 at 21:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH, this looks more like a compiler bug than anything else.

> I tend to agree. Especially since valgrind has no complaints on x64 linux,
> and neither does DrMemory for 32-bit builds with the same toolchain on the
> same Windows and same SDK.

If that is the explanation, I'm suspicious that it's got something to do
with the interaction of a static inline-able (single-call-site) function
and taking the address of a formal parameter.  We certainly have multiple
other instances of each thing, but maybe not both at the same place.
This leads to a couple of suggestions for dodging the problem:

1. Make get_foreign_key_join_selectivity non-static so that it doesn't
get inlined, along the lines of
                       List *restrictlist);
-static Selectivity get_foreign_key_join_selectivity(PlannerInfo *root,
+extern Selectivity get_foreign_key_join_selectivity(PlannerInfo *root,                             Relids
outer_relids,
... */
-static Selectivity
+Selectivityget_foreign_key_join_selectivity(PlannerInfo *root,

2. Don't pass the original formal parameter to
get_foreign_key_join_selectivity, ie do something like
static doublecalc_joinrel_size_estimate(PlannerInfo *root,                           RelOptInfo *outer_rel,
             RelOptInfo *inner_rel,                           double outer_rows,                           double
inner_rows,                          SpecialJoinInfo *sjinfo,
 
-                            List *restrictlist)
+                            List *orig_restrictlist){    JoinType    jointype = sjinfo->jointype;
+    List       *restrictlist = orig_restrictlist;    Selectivity fkselec;    Selectivity jselec;    Selectivity
pselec;

Obviously, if either of those things do make the problem go away, it's
a compiler bug.  If not, we'll need to dig deeper.
        regards, tom lane

"Haroon ." <contact.mharoon@gmail.com> writes:
> And if I comment these out i.e. setup_description, setup_privileges and
> 'setup_schema' it seem to progress well without any errors/crashes.

Presumably, what you've done there is remove every single join query
from the post-bootstrap scripts.  That isn't particularly useful in
itself, but it does suggest that you would be able to fire up a
normal session afterwards in which you could use a more conventional
debugging approach.  The problem can evidently be categorized as
"planning of any join query whatsoever crashes", so a test case
ought to be easy enough to come by.
        regards, tom lane

"Haroon ." <contact.mharoon@gmail.com> writes:
> On Sat, Jun 25, 2016 at 6:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This leads to a couple of suggestions for dodging the problem:
>> 
>> 2. Don't pass the original formal parameter to
>> get_foreign_key_join_selectivity, ie do something like
>> 
>> static double
>> calc_joinrel_size_estimate(PlannerInfo *root,
>> RelOptInfo *outer_rel,
>> RelOptInfo *inner_rel,
>> double outer_rows,
>> double inner_rows,
>> SpecialJoinInfo *sjinfo,
>> -                                                  List *restrictlist)
>> +                                                  List *orig_restrictlist)
>> {
>> JoinType        jointype = sjinfo->jointype;
>> +       List       *restrictlist = orig_restrictlist;
>> Selectivity fkselec;
>> Selectivity jselec;
>> Selectivity pselec;
>> 
>> 
> The problem appears to be related to 'taking the address of a formal
> parameter'. NOT passing the original formal parameter to
> get_foreign_key_join_selectivity fixes it (dodges the problem) on VS2013.

Thanks for investigating!  I'll go commit that change.  I wish someone
would put up a buildfarm critter using VS2013, though.
        regards, tom lane

Tom Lane wrote:
> "Haroon ." <contact.mharoon@gmail.com> writes:

> > The problem appears to be related to 'taking the address of a formal
> > parameter'. NOT passing the original formal parameter to
> > get_foreign_key_join_selectivity fixes it (dodges the problem) on VS2013.
> 
> Thanks for investigating!  I'll go commit that change.  I wish someone
> would put up a buildfarm critter using VS2013, though.

Uh, isn't that what woodlouse is using?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier wrote:
> On Fri, Jun 24, 2016 at 11:51 AM, Tsunakawa, Takayuki
> <tsunakawa.takay@jp.fujitsu.com> wrote:
> >> From: pgsql-hackers-owner@postgresql.org
> >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
> >> Sent: Friday, June 24, 2016 11:37 AM
> >> On Fri, Jun 24, 2016 at 11:33 AM, Craig Ringer <craig@2ndquadrant.com>
> >> wrote:
> >> > It might be worth testing that out and adding an initdb startup
> >> > flag to create the directory, since initdb is such a PITA to
> >> > debug.
> >>
> >> I was more thinking about putting that under -DDEBUG for example.
> >
> > I think just the existing option -d (--debug) and/or -n (--no-clean)
> > would be OK.
> 
> If the majority thinks that an option switch is more adapted, I won't
> fight it strongly. Just please let's not mess up with the behavior of
> the existing options.

I think creating crashdumps/ when both -d and -n are specified is a bit
odd but reasonable.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Thanks for investigating!  I'll go commit that change.  I wish someone
>> would put up a buildfarm critter using VS2013, though.

> Uh, isn't that what woodlouse is using?

Well, it wasn't reporting this crash, so there's *something* different.
        regards, tom lane

Craig Ringer wrote:
> On 30 June 2016 at 07:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > Tom Lane wrote:
> > >> Thanks for investigating!  I'll go commit that change.  I wish someone
> > >> would put up a buildfarm critter using VS2013, though.
> >
> > > Uh, isn't that what woodlouse is using?
> >
> > Well, it wasn't reporting this crash, so there's *something* different.

> It may only affect the i386 to x86_64 cross compiler. If Woodlouse is using
> native x86_64 compilers perhaps that's why?

Hmm, so what about a pure 32bit build, if such a thing still exists?  If
so and it causes the same crash, perhaps we should have one member for
each VS version running on 32bit x86.

(I note that the coverage of MSVC versions has greatly improved in
recent months.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Craig Ringer wrote:
> On 30 June 2016 at 20:19, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> 
> > Hmm, so what about a pure 32bit build, if such a thing still exists?  If
> > so and it causes the same crash, perhaps we should have one member for
> > each VS version running on 32bit x86.
> 
> It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target. I
> tested that.

Ah, okay.  I doubt it's worth setting up buildfarm members testing all
cross-compiles just to try and catch possible compiler bugs that way, so
unless somebody wants to invest more effort in this area, it seems we're
done here.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 1, 2016 at 9:57 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Craig Ringer wrote:
>> On 30 June 2016 at 20:19, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> > Hmm, so what about a pure 32bit build, if such a thing still exists?  If
>> > so and it causes the same crash, perhaps we should have one member for
>> > each VS version running on 32bit x86.
>>
>> It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target. I
>> tested that.
>
> Ah, okay.  I doubt it's worth setting up buildfarm members testing all
> cross-compiles just to try and catch possible compiler bugs that way, so
> unless somebody wants to invest more effort in this area, it seems we're
> done here.

Sure. To be honest just using the latest version of MSVC available for
the builds is fine I think. Windows is very careful regarding
backward-compatibility of its compiled stuff usually, even if by using
VS2015 you make the builds of Postgres incompatible with XP. But
software is a world that keeps moving on, and XP is already out of
support by Redmond.
-- 
Michael