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

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

I wrote:
> Andrew Gierth  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

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6b7503d..bf03e67 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2393,7 +2393,7 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
 
 	foreach(lc, rtable)
 	{
-		RangeTblEntry *rte = castNode(RangeTblEntry, lfirst(lc));
+		RangeTblEntry *rte = lfirst_node(RangeTblEntry, lc);
 
 		APP_JUMB(rte->rtekind);
 		switch (rte->rtekind)
@@ -2656,7 +2656,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
 JumbleExpr(jstate, (Node *) caseexpr->arg);
 foreach(temp, caseexpr->args)
 {
-	CaseWhen   *when = castNode(CaseWhen, lfirst(temp));
+	CaseWhen   *when = lfirst_node(CaseWhen, temp);
 
 	JumbleExpr(jstate, (Node *) when->expr);
 	JumbleExpr(jstate, (Node *) when->result);
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 129bdb5..c5149a0 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1350,7 +1350,7 @@ deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
 
 	foreach(lc, tlist)
 	{
-		TargetEntry *tle = castNode(TargetEntry, lfirst(lc));
+		TargetEntry *tle = lfirst_node(TargetEntry, lc);
 
 		if (i > 0)
 			appendStringInfoString(buf, ", ");
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2851869..6670df5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1169,7 +1169,7 @@ postgresGetForeignPlan(PlannerInfo *root,
 	 */
 	foreach(lc, scan_clauses)
 	{
-		RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
+		RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
 
 		/* Ignore any pseudoconstants, they're dealt with elsewhere */
 		if (rinfo->pseudoconstant)
@@ -5022,8 +5022,8 @@ conversion_error_callback(void *arg)
 		EState	   *estate = fsstate->ss.ps.state;
 		TargetEntry *tle;
 
-		tle = castNode(TargetEntry, list_nth(fsplan->fdw_scan_tlist,
-			 errpos->cur_attno - 1));
+		tle = list_nth_node(TargetEntry, fsplan->fdw_scan_tlist,
+			errpos->cur_attno - 1);
 
 		/*
 		 * Target list can have Vars and expressions.  For Vars, we can get
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 1eb7930..1492722 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -854,7 +854,7 @@ get_object_address(ObjectType objtype, Node *object,
 
 	objlist = castNode(List, object);
 	domaddr = get_object_address_type(OBJECT_DOMAIN,
-	  castNode(TypeName, linitial(objlist)),
+	  linitial_node(TypeName, objlist),
 	  missing_ok);
 	constrname = strVal(lsecond(objlist));
 
@@ -932,8 +932,8 @@ get_object_address(ObjectType objtype, Node *object,
 break;
 			case OBJECT_CAST:
 {
-	TypeName   *sourcetype = castNode(TypeName, linitial(castNode(List, object)));
-	TypeName   *targettype = castNode(TypeName, lsecond(castNode(List, object)));
+	TypeName   *sourcetype = linitial_node(TypeName, castNode(List, object));
+	TypeName   *targettype = lsecond_node(TypeName, castNode(List, object));
 	Oid			sourcetypeid;
 	Oid			targettypeid;
 
@@ -947,7 +947,7 @@ get_object_address(ObjectType objtype, Node *object,
 break;
 			case OBJECT_TRANSFORM:
 {
-	TypeName   *typename = castNode(TypeName, linitial(castNode(List, object)));
+	TypeName   *typename = linitial_node(TypeName, castNode(List, object));
 	char	   *langname = strVal(lsecond(castNode(List, object)));
 	Oid			

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

Andrew Gierth  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Andrew Gierth]

> "Thomas" == Thomas Munro  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)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Thomas Munro]

On Sun, Apr 9, 2017 at 8:27 AM, Andrew Gierth
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Andreas Seltenreich]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 7:41 PM, Mark Dilger  wrote:
> 
> 
>> On Apr 8, 2017, at 7:35 PM, Tom Lane  wrote:
>> 
>> Mark Dilger  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'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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 7:35 PM, Tom Lane  wrote:
> 
> Mark Dilger  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:



patch.mark.1
Description: Binary data


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

mark
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

Mark Dilger  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 6:48 PM, Mark Dilger  wrote:
> 
> 
>> On Apr 8, 2017, at 6:38 PM, Mark Dilger  wrote:
>> 
>> 
>>> On Apr 8, 2017, at 5:13 PM, Tom Lane  wrote:
>>> 
>>> I wrote:
 Robert Haas  writes:
> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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 = 'AT';


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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 6:38 PM, Mark Dilger  wrote:
> 
> 
>> On Apr 8, 2017, at 5:13 PM, Tom Lane  wrote:
>> 
>> I wrote:
>>> Robert Haas  writes:
 On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Mark Dilger]

> On Apr 8, 2017, at 5:13 PM, Tom Lane  wrote:
> 
> I wrote:
>> Robert Haas  writes:
>>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

I wrote:
> Robert Haas  writes:
>> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index bf8545d..39d7b41 100644
*** a/src/backend/nodes/bitmapset.c
--- b/src/backend/nodes/bitmapset.c
*** bms_copy(const Bitmapset *a)
*** 115,120 
--- 115,121 
  
  	if (a == NULL)
  		return NULL;
+ 	Assert(IsA(a, Bitmapset));
  	size = BITMAPSET_SIZE(a->nwords);
  	result = (Bitmapset *) palloc(size);
  	memcpy(result, a, size);
*** bms_equal(const Bitmapset *a, const Bitm
*** 145,150 
--- 146,153 
  	}
  	else if (b == NULL)
  		return bms_is_empty(a);
+ 	Assert(IsA(a, Bitmapset));
+ 	Assert(IsA(b, Bitmapset));
  	/* Identify shorter and longer input */
  	if (a->nwords <= b->nwords)
  	{
*** bms_make_singleton(int x)
*** 187,192 
--- 190,196 
  	wordnum = WORDNUM(x);
  	bitnum = BITNUM(x);
  	result = (Bitmapset *) palloc0(BITMAPSET_SIZE(wordnum + 1));
+ 	result->type = T_Bitmapset;
  	result->nwords = wordnum + 1;
  	result->words[wordnum] = ((bitmapword) 1 << bitnum);
  	return result;
*** void
*** 201,207 
--- 205,214 
  bms_free(Bitmapset *a)
  {
  	if (a)
+ 	{
+ 		Assert(IsA(a, Bitmapset));
  		pfree(a);
+ 	}
  }
  
  
*** bms_union(const Bitmapset *a, const Bitm
*** 222,227 
--- 229,236 
  	int			otherlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL)
  		return bms_copy(b);
*** bms_intersect(const Bitmapset *a, const 
*** 256,261 
--- 265,272 
  	int			resultlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL || b == NULL)
  		return NULL;
*** bms_difference(const Bitmapset *a, const
*** 287,292 
--- 298,305 
  	int			shortlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL)
  		return NULL;
*** bms_is_subset(const Bitmapset *a, const 
*** 311,316 
--- 324,331 
  	int			longlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL)
  		return true;			/* empty set is a subset of anything */
*** bms_subset_compare(const Bitmapset *a, c
*** 349,354 
--- 364,371 
  	int			longlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL)
  	{
*** bms_is_member(int x, const Bitmapset *a)
*** 427,432 
--- 444,450 
  		elog(ERROR, "negative bitmapset member not allowed");
  	if (a == NULL)
  		return false;
+ 	Assert(IsA(a, Bitmapset));
  	wordnum = WORDNUM(x);
  	bitnum = BITNUM(x);
  	if (wordnum >= a->nwords)
*** bms_overlap(const Bitmapset *a, const Bi
*** 445,450 
--- 463,470 
  	int			shortlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL || b == NULL)
  		return false;
*** bms_overlap_list(const Bitmapset *a, con
*** 468,473 
--- 488,494 
  	int			wordnum,
  bitnum;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
  	if (a == NULL || b == NIL)
  		return false;
  
*** bms_nonempty_difference(const Bitmapset 
*** 496,501 
--- 517,524 
  	int			shortlen;
  	int			i;
  
+ 	Assert(a == NULL || IsA(a, Bitmapset));
+ 	Assert(b == NULL || IsA(b, Bitmapset));
  	/* Handle cases where either input is NULL */
  	if (a == NULL)
  		return false;
*** bms_singleton_member(const Bitmapset *a)
*** 531,536 
--- 554,560 
  
  	if (a == NULL)
  		elog(ERROR, "bitmapset is empty");
+ 	Assert(IsA(a, Bitmapset));
  	nwords = a->nwords;
 

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Andres Freund]

On 2017-04-08 17:20:28 -0400, Tom Lane wrote:
> Robert Haas  writes:
> > On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

Robert Haas  writes:
> On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Robert Haas]

On Sat, Apr 8, 2017 at 3:57 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Andrew Gierth]

> "Tom" == Tom Lane  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)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

I wrote:
> Andrew Gierth  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, ))
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Tom Lane]

Andrew Gierth  writes:
> "Andreas" == Andreas Seltenreich  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [sqlsmith] Planner crash on foreign table join

[Andrew Gierth]

> "Andreas" == Andreas Seltenreich  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)


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers