[prev in list] [next in list] [prev in thread] [next in thread]
List: postgresql-general
Subject: Re: [HACKERS] deparsing utility commands
From: "Shulgin, Oleksandr" <oleksandr.shulgin () zalando ! de>
Date: 2015-07-31 13:45:43
Message-ID: CACACo5SLuaJ79TGX-ZUHCEYrZ411JxgyVeTyP6G639Kfyec8sQ () mail ! gmail ! com
[Download RAW message or body]
On Wed, Jul 29, 2015 at 12:44 PM, Shulgin, Oleksandr <
oleksandr.shulgin@zalando.de> wrote:
> On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera <alvherre@2ndquadrant.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
[Attachment #3 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 29, 2015 \
at 12:44 PM, Shulgin, Oleksandr <span dir="ltr"><<a \
href="mailto:oleksandr.shulgin@zalando.de" \
target="_blank">oleksandr.shulgin@zalando.de</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div \
class="gmail_quote"><span class="">On Tue, May 12, 2015 at 12:25 AM, Alvaro Herrera \
<span dir="ltr"><<a href="mailto:alvherre@2ndquadrant.com" \
target="_blank">alvherre@2ndquadrant.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span>David Steele wrote:<br> <br>
> I have reviewed and tested this patch and everything looks good to me.<br>
> It also looks like you added better coverage for schema DDL, which is a<br>
> welcome addition.<br>
<br>
</span>Thanks -- I have pushed this \
now.<br></blockquote><div><br></div></span><div>Hi,</div><div><br></div><div>I've \
tried compiling the 0003-ddl_deparse-extension part from <a \
href="http://www.postgresql.org/message-id/20150409161419.GC4369@alvh.no-ip.org" \
target="_blank">http://www.postgresql.org/message-id/20150409161419.GC4369@alvh.no-ip.org</a> \
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 ;-).</div><div><br></div><div>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.</div><div><br></div><div>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():</div><div><br></div><div><div><span \
style="white-space:pre-wrap"> </span>if (typemod >= 0)</div><div><span \
style="white-space:pre-wrap"> </span>*typemodstr = printTypmod(NULL, typemod, \
typeform->typmodout);</div><div><span \
style="white-space:pre-wrap"> </span>else</div><div><span \
style="white-space:pre-wrap"> </span>*typemodstr = \
pstrdup("");</div></div><div><br></div><div>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?</div></div></div></div></blockquote><div><br></div><div>Testing shows this \
is a bug indeed, easily triggered by deparsing any type with \
typmod.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> My hope is to get this test module extended quite a bit, not \
only to<br> cover existing commands, but also so that it causes future changes to<br>
cause failure unless command collection is considered. (In a previous<br>
version of this patch, there was a test mode that ran everything in the<br>
serial_schedule of regular regression tests. That was IMV a good way to<br>
ensure that new commands were also tested to run under command<br>
collection. That hasn't been enabled in the new test module, and I<br>
think it's necessary.)<br></blockquote><div> <br></div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> If anyone wants to contribute to the test module so that \
more is<br> covered, that would be much \
appreciated.<br></blockquote><div><br></div></span><div>I'm planning to have a \
look at this part also.</div></div></div></div></blockquote><div> </div><div>While \
running deparsecheck suite I'm getting a number of oddly looking \
errors:</div><div><br></div><div>WARNING: state: 42883 errm: operator does not \
exist: pg_catalog.oid = pg_catalog.oid<br></div><div><br></div><div>This is caused by \
deparsing create view, e.g.:</div><div><div><br></div><div>STATEMENT: create view \
v1 as select * from t1 ;</div><div>ERROR: operator does not exist: pg_catalog.oid = \
pg_catalog.oid at character 52</div><div>HINT: No operator matches the given name \
and argument type(s). You might need to add explicit type casts.</div><div>QUERY: \
SELECT * FROM pg_catalog.pg_rewrite WHERE ev_class = $1 AND rulename = \
$2</div><div>CONTEXT: PL/pgSQL function test_ddl_deparse() line 1 at FOR over \
SELECT rows</div></div><div><br></div><div>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.</div><div><br></div><div>Thoughts?</div><div><br></div><div>--</div><div>Alex</div><div><br></div></div>
</div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic