safer node casting

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I propose a macro castNode() that combines the assertion and the cast,
so this would become
RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Seems like an OK idea, but I'm concerned by the implied multiple
evaluations, particularly if you're going to apply this to function
results --- as the above example does. I think you need to go the
extra mile to make it single-evaluation. See newNode() for ideas.

Just to bikeshed a bit ... would "castNode" be a better name?
Seems like a closer analogy to makeNode(), for instance.

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

I wrote:

Just to bikeshed a bit ... would "castNode" be a better name?

Um ... -ENOCAFFEINE. Never mind that bit.

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

On Sat, Dec 31, 2016 at 11:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I propose a macro castNode() that combines the assertion and the cast,
so this would become
RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

+1. That would be wonderful.

Seems like an OK idea, but I'm concerned by the implied multiple
evaluations, particularly if you're going to apply this to function
results --- as the above example does. I think you need to go the
extra mile to make it single-evaluation. See newNode() for ideas.

+1.

In case the Assert fails, the debugger would halt at castNode macro
and a first time reader would be puzzled to see no assert there.
Obviously looking at the #define should clarify the confusion. But I
don't see how that can be avoided. I was thinking of using a function
castNodeFunc(), but it can't accomodate Assert with _type_. Will
calling this function as checkAndCastNode() help?

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

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

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

There is a common coding pattern that goes like this:

RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
Assert(IsA(rinfo, RestrictInfo));

I propose a macro castNode() that combines the assertion and the cast,
so this would become

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

I'm quite a bit in favor of something like this, having proposed it
before ;)

+#define castNode(_type_,nodeptr) (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))

ISTM that we need to do the core part of this in an inline function, to
avoid multiple evaluation hazards - which seem quite likely to occur
here - it's pretty common to cast the result of a function after all.

Something like

static inline Node*
castNodeImpl(void *c, enum NodeTag t)
{
Assert(c == NULL || IsA(c, t));
return c;
}

#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))

should work without too much trouble afaics?

Greetings,

Andres Freund

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

On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

There is a common coding pattern that goes like this:

RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
Assert(IsA(rinfo, RestrictInfo));

I propose a macro castNode() that combines the assertion and the cast,
so this would become

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

I'm quite a bit in favor of something like this, having proposed it
before ;)

+#define castNode(_type_,nodeptr) (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))

ISTM that we need to do the core part of this in an inline function, to
avoid multiple evaluation hazards - which seem quite likely to occur
here - it's pretty common to cast the result of a function after all.

Something like

static inline Node*
castNodeImpl(void *c, enum NodeTag t)
{
Assert(c == NULL || IsA(c, t));
return c;
}

#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))

should work without too much trouble afaics?

I tried this quickly as per attached patch. It gave a compiler error
createplan.c: In function ‘castNodeImpl’:
createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
createplan.c:340:2: note: each undeclared identifier is reported only
once for each function it appears in
createplan.c: In function ‘create_plan_recurse’:
createplan.c:445:13: error: expected expression before ‘AggPath’

Is the attached patch as per your suggestion?

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Hi,

On 2017-01-03 11:00:47 +0530, Ashutosh Bapat wrote:

On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

There is a common coding pattern that goes like this:

RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
Assert(IsA(rinfo, RestrictInfo));

I propose a macro castNode() that combines the assertion and the cast,
so this would become

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

I'm quite a bit in favor of something like this, having proposed it
before ;)

+#define castNode(_type_,nodeptr) (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))

ISTM that we need to do the core part of this in an inline function, to
avoid multiple evaluation hazards - which seem quite likely to occur
here - it's pretty common to cast the result of a function after all.

Something like

static inline Node*
castNodeImpl(void *c, enum NodeTag t)
{
Assert(c == NULL || IsA(c, t));
return c;
}

#define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))

should work without too much trouble afaics?

I tried this quickly as per attached patch. It gave a compiler error
createplan.c: In function ‘castNodeImpl’:
createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
createplan.c:340:2: note: each undeclared identifier is reported only
once for each function it appears in
createplan.c: In function ‘create_plan_recurse’:
createplan.c:445:13: error: expected expression before ‘AggPath’

Well, I wrote that just to outline my suggestion, not as a patch ;).
It's just that we have to replace IsA() with nodeTag(nodeptr) == t
(because IsA does string concat magic).

