Horton

Thread

Commits

  1. Improve castNode notation by introducing list-extraction-specific variants.

  2. Clean up bugs in clause_selectivity() cleanup.

  3. Add newly-symlinked files to "make clean" target.

  4. Reset API of clause_selectivity()

  1. [sqlsmith] Planner crash on foreign table join

    Andreas Seltenreich <seltenreich@gmx.de> — 2017-04-08T06:45:46Z

    Hi,

testing master at f0e44021df with a loopback postgres_fdw installed, I
see lots of crashes on queries joining foreign tables with various
expressions.  Below is a reduced recipe for the regression database and
a backtrace.

regards,
Andreas

--8<---------------cut here---------------start------------->8---
create extension postgres_fdw;
create server myself foreign data wrapper postgres_fdw;
create schema fdw_postgres;
create user fdw login;
grant all on schema public to fdw;
grant all on all tables in schema public to fdw;
create user mapping for public server myself options (user 'fdw');
import foreign schema public from server myself into fdw_postgres;

explain select from
  fdw_postgres.hslot
    left join fdw_postgres.num_exp_div
    on ((exists (values (1))) and (values (1)) is null);
--8<---------------cut here---------------end--------------->8---

Program terminated with signal SIGSEGV, Segmentation fault.
#0  bms_get_singleton_member (a=0x10, member=member@entry=0x7fffb577cafc) at bitmapset.c:577
#1  0x000056425107b531 in find_relation_from_clauses (clauses=0x564251a68570, root=0x564251a273d8) at clausesel.c:445
#2  clauselist_selectivity (root=root@entry=0x564251a273d8, clauses=0x564251a68570, varRelid=varRelid@entry=0, jointype=JOIN_LEFT, sjinfo=0x564251a661c0) at clausesel.c:128
#3  0x00007f61d3d9f22f in postgresGetForeignJoinPaths (root=<optimized out>, joinrel=0x564251a66ba8, outerrel=<optimized out>, innerrel=<optimized out>, jointype=<optimized out>, extra=0x7fffb577cc50) at postgres_fdw.c:4466
#4  0x000056425108a238 in add_paths_to_joinrel (root=root@entry=0x564251a273d8, joinrel=joinrel@entry=0x564251a66ba8, outerrel=outerrel@entry=0x564251a65378, innerrel=innerrel@entry=0x564251a65f30, jointype=jointype@entry=JOIN_LEFT, sjinfo=sjinfo@entry=0x564251a661c0, restrictlist=0x564251a681c8) at joinpath.c:278
#5  0x000056425108bff2 in populate_joinrel_with_paths (restrictlist=<optimized out>, sjinfo=0x564251a661c0, joinrel=0x564251a66ba8, rel2=0x564251a65f30, rel1=0x564251a65378, root=0x564251a273d8) at joinrels.c:795
#6  make_join_rel (root=root@entry=0x564251a273d8, rel1=rel1@entry=0x564251a65378, rel2=rel2@entry=0x564251a65f30) at joinrels.c:731
#7  0x000056425108c7ef in make_rels_by_clause_joins (other_rels=<optimized out>, old_rel=<optimized out>, root=<optimized out>) at joinrels.c:277
#8  join_search_one_level (root=root@entry=0x564251a273d8, level=level@entry=2) at joinrels.c:99
#9  0x0000564251079bdb in standard_join_search (root=0x564251a273d8, levels_needed=2, initial_rels=<optimized out>) at allpaths.c:2385
#10 0x000056425107ac7b in make_one_rel (root=root@entry=0x564251a273d8, joinlist=joinlist@entry=0x564251a65998) at allpaths.c:184
#11 0x0000564251099ef4 in query_planner (root=root@entry=0x564251a273d8, tlist=tlist@entry=0x0, qp_callback=qp_callback@entry=0x56425109aeb0 <standard_qp_callback>, qp_extra=qp_extra@entry=0x7fffb577cff0) at planmain.c:253
#12 0x000056425109dbc2 in grouping_planner (root=root@entry=0x564251a273d8, inheritance_update=inheritance_update@entry=0 '\000', tuple_fraction=<optimized out>, tuple_fraction@entry=0) at planner.c:1684
#13 0x00005642510a0133 in subquery_planner (glob=glob@entry=0x564251a2a6d0, parse=parse@entry=0x5642519aac60, parent_root=parent_root@entry=0x0, hasRecursion=hasRecursion@entry=0 '\000', tuple_fraction=tuple_fraction@entry=0) at planner.c:833
#14 0x00005642510a0f71 in standard_planner (parse=0x5642519aac60, cursorOptions=256, boundParams=0x0) at planner.c:333
#15 0x00005642511458cd in pg_plan_query (querytree=querytree@entry=0x5642519aac60, cursorOptions=256, boundParams=boundParams@entry=0x0) at postgres.c:802
#16 0x0000564250fa9a40 in ExplainOneQuery (query=0x5642519aac60, cursorOptions=<optimized out>, into=0x0, es=0x564251a513a0, queryString=0x564251a09590 "explain select from\n  fdw_postgres.hslot\n    left join fdw_postgres.num_exp_div\n    on ((exists (values (1))) and (values (1)) is null);", params=0x0, queryEnv=0x0) at explain.c:367
#17 0x0000564250faa005 in ExplainQuery (pstate=pstate@entry=0x564251a511f0, stmt=stmt@entry=0x564251a0aa58, queryString=queryString@entry=0x564251a09590 "explain select from\n  fdw_postgres.hslot\n    left join fdw_postgres.num_exp_div\n    on ((exists (values (1))) and (values (1)) is null);", params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, dest=dest@entry=0x564251a51308) at explain.c:256
#18 0x000056425114b9cb in standard_ProcessUtility (pstmt=0x564251a0b2e0, queryString=0x564251a09590 "explain select from\n  fdw_postgres.hslot\n    left join fdw_postgres.num_exp_div\n    on ((exists (values (1))) and (values (1)) is null);", context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0, dest=0x564251a51308, completionTag=0x7fffb577d390 "") at utility.c:680
#19 0x00005642511487b4 in PortalRunUtility (portal=0x564251a07580, pstmt=0x564251a0b2e0, isTopLevel=<optimized out>, setHoldSnapshot=<optimized out>, dest=<optimized out>, completionTag=0x7fffb577d390 "") at pquery.c:1179
#20 0x0000564251149633 in FillPortalStore (portal=portal@entry=0x564251a07580, isTopLevel=isTopLevel@entry=1 '\001') at pquery.c:1039
#21 0x000056425114a21d in PortalRun (portal=portal@entry=0x564251a07580, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=1 '\001', run_once=run_once@entry=1 '\001', dest=dest@entry=0x564251a0b378, altdest=altdest@entry=0x564251a0b378, completionTag=0x7fffb577d5b0 "") at pquery.c:769
#22 0x0000564251145d8a in exec_simple_query (query_string=0x564251a09590 "explain select from\n  fdw_postgres.hslot\n    left join fdw_postgres.num_exp_div\n    on ((exists (values (1))) and (values (1)) is null);") at postgres.c:1105
#23 0x0000564251147ab1 in PostgresMain (argc=<optimized out>, argv=argv@entry=0x5642519b2e00, dbname=<optimized out>, username=<optimized out>) at postgres.c:4075
#24 0x0000564250e5c4cc in BackendRun (port=0x5642519a7d70) at postmaster.c:4317
#25 BackendStartup (port=0x5642519a7d70) at postmaster.c:3989
#26 ServerLoop () at postmaster.c:1729
#27 0x00005642510d07c3 in PostmasterMain (argc=3, argv=0x5642519844d0) at postmaster.c:1337
#28 0x0000564250e5db2d in main (argc=3, argv=0x5642519844d0) at main.c:228

  2. Re: [sqlsmith] Planner crash on foreign table join

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2017-04-08T07:57:04Z

    >>>>> "Andreas" == Andreas Seltenreich <seltenreich@gmx.de> writes:

 Andreas> Hi,
 
 Andreas> testing master at f0e44021df with a loopback postgres_fdw
 Andreas> installed, I see lots of crashes on queries joining foreign
 Andreas> tables with various expressions.  Below is a reduced recipe
 Andreas> for the regression database and a backtrace.

Commit ac2b095088 assumes that clauselist_selectivity is being passed a
list of RelOptInfo, but postgres_fdw is passing it a list of bare
clauses.  One of them is wrong :-)

-- 
Andrew (irc:RhodiumToad)

  3. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T16:27:43Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Andreas" == Andreas Seltenreich <seltenreich@gmx.de> writes:
>  Andreas> testing master at f0e44021df with a loopback postgres_fdw
>  Andreas> installed, I see lots of crashes on queries joining foreign
>  Andreas> tables with various expressions.  Below is a reduced recipe
>  Andreas> for the regression database and a backtrace.

> Commit ac2b095088 assumes that clauselist_selectivity is being passed a
> list of RelOptInfo, but postgres_fdw is passing it a list of bare
> clauses.  One of them is wrong :-)

It's a bit scary that apparently none of the committed regression tests
caught that.

More generally, I think the convention up to now has been that
clauselist_selectivity would work on either RestrictInfos or bare boolean
clauses, caching its results in the former case but succeeding anyway.
If we're to standardize on only one of those behaviors it should certainly
be the former, but I think postgres_fdw is probably not the only code that
will be broken if we remove the option for the latter.

			regards, tom lane

  4. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T19:57:25Z

    I wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> Commit ac2b095088 assumes that clauselist_selectivity is being passed a
>> list of RelOptInfo, but postgres_fdw is passing it a list of bare
>> clauses.  One of them is wrong :-)

> It's a bit scary that apparently none of the committed regression tests
> caught that.

Experimentation shows that actually, the standard regression tests
provide dozens of opportunities for find_relation_from_clauses to fail on
non-RestrictInfo input.  However, it lacks any IsA check, and the only
thing that it does with the alleged rinfo is 
	if (bms_get_singleton_member(rinfo->clause_relids, &relid))
As long as there's some kind of object pointer where the clause_relids
field would be, it's highly likely that bms_get_singleton_member will just
return false without crashing, thereby obscuring the fault.  Andreas'
example kills it by causing the argument to be a Param node, whose field
layout doesn't put a pointer there.

This makes me wonder whether we were being penny-wise and pound-foolish
by not making Bitmapsets be a kind of Node, so that there could be IsA
assertions in the bitmapset.c routines, as there are for Lists.  Most
Bitmapsets in a typical backend probably have only one payload word
(ie highest member < 32), so right now they occupy 8 bytes.  Adding
a nodetag would kick them up to the next palloc category, 16 bytes,
which is probably why I didn't do it like that to begin with.
Still, that decision is looking unduly byte-miserly in 2017.

			regards, tom lane

  5. Re: [sqlsmith] Planner crash on foreign table join

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2017-04-08T20:27:28Z

    >>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> Experimentation shows that actually, the standard regression tests
 Tom> provide dozens of opportunities for find_relation_from_clauses to
 Tom> fail on non-RestrictInfo input.  However, it lacks any IsA check,

In a discussion with Andres on the hash grouping sets review thread, I
proposed that we should have something of the form

#define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))

to replace the current idiom of

    foreach(l, blah)
    {
        SomeType *x = (SomeType *) lfirst(l);

(in my code I tend to omit the (SomeType *), which I dislike because it
adds no real protection)

by

    foreach(l, blah)
    {
        SomeType *x = lfirst_node(SomeType, l);

in order to get that IsA check in there in a convenient way.

-- 
Andrew (irc:RhodiumToad)

  6. Re: [sqlsmith] Planner crash on foreign table join

    Robert Haas <robertmhaas@gmail.com> — 2017-04-08T21:01:06Z

    On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This makes me wonder whether we were being penny-wise and pound-foolish
> by not making Bitmapsets be a kind of Node, so that there could be IsA
> assertions in the bitmapset.c routines, as there are for Lists.  Most
> Bitmapsets in a typical backend probably have only one payload word
> (ie highest member < 32), so right now they occupy 8 bytes.  Adding
> a nodetag would kick them up to the next palloc category, 16 bytes,
> which is probably why I didn't do it like that to begin with.
> Still, that decision is looking unduly byte-miserly in 2017.

I think it's pretty dubious to change this, honestly.  Just because it
would have caught this one bug doesn't make it an especially valuable
thing in general.  Bytes are still not free.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  7. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T21:09:16Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom> Experimentation shows that actually, the standard regression tests
>  Tom> provide dozens of opportunities for find_relation_from_clauses to
>  Tom> fail on non-RestrictInfo input.  However, it lacks any IsA check,

> In a discussion with Andres on the hash grouping sets review thread, I
> proposed that we should have something of the form

> #define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))

That seems like a fairly good idea.  A significant fraction of the
existing castNode() calls are being applied to lfirst(something),
and this would shorten that idiom a bit.

There's another noticeable fraction that are being applied to
linitial(something), but I'm not sure if defining linitial_node()
is worth the trouble.

			regards, tom lane

  8. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-08T21:20:28Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This makes me wonder whether we were being penny-wise and pound-foolish
>> by not making Bitmapsets be a kind of Node, so that there could be IsA
>> assertions in the bitmapset.c routines, as there are for Lists.

> I think it's pretty dubious to change this, honestly.  Just because it
> would have caught this one bug doesn't make it an especially valuable
> thing in general.  Bytes are still not free.

Yeah, true.  OTOH I recall Andres lobbying to change the bitmap word
size to 64 bits on 64-bit hardware, and it *would* be free in that case
due to alignment padding.

What I think I might do is write a trial patch that turns Bitmapsets
into Nodes, and see if it catches any other existing bugs.  If it does
not, that would be good evidence for your position.

We could also consider installing the nodetag only in Assert-enabled
builds, although that approach would prevent us from applying followon
simplifications such as not having to treat bitmapset fields specially
in copyfuncs.c and like places.

			regards, tom lane

  9. Re: [sqlsmith] Planner crash on foreign table join

    Andres Freund <andres@anarazel.de> — 2017-04-08T21:30:36Z

    On 2017-04-08 17:20:28 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> This makes me wonder whether we were being penny-wise and pound-foolish
> >> by not making Bitmapsets be a kind of Node, so that there could be IsA
> >> assertions in the bitmapset.c routines, as there are for Lists.
> 
> > I think it's pretty dubious to change this, honestly.  Just because it
> > would have caught this one bug doesn't make it an especially valuable
> > thing in general.  Bytes are still not free.
> 
> Yeah, true.  OTOH I recall Andres lobbying to change the bitmap word
> size to 64 bits on 64-bit hardware, and it *would* be free in that case
> due to alignment padding.

Hah, yes, I did. A loong time ago ;) I still think it's a good idea, and
probably has become more useful with just about anyone using 64bits
these days.  Also interesting for tidbitmap, which reuses bitmapset's
bitmapword.

> We could also consider installing the nodetag only in Assert-enabled
> builds, although that approach would prevent us from applying followon
> simplifications such as not having to treat bitmapset fields specially
> in copyfuncs.c and like places.

Yea, don't like this much.


- Andres

  10. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-09T00:13:56Z

    I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it's pretty dubious to change this, honestly.  Just because it
>> would have caught this one bug doesn't make it an especially valuable
>> thing in general.  Bytes are still not free.

> What I think I might do is write a trial patch that turns Bitmapsets
> into Nodes, and see if it catches any other existing bugs.  If it does
> not, that would be good evidence for your position.

I made the attached quick-hack patch, and found that check-world
passes just fine with it.  That's not complete proof that we have
no other bugs of this ilk, but it definitely supports the idea
that we don't really need to add the overhead.  I'll just put this
in the archives for possible future reference.

(Or perhaps Andreas would like to try bashing on a copy with this
installed.)

			regards, tom lane

  11. Re: [sqlsmith] Planner crash on foreign table join

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-09T01:38:09Z

    > On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think it's pretty dubious to change this, honestly.  Just because it
>>> would have caught this one bug doesn't make it an especially valuable
>>> thing in general.  Bytes are still not free.
> 
>> What I think I might do is write a trial patch that turns Bitmapsets
>> into Nodes, and see if it catches any other existing bugs.  If it does
>> not, that would be good evidence for your position.
> 
> I made the attached quick-hack patch, and found that check-world
> passes just fine with it. 

Not so for me.  I get a failure almost immediately:

Running in no-clean mode.  Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user "mark".
This user must also own the server process.

The database cluster will be initialized with locales
  COLLATE:  en_US.UTF-8
  CTYPE:    en_US.UTF-8
  MESSAGES: C
  MONETARY: en_US.UTF-8
  NUMERIC:  en_US.UTF-8
  TIME:     en_US.UTF-8
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".
    
Data page checksums are disabled.
        
creating directory /Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... TRAP: FailedAssertion("!(((((const Node*)(a))->type) == T_Bitmapset))", File: "bitmapset.c", Line: 731)
child process was terminated by signal 6: Abort trap
initdb: data directory "/Users/mark/hydra/postgresql/src/test/regress/./tmp_check/data" not removed at user's request

  12. Re: [sqlsmith] Planner crash on foreign table join

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-09T01:48:51Z

    > On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 
>> I wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> I think it's pretty dubious to change this, honestly.  Just because it
>>>> would have caught this one bug doesn't make it an especially valuable
>>>> thing in general.  Bytes are still not free.
>> 
>>> What I think I might do is write a trial patch that turns Bitmapsets
>>> into Nodes, and see if it catches any other existing bugs.  If it does
>>> not, that would be good evidence for your position.
>> 
>> I made the attached quick-hack patch, and found that check-world
>> passes just fine with it. 
> 
> Not so for me.  I get a failure almost immediately:

I recant.  Looks like I didn't get the patch applied quite right.  So sorry for the noise.

  13. Re: [sqlsmith] Planner crash on foreign table join

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-09T02:02:08Z

    > On Apr 8, 2017, at 6:48 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 6:38 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
>> 
>> 
>>> On Apr 8, 2017, at 5:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 
>>> I wrote:
>>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>> I think it's pretty dubious to change this, honestly.  Just because it
>>>>> would have caught this one bug doesn't make it an especially valuable
>>>>> thing in general.  Bytes are still not free.
>>> 
>>>> What I think I might do is write a trial patch that turns Bitmapsets
>>>> into Nodes, and see if it catches any other existing bugs.  If it does
>>>> not, that would be good evidence for your position.
>>> 
>>> I made the attached quick-hack patch, and found that check-world
>>> passes just fine with it. 
>> 
>> Not so for me.  I get a failure almost immediately:
> 
> I recant.  Looks like I didn't get the patch applied quite right.  So sorry for the noise.

The regression tests now fail on a number of tests due to a server crash:

2017-04-08 18:55:19.826 PDT [90779] pg_regress/errors STATEMENT:  select infinite_recurse();
TRAP: FailedAssertion("!(((((const Node*)(a))->type) == T_Bitmapset))", File: "bitmapset.c", Line: 601)
2017-04-08 18:55:22.487 PDT [90242] LOG:  server process (PID 90785) was terminated by signal 6: Abort trap
2017-04-08 18:55:22.487 PDT [90242] DETAIL:  Failed process was running: explain (costs off)
    select * from onek2 where unique2 = 11 and stringu1 = 'ATAAAA';


This is very near where the original crash reported in this thread was crashing, probably only
different due to the extra lines of Assert that were added.  Am I missing some portion of the
fix that you are testing?  I have only applied the patch that Tom included in the previous email.

mark

  14. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-09T02:35:50Z

    Mark Dilger <hornschnorter@gmail.com> writes:
> This is very near where the original crash reported in this thread was crashing, probably only
> different due to the extra lines of Assert that were added.  Am I missing some portion of the
> fix that you are testing?  I have only applied the patch that Tom included in the previous email.

No, that was the whole patch --- but did you do a full "make clean" and
rebuild?  The patch changed NodeTag numbers which would affect the whole
tree.

			regards, tom lane

  15. Re: [sqlsmith] Planner crash on foreign table join

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-09T02:41:36Z

    > On Apr 8, 2017, at 7:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Mark Dilger <hornschnorter@gmail.com> writes:
>> This is very near where the original crash reported in this thread was crashing, probably only
>> different due to the extra lines of Assert that were added.  Am I missing some portion of the
>> fix that you are testing?  I have only applied the patch that Tom included in the previous email.
> 
> No, that was the whole patch --- but did you do a full "make clean" and
> rebuild?  The patch changed NodeTag numbers which would affect the whole
> tree.

I had trouble applying your patch using

 mark$ patch -p 1 < patch
patching file src/backend/nodes/bitmapset.c
Hunk #1 FAILED at 115.
Hunk #2 FAILED at 146.
Hunk #3 FAILED at 190.
Hunk #4 FAILED at 205.
Hunk #5 FAILED at 229.
Hunk #6 FAILED at 265.
Hunk #7 FAILED at 298.
Hunk #8 FAILED at 324.
Hunk #9 FAILED at 364.
Hunk #10 FAILED at 444.
Hunk #11 FAILED at 463.
Hunk #12 FAILED at 488.
Hunk #13 FAILED at 517.
Hunk #14 FAILED at 554.
Hunk #15 FAILED at 598.
Hunk #16 FAILED at 635.
Hunk #17 FAILED at 665.
Hunk #18 FAILED at 694.
Hunk #19 FAILED at 732.
Hunk #20 FAILED at 770.
Hunk #21 FAILED at 789.
Hunk #22 FAILED at 825.
Hunk #23 FAILED at 853.
Hunk #24 FAILED at 878.
Hunk #25 FAILED at 927.
Hunk #26 FAILED at 981.
Hunk #27 FAILED at 1027.
27 out of 27 hunks FAILED -- saving rejects to file src/backend/nodes/bitmapset.c.rej
patching file src/include/nodes/bitmapset.h
Hunk #1 FAILED at 20.
Hunk #2 FAILED at 38.
2 out of 2 hunks FAILED -- saving rejects to file src/include/nodes/bitmapset.h.rej
patching file src/include/nodes/nodes.h
Hunk #1 FAILED at 291.
1 out of 1 hunk FAILED -- saving rejects to file src/include/nodes/nodes.h.rej


So I did the patching manually against my copy of the current master,
aba696d1af9a267eee85d69845c3cdeccf788525, and then ran:

make distclean; ./configure --enable-cassert --enable-tap-tests --enable-depend && make -j4 && make check-world

I am attaching my patch, which I admit on closer inspection differs from yours, but not
in any way I can tell is relevant:

  16. Re: [sqlsmith] Planner crash on foreign table join

    Mark Dilger <hornschnorter@gmail.com> — 2017-04-09T02:51:29Z

    > On Apr 8, 2017, at 7:41 PM, Mark Dilger <hornschnorter@gmail.com> wrote:
> 
> 
>> On Apr 8, 2017, at 7:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 
>> Mark Dilger <hornschnorter@gmail.com> writes:
>>> This is very near where the original crash reported in this thread was crashing, probably only
>>> different due to the extra lines of Assert that were added.  Am I missing some portion of the
>>> fix that you are testing?  I have only applied the patch that Tom included in the previous email.
>> 
>> No, that was the whole patch --- but did you do a full "make clean" and
>> rebuild?  The patch changed NodeTag numbers which would affect the whole
>> tree.
> 
> <snip>

> I'm going to pull completely fresh sources and reapply and retest, though you are welcome
> to review my patch while I do that.

That fixed it.  Thanks.

  17. Re: [sqlsmith] Planner crash on foreign table join

    Andreas Seltenreich <seltenreich@gmx.de> — 2017-04-09T12:38:20Z

    Tom Lane writes:

> I made the attached quick-hack patch, and found that check-world
> passes just fine with it.  That's not complete proof that we have
> no other bugs of this ilk, but it definitely supports the idea
> that we don't really need to add the overhead.  I'll just put this
> in the archives for possible future reference.
>
> (Or perhaps Andreas would like to try bashing on a copy with this
> installed.)

I certainly do :-).  SQLsmith has been fuzzing for couple hours with the
patch applied, and so far none of the assertions fired.  I'll leave the
patch on my fuzzing branch until merging becomes burdensome.

regards,
Andreas

  18. Re: [sqlsmith] Planner crash on foreign table join

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-04-09T21:55:19Z

    On Sun, Apr 9, 2017 at 8:27 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
>     foreach(l, blah)
>     {
>         SomeType *x = (SomeType *) lfirst(l);
>
> (in my code I tend to omit the (SomeType *), which I dislike because it
> adds no real protection)

Just BTW, without that cast it's not compilable as C++, so I'm
guessing that Peter E will finish up putting it back in wherever you
leave it out...

-- 
Thomas Munro
http://www.enterprisedb.com

  19. Re: [sqlsmith] Planner crash on foreign table join

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2017-04-10T05:14:42Z

    >>>>> "Thomas" == Thomas Munro <thomas.munro@enterprisedb.com> writes:

 >> SomeType *x = (SomeType *) lfirst(l);
 >> 
 >> (in my code I tend to omit the (SomeType *), which I dislike because
 >> it adds no real protection)

 Thomas> Just BTW, without that cast it's not compilable as C++, so I'm
 Thomas> guessing that Peter E will finish up putting it back in
 Thomas> wherever you leave it out...

There's north of 150 other examples (just grep for '= lfirst' in the
source). Some were even committed by Peter E :-)

In the discussion with Andres the same point came up for palloc, for
which I suggested we add something along the lines of:

