Re: plpgsql.extra_warnings='num_into_expressions'

Hi again,

Here's a patch which allows you to notice those annoying bugs with INTO
slightly more quickly. Adding to the next commit phest.

.marko

Hi

I looked on this patch and I am thinking so it is not a good idea. It
introduce early dependency between functions and pg_class based objects.

This check should not be integrated to function validation directly.

We can integrate it to plpgsql_check

Regards

Pavel

2014-07-21 22:56 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> Hi again,
>
> Here's a patch which allows you to notice those annoying bugs with INTO
> slightly more quickly. Adding to the next commit phest.
>
>
> .marko
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On 7/22/14, 7:06 AM, Pavel Stehule wrote:
> I looked on this patch and I am thinking so it is not a good idea. It
> introduce early dependency between functions and pg_class based objects.

What dependency? The patch only looks at the raw parser output, so it
won't e.g. know whether SELECT * INTO a, b FROM foo; is problematic or
not.

.marko

2014-07-22 8:52 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> On 7/22/14, 7:06 AM, Pavel Stehule wrote:
>
>> I looked on this patch and I am thinking so it is not a good idea. It
>> introduce early dependency between functions and pg_class based objects.
>>
>
> What dependency? The patch only looks at the raw parser output, so it
> won't e.g. know whether SELECT * INTO a, b FROM foo; is problematic or
> not.
>

I am sorry, I was confused

There is dependencty in variable type, but this dependency is not new.

Regards

Pavel

>
>
> .marko
>

On 7/21/14, 10:56 PM, I wrote:
> Here's a patch which allows you to notice those annoying bugs with INTO
> slightly more quickly. Adding to the next commit phest.

New version, fixed bugs with set operations and VALUES lists.

.marko

On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:
> On 7/21/14, 10:56 PM, I wrote:
>> Here's a patch which allows you to notice those annoying bugs with INTO
>> slightly more quickly. Adding to the next commit phest.
>
> New version, fixed bugs with set operations and VALUES lists.

Looks good.

It seems weird to pass a PLpgSQL_row struct to check_sql_expr.
check_sql_expr only needs to know how many attributes is expected to be
in the target list, so it would be more natural to just pass an "int
expected_natts".

Once you do that, you could trivially also add checking for other cases,
e.g:

do $$
declare
i int4;
begin
-- fails at runtime, because "SELECT 1,3" returns two attributes,
-- but FOR expects 1
for i in 1,3..(2) loop
raise notice 'foo %', i;
end loop;
end;
$$;

There's probably more checking like that that you could add, but that
can be done as add-on patches, if ever. The INTO mistake happens a lot
more easily.

- Heikki

On 8/21/14, 1:19 PM, Heikki Linnakangas wrote:
> On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:
>> On 7/21/14, 10:56 PM, I wrote:
>>> Here's a patch which allows you to notice those annoying bugs with INTO
>>> slightly more quickly. Adding to the next commit phest.
>>
>> New version, fixed bugs with set operations and VALUES lists.
>
> Looks good.
>
> There's probably more checking like that that you could add, but that
> can be done as add-on patches, if ever. The INTO mistake happens a lot
> more easily.

Yeah, I think the mistake is at least as easy to do in "FOR .. IN
<query>", and I'm planning to add checks for that as well. But I
haven't found the time to look at it amongst all the other patches and
projects I have going (and also, unfortunately, I'm on vacation right now).

> It seems weird to pass a PLpgSQL_row struct to check_sql_expr.
> check_sql_expr only needs to know how many attributes is expected to be
> in the target list, so it would be more natural to just pass an "int
> expected_natts".

I'm not sure about this, though. AFAICT all the interesting cases are
already holding a PLpgSQL_row, and in that case it seems easier to just
pass that in to check_sql_expr() without making the callers worry about
extracting the expected_natts from the row. And we can always change
the interface should such a case come up, since the interface is
completely internal. Just my 0.02EUR, of course.

.marko

On 08/21/2014 02:09 PM, Marko Tiikkaja wrote:
> On 8/21/14, 1:19 PM, Heikki Linnakangas wrote:
>> On 08/07/2014 01:11 AM, Marko Tiikkaja wrote:
>>> On 7/21/14, 10:56 PM, I wrote:
>>>> Here's a patch which allows you to notice those annoying bugs with INTO
>>>> slightly more quickly. Adding to the next commit phest.
>>>
>>> New version, fixed bugs with set operations and VALUES lists.
>>
>> Looks good.
>>
>> There's probably more checking like that that you could add, but that
>> can be done as add-on patches, if ever. The INTO mistake happens a lot
>> more easily.
>
> Yeah, I think the mistake is at least as easy to do in "FOR .. IN
> <query>", and I'm planning to add checks for that as well. But I
> haven't found the time to look at it amongst all the other patches and
> projects I have going

Ok.

> (and also, unfortunately, I'm on vacation right now).

Oh, have fun!

>> It seems weird to pass a PLpgSQL_row struct to check_sql_expr.
>> check_sql_expr only needs to know how many attributes is expected to be
>> in the target list, so it would be more natural to just pass an "int
>> expected_natts".
>
> I'm not sure about this, though. AFAICT all the interesting cases are
> already holding a PLpgSQL_row, and in that case it seems easier to just
> pass that in to check_sql_expr() without making the callers worry about
> extracting the expected_natts from the row.

Hmm. The integer FOR syntax I used in my example does not, it always
expects 1 output column.

> And we can always change
> the interface should such a case come up, since the interface is
> completely internal. Just my 0.02EUR, of course.

You might want to add a helper function to count the number of
attributes in a PLpgSQL_row. Then the check_sql_expr call would be
almost as simple: check_sql_expr(..., get_row_natts(row)).

- Heikki