Greetings,

Andres Freund

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

Hi Peter,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really would
have liked this a number of times already... I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Greetings,

Andres Freund

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

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really would
have liked this a number of times already... I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Yeah, I'd like that in sooner rather than later, too. But we do need
it to be foolproof - no multiple evals. The first draft had
inadequate-parenthesization hazards, too.

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

On 2017-01-25 19:21:40 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really would
have liked this a number of times already... I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Yeah, I'd like that in sooner rather than later, too. But we do need
it to be foolproof - no multiple evals. The first draft had
inadequate-parenthesization hazards,

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Andres

Andres Freund <andres@anarazel.de> writes:

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Looks generally good. A couple quibbles from a quick read-through:

* All but the first change in ProcessCopyOptions seem rather pointless:

 			else if (defel->arg && IsA(defel->arg, List))
-				cstate->force_quote = (List *) defel->arg;
+				cstate->force_quote = castNode(List, defel->arg);

In these places, castNode() isn't checking anything the if-condition
didn't. Maybe it's good style anyway, but I'm not really convinced.

* In ExecInitAgg:

 			aggnode = list_nth(node->chain, phase - 1);
-			sortnode = (Sort *) aggnode->plan.lefttree;
-			Assert(IsA(sortnode, Sort));
+			sortnode = castNode(Sort, aggnode->plan.lefttree);

it seems like the assignment to aggnode ought to have a castNode on it too
(the fact that it lacks any cast now is sloppy and not per project style,
IMO).

There were a bunch of places in ab1f0c822 where I wished I had this,
but I can go back and back-fill that later; doesn't need to be in the
first commit.

BTW, maybe it's just the first flush of enthusiasm, but I can see us
using this so much that the lack of it in back branches will become
a serious PITA for back-patching. So I'm strongly tempted to propose
that your 0001 should be back-patched. However, before 9.6 we didn't
have the compiler requirement that "static inline" in headers must be
handled sanely. Maybe a useful compromise would be to put 0001 in 9.6,
and before that just add

#define castNode(_type_,nodeptr) ((_type_ *)(nodeptr))

which would allow the notation to be used safely, albeit without
any assertion backup. Alternatively, we could enable the assertion
code only for gcc, which would probably be plenty good enough for
finding bugs in stable branches.

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

On 2017-01-26 16:55:33 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Looks generally good. A couple quibbles from a quick read-through:

* All but the first change in ProcessCopyOptions seem rather pointless:

else if (defel->arg && IsA(defel->arg, List))
-				cstate->force_quote = (List *) defel->arg;
+				cstate->force_quote = castNode(List, defel->arg);

In these places, castNode() isn't checking anything the if-condition
didn't. Maybe it's good style anyway, but I'm not really convinced.

Agreed that it's not not necessary - I didn't add this one (or any
castNode actually). But I don't think it matters much.

* In ExecInitAgg:

aggnode = list_nth(node->chain, phase - 1);
-			sortnode = (Sort *) aggnode->plan.lefttree;
-			Assert(IsA(sortnode, Sort));
+			sortnode = castNode(Sort, aggnode->plan.lefttree);

it seems like the assignment to aggnode ought to have a castNode on it
too

Yea, looks good.

(the fact that it lacks any cast now is sloppy and not per project style,
IMO).

There's a lot of these missing :(. This is one of these things that'd
be a lot easier to enforce if we'd be able to compile in a c++
compatible mode (-Wc++-compat), because there void * to X * casts
have to be done explicitly.

BTW, maybe it's just the first flush of enthusiasm, but I can see us
using this so much that the lack of it in back branches will become
a serious PITA for back-patching.

Might, yea.

So I'm strongly tempted to propose
that your 0001 should be back-patched. However, before 9.6 we didn't
have the compiler requirement that "static inline" in headers must be
handled sanely. Maybe a useful compromise would be to put 0001 in 9.6,
and before that just add

#define castNode(_type_,nodeptr) ((_type_ *)(nodeptr))

which would allow the notation to be used safely, albeit without
any assertion backup. Alternatively, we could enable the assertion
code only for gcc, which would probably be plenty good enough for
finding bugs in stable branches.

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc? Then we
can just remove the defined(PG_USE_INLINE) and it's associated comment
in >= 9.6.

Regards,

Andres

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

Andres Freund <andres@anarazel.de> writes:

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc?

Ah, yeah, that would work --- I'd already swapped out that business ;-)

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

On 2017-01-26 17:27:45 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc?

Ah, yeah, that would work --- I'd already swapped out that business ;-)

Done that way.

- Andres

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

Hi,

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

This is inspired by the dynamic_cast operator in C++, but follows the
syntax of the well-known makeNode() macro.

The analogy to dynamic_cast goes only so far, because we don't actually
support inheritance. I.e. in c++ we could successfully cast SeqScanState to a
PlanState, ScanState and SeqScanState - but with our model only
SeqScanState can be checked.

Greetings,

Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

This is inspired by the dynamic_cast operator in C++, but follows the
syntax of the well-known makeNode() macro.

The analogy to dynamic_cast goes only so far, because we don't actually
support inheritance. I.e. in c++ we could successfully cast SeqScanState to a
PlanState, ScanState and SeqScanState - but with our model only
SeqScanState can be checked.

Yeah, I was thinking about that earlier --- this can only be used to cast
to a concrete node type, not one of the "abstract" types like Plan * or
Expr *. Not sure if that's worth worrying about though; I don't think
I've ever seen actual bugs in PG code from casting the wrong thing in that
direction. For the most part, passing the wrong thing would end up firing
a default: case in a switch, or some such, so we already do have some
defenses for that direction.

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

On 2017-01-26 20:29:06 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

This is inspired by the dynamic_cast operator in C++, but follows the
syntax of the well-known makeNode() macro.

The analogy to dynamic_cast goes only so far, because we don't actually
support inheritance. I.e. in c++ we could successfully cast SeqScanState to a
PlanState, ScanState and SeqScanState - but with our model only
SeqScanState can be checked.

Yeah, I was thinking about that earlier --- this can only be used to cast
to a concrete node type, not one of the "abstract" types like Plan * or
Expr *. Not sure if that's worth worrying about though; I don't think
I've ever seen actual bugs in PG code from casting the wrong thing in that
direction. For the most part, passing the wrong thing would end up firing
a default: case in a switch, or some such, so we already do have some
defenses for that direction.

Yea, I'm not actually worried about it - I was more generally remarking
on the analogy made by Peter. For a second I was considering bringing up
the analogy in a comment or such, and this was one of a number of
arguments that made me disregard that idea.

Andres

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

On 1/26/17 16:15, Andres Freund wrote:

On 2017-01-25 19:21:40 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really would
have liked this a number of times already... I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Yeah, I'd like that in sooner rather than later, too. But we do need
it to be foolproof - no multiple evals. The first draft had
inadequate-parenthesization hazards,

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Thanks for finishing that up. I have committed more uses that I had
lying around partially done. Looks nice now.

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

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

On Tue, Feb 21, 2017 at 9:00 AM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

On 1/26/17 16:15, Andres Freund wrote:

On 2017-01-25 19:21:40 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:

RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));

Are you planning to add this / update this patch? Because I really

would

have liked this a number of times already... I can update it according
to my suggestions (to avoid multiple eval scenarios) if helpful

Yeah, I'd like that in sooner rather than later, too. But we do need
it to be foolproof - no multiple evals. The first draft had
inadequate-parenthesization hazards,

How about something like the attached? The first patch just contains
castNode(), the second one a rebased version of Peter's changes (with
some long lines broken up).

Thanks for finishing that up. I have committed more uses that I had
lying around partially done. Looks nice now.

With commit 38d103763d14baddf3cbfe4b00b501059fc9447f, I'm now getting a
compiler warning:

indxpath.c: In function 'generate_bitmap_or_paths':
indxpath.c:1312: warning: unused variable 'rinfo'

I have: gcc version 4.4.7 20120313 (Red Hat 4.4.7-17) (GCC)

No arguments to ./configure are needed.

Cheers,

Jeff

Jeff Janes <jeff.janes@gmail.com> writes:

With commit 38d103763d14baddf3cbfe4b00b501059fc9447f, I'm now getting a
compiler warning:
indxpath.c: In function 'generate_bitmap_or_paths':
indxpath.c:1312: warning: unused variable 'rinfo'

Me too, without asserts. Fixed.

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