Re: deparsing utility commands

On Wed, Jul 29, 2015 at 12:44 PM, Shulgin, Oleksandr <
oleksandr(dot)shulgin(at)zalando(dot)de> wrote:

> On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com
> > wrote:
>
>> David Steele wrote:
>>
>> > I have reviewed and tested this patch and everything looks good to me.
>> > It also looks like you added better coverage for schema DDL, which is a
>> > welcome addition.
>>
>> Thanks -- I have pushed this now.
>>
>
> Hi,
>
> I've tried compiling the 0003-ddl_deparse-extension part from
> http://www.postgresql.org/message-id/20150409161419.GC4369@alvh.no-ip.org
> on current master and that has failed because the 0002 part hasn't been
> actually pushed (I've asked Alvaro off the list about this, that's how I
> know the reason ;-).
>
> I was able to update the 0002 part so it applies cleanly (updated version
> attached), and then the contrib module compiles after one minor change and
> seems to work.
>
> I've started to look into what it would take to move 0002's code to the
> extension itself, and I've got a question about use of printTypmod() in
> format_type_detailed():
>
> if (typemod >= 0)
> *typemodstr = printTypmod(NULL, typemod, typeform->typmodout);
> else
> *typemodstr = pstrdup("");
>
> Given that printTypmod() does psprintf("%s%s") one way or the other,
> shouldn't we pass an empty string here instead of NULL as typname argument?
>

Testing shows this is a bug indeed, easily triggered by deparsing any type
with typmod.

My hope is to get this test module extended quite a bit, not only to
>> cover existing commands, but also so that it causes future changes to
>> cause failure unless command collection is considered. (In a previous
>> version of this patch, there was a test mode that ran everything in the
>> serial_schedule of regular regression tests. That was IMV a good way to
>> ensure that new commands were also tested to run under command
>> collection. That hasn't been enabled in the new test module, and I
>> think it's necessary.)
>>
>
>
>> If anyone wants to contribute to the test module so that more is
>> covered, that would be much appreciated.
>>
>
> I'm planning to have a look at this part also.
>

While running deparsecheck suite I'm getting a number of oddly looking
errors:

WARNING: state: 42883 errm: operator does not exist: pg_catalog.oid =
pg_catalog.oid

This is caused by deparsing create view, e.g.:

STATEMENT: create view v1 as select * from t1 ;
ERROR: operator does not exist: pg_catalog.oid = pg_catalog.oid at
character 52
HINT: No operator matches the given name and argument type(s). You might
need to add explicit type casts.
QUERY: SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND
rulename = $2
CONTEXT: PL/pgSQL function test_ddl_deparse() line 1 at FOR over SELECT
rows

The pg_rewrite query comes from ruleutils.c, while ddl_deparse.c calls it
through pg_get_viewdef_internal() but don't understand how is it different
from e.g., select pg_get_viewdef(...), and that last one is not affected.

Thoughts?

--
Alex