#define palloc_object(_type_) (_type_ *) palloc(sizeof(_type_))
#define palloc_array(_type_, n) (_type_ *) palloc((n) * sizeof(_type_))

palloc() without a cast is even more common than lfirst() without one,
and something like half of those (and 80%+ of the pallocs that do have a
cast) are palloc(sizeof(...)) or palloc(something * sizeof(...)).

-- 
Andrew (irc:RhodiumToad)

  20. Re: [sqlsmith] Planner crash on foreign table join

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T11:34:04Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> In the discussion with Andres the same point came up for palloc, for
> which I suggested we add something along the lines of:

> #define palloc_object(_type_) (_type_ *) palloc(sizeof(_type_))
> #define palloc_array(_type_, n) (_type_ *) palloc((n) * sizeof(_type_))

I'm far less excited about that, mainly because you'd have to also cover
palloc0, repalloc, MemoryContextAlloc, etc etc.  Also I've not seen
very many actual bugs that this would've helped with.

			regards, tom lane

  21. Adding lfirst_node (was Re: [sqlsmith] Planner crash on foreign table join)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-10T16:20:16Z

    I wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> In a discussion with Andres on the hash grouping sets review thread, I
>> proposed that we should have something of the form

>> #define lfirst_node(_type_, l) (castNode(_type_,lfirst(l)))

> That seems like a fairly good idea.  A significant fraction of the
> existing castNode() calls are being applied to lfirst(something),
> and this would shorten that idiom a bit.

PFA a patch to do this.  It turns out that just under half of the
castNode() calls in the current tree have List-cell-extraction
functions as arguments, and so can be replaced like this.  So I think
this is a great idea and we should do it; it's a definite notational
improvement.

As with the original addition of castNode, it seems like a good idea
to back-patch the additions to pg_list.h, so that we won't have
back-patching problems for new code using this feature.

> There's another noticeable fraction that are being applied to
> linitial(something), but I'm not sure if defining linitial_node()
> is worth the trouble.

It is, and in fact I ended up providing equivalents for all the
List-cell-extraction functions.  All except lfourth_node() are
actually in use in this patch.

Barring objections, I'll push this shortly.

			regards, tom lane

  22. Re: Adding lfirst_node (was Re: [sqlsmith] Planner crash on foreign table join)

    Andres Freund <andres@anarazel.de> — 2017-04-10T16:23:38Z

    On 2017-04-10 12:20:16 -0400, Tom Lane wrote:
> Barring objections, I'll push this shortly.

+1, to just about all of it