Обсуждение: Re: [COMMITTERS] pgsql: Add infrastructure to supportEphemeralNamedRelation references.

Hi,

On 2017-04-01 04:24:25 +0000, Kevin Grittner wrote:
> Add infrastructure to support EphemeralNamedRelation references.

My compiler, quite justifiedly, complains:

/home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
/home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero
characterconstant [-Wpointer-compare] 
      (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
                                           ^~
/home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the pointer?
      (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
       ^

- Andres

On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres@anarazel.de> wrote:

> My compiler, quite justifiedly, complains:
>
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and zero
characterconstant [-Wpointer-compare] 
>       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>                                            ^~
> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the
pointer?
>       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>        ^

Good catch.  Will push a change from list_nth() to list_nth_oid()
for the benefit of stricter compilers.  While I'm at it, I'll throw
on another layer of parentheses to ensure people read that
correctly.  Out of curiosity, what compiler or setting catches this?

--
Kevin Grittner

On Fri, Apr 7, 2017 at 10:03 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres@anarazel.de> wrote:
>
>> My compiler, quite justifiedly, complains:
>>
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and
zerocharacter constant [-Wpointer-compare] 
>>       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>>                                            ^~
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the
pointer?
>>       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
>>        ^
>
> Good catch.  Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers.  While I'm at it, I'll throw
> on another layer of parentheses to ensure people read that
> correctly.  Out of curiosity, what compiler or setting catches this?

Doesn't it also have the logic backwards?  According to the comment,
the attribute is dropped if the type *is* InvalidOid, so we want
result == true in that case.  But I don't actually know how to reach
this code to test it.

                /*
-                * We checked when we loaded ctecoltypes for the tuplestore
+                * We checked when we loaded coltypes for the tuplestore
                 * that InvalidOid was only used for dropped columns, so it is
                 * safe to count on that here.
                 */
                result =
-                       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
+                       (list_nth_oid(rte->coltypes, attnum - 1) == InvalidOid);
            }

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

On 2017-04-06 17:03:20 -0500, Kevin Grittner wrote:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres@anarazel.de> wrote:
>
> > My compiler, quite justifiedly, complains:
> >
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c: In function ‘get_rte_attribute_is_dropped’:
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and
zerocharacter constant [-Wpointer-compare] 
> >       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> >                                            ^~
> > /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:7: note: did you mean to dereference the
pointer?
> >       (list_nth(rte->coltypes, attnum - 1) != InvalidOid);
> >        ^
>
> Good catch.  Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers.  While I'm at it, I'll throw
> on another layer of parentheses to ensure people read that
> correctly.  Out of curiosity, what compiler or setting catches this?

gcc-7 here, and the specific warning is -Wpointer-compare.

- Andres

On Thu, Apr 6, 2017 at 5:07 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

> Doesn't it also have the logic backwards?  According to the comment,
> the attribute is dropped if the type *is* InvalidOid, so we want
> result == true in that case.  But I don't actually know how to reach
> this code to test it.

You're right.  So on this one line I had a reverse logic bug,
something that strict compilers would not accept, and code that
assumed that readers and compilers would always know that tests for
equality or inequality bind tighter than assignment.

I'll commit this fix first so I don't hold up Andres or break any
picky buildfarm critters and then see whether I can't manage to get
the tests to cover this code.

Thanks!


On Thu, Apr 6, 2017 at 5:16 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-04-06 17:03:20 -0500, Kevin Grittner wrote:
>> Out of curiosity, what compiler or setting catches this?
>
> gcc-7 here, and the specific warning is -Wpointer-compare.

Thanks!  I'll add that to my builds.

--
Kevin Grittner

Kevin Grittner <kgrittn@gmail.com> writes:
> On Thu, Apr 6, 2017 at 4:19 PM, Andres Freund <andres@anarazel.de> wrote:
>> My compiler, quite justifiedly, complains:
>> /home/andres/src/postgresql/src/backend/parser/parse_relation.c:2899:43: warning: comparison between pointer and
zerocharacter constant [-Wpointer-compare] 
>> (list_nth(rte->coltypes, attnum - 1) != InvalidOid);

> Good catch.  Will push a change from list_nth() to list_nth_oid()
> for the benefit of stricter compilers.

If the problem is that the list is an OID list, then why didn't the
"Assert(IsPointerList(list))" in list_nth fire?  Either this is the
wrong fix, or this code has never been exercised (at least not in
an assert-enabled build).

rte->coltypes certainly ought to be an OID list, so I lean to the
inadequate-testing theory ...

            regards, tom lane

On Thu, Apr 6, 2017 at 5:20 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

> I'll commit this fix first so I don't hold up Andres or break any
> picky buildfarm critters

Done.

> and then see whether I can't manage to get
> the tests to cover this code.

The function in question is only called from rewrite, and here's the
relevant comment:

 * About JOINs and dropped columns: although the parser never includes an
 * already-dropped column in a JOIN RTE's alias var list, it is possible for
 * such a list in a stored rule to include references to dropped columns.
 * (If the column is not explicitly referenced anywhere else in the query,
 * the dependency mechanism won't consider it used by the rule and so won't
 * prevent the column drop.)  To support get_rte_attribute_is_dropped(), we
 * replace join alias vars that reference dropped columns with null pointers.

So, to test this I guess I need to create a view that does SELECT *
on a table, write a plpgsql trigger function and use it as an AFTER
EACH STATEMENT trigger on that table -- referencing the view and
explicitly using a specific column from the transition table in a
join qual, modify the table so the trigger gets fired and the
function gets cached, ALTER the table to drop the column so
referenced without doing anything that might cause the function plan
to be discarded from cache, and then modify the table again to fire
the cached trigger.  Does that seem like the right test to add?  Or
would even that fail to reach this code because the transition table
is not on the view?

Oh well, I guess I'll write the code and find out -- seems easier
than reverse-engineering that code path.

--
Kevin Grittner

On Thu, Apr 6, 2017 at 6:10 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

>  * About JOINs and dropped columns: although the parser never includes an
>  * already-dropped column in a JOIN RTE's alias var list, it is possible for
>  * such a list in a stored rule to include references to dropped columns.
>  * (If the column is not explicitly referenced anywhere else in the query,
>  * the dependency mechanism won't consider it used by the rule and so won't
>  * prevent the column drop.)  To support get_rte_attribute_is_dropped(), we
>  * replace join alias vars that reference dropped columns with null pointers.

test=# create table t1 (t1id int not null, t1desc text);
CREATE TABLE
test=# create table t2 (t1id int not null, t2id int not null, t2desc text);
CREATE TABLE
test=# create view v1 as select t2desc from t1 join t2 on t1.t1id = t2.t1id;
CREATE VIEW
test=# alter table t1 drop column t1id;
ERROR:  cannot drop table t1 column t1id because other objects depend on it
DETAIL:  view v1 depends on table t1 column t1id
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

Is that comment wrong?

--
Kevin Grittner

On Fri, Apr 7, 2017 at 8:26 PM, Euler Taveira <euler@timbira.com.br> wrote:
> 2017-04-07 13:06 GMT-03:00 Kevin Grittner <kgrittn@gmail.com>:
>>
>> ERROR:  cannot drop table t1 column t1id because other objects depend on
>> it
>> DETAIL:  view v1 depends on table t1 column t1id
>> HINT:  Use DROP ... CASCADE to drop the dependent objects too.
>>
>> Is that comment wrong?
>
> Sort of. If you consider ALTER TABLE ... DROP COLUMN ... CASCADE, it is not
> that wrong. However, if you want to be strict, there should be a check to
> identify a table column and then hint a specific message (ALTER instead of
> DROP).

I think you missed the point -- I wasn't talking about the error
message or its associated DETAIL or HINT.  I was talking about this
C comment:

>  * About JOINs and dropped columns: although the parser never includes an
>  * already-dropped column in a JOIN RTE's alias var list, it is possible for
>  * such a list in a stored rule to include references to dropped columns.
>  * (If the column is not explicitly referenced anywhere else in the query,
>  * the dependency mechanism won't consider it used by the rule and so won't
>  * prevent the column drop.)  To support get_rte_attribute_is_dropped(), we
>  * replace join alias vars that reference dropped columns with null pointers.

"If the column is not explicitly referenced anywhere else in the
query [other than JOIN conditions, as I read it], the dependency
mechanism won't consider it used by the rule and so won't prevent
the column drop."

In my example the only place the column was explicitly referenced
was in a join condition, yet the dependency was recognized and the
column drop was prevented.  Am I missing something or did someone
invalidate that comment without changing it to reflect the new
reality.  It would appear that, under current conditions, there is
no way to reach the code which went untested.  At least, I can't see
it.

--
Kevin Grittner