Re: [HACKERS] clearing opfuncid vs. parallel query

[Noah Misch]

On Thu, Oct 22, 2015 at 05:14:29PM -0400, Robert Haas wrote:
> On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> Despite everybody's best efforts, we mess this up more than is really
> >> desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
> >> in a whole bunch of preceding commits, and I wonder if anybody else
> >> would have found those if Noah hadn't.  It's just too easy to miss
> >> these things today.  If generating the .c files isn't practical,
> >> another alternative worth exploring would be a tool to cross-check
> >> them against the .h files.

FWIW, such a tool found the bugs I fixed in 53bbc68, b5eccae, b8fe12a.  My
script generates {copy,equal,out,read}funcs.c functions from the headers and
diffs each function against its hand-maintained counterpart.  I read through
the diff for anything suspicious.  (Most functions with deliberate nonstandard
behavior carry a comment about it.)

> > Yeah, I could get on board with that.  It doesn't seem terribly hard:
> > just make sure that all fields mentioned in the struct declaration are
> > mentioned in each relevant routine.  (Cases where we intentionally skip
> > a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")
> >
> > It would be nice if we could also check that the macro type is sane for
> > the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
> > But that would probably increase the difficulty very substantially for
> > just a small gain in error detection.
> 
> I actually think that's quite an easy mistake to make, so I'd be happy
> if we could catch it.  But anything would be better than nothing.

So far, type mismatch defects have been no less common than neglecting a field
entirely.


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

> I thought that's what you were proposing.  Process the struct
> definitions and emit .c files.
We have 2 ways. The first is always to generate the * .c files from the * .h 
files. Another way is to generate once from * .h file a XML/JSON and after 
generate from it to * .c files (parsing xml/json easy). 

> Anything that is part of the build process will have to be done in C or
> Perl.
I know about the relationship between Postgres and C / Perl. Yet this is not 
the language which would be worth to do something associated with code 
generation. Python is better in many ways than the Perl. I'm not trying to 
convince someone. I just see the situation, and I do not like.

What do you think about the format of the serialization? Now it is very 
primitive. For example, there are selected data in order, rather than by key. 
In its development, I used jsonb, it also helped to simplify the saved of 
query plans in the Postgres table.

Thanks.

-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

On Thursday 22 October 2015 09:26:46 David Fetter wrote:
> On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:
> > Hello.
> > Currently using nodeToString and stringToNode you can not pass a
> > full plan. In this regard, what is the plan to fix it? Or in the
> > under task parallel query does not have such a problem?
> > 
> > > This turns out not to be straightforward to code, because we don't
> > > have a generic plan tree walker,
> > 
> > I have an inner development. I am using python analyzing header
> > files and generates a universal walker (parser, paths ,executer and
> > etc trees), as well as the serializer and deserializer to jsonb.
> > Maybe I should publish this code?
> 
> Please do.
Tom Lane and Robert Haas are very unhappy with a python. Is there any reason? 

Thanks!

-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Alexander Korotkov]

On Fri, Oct 23, 2015 at 12:31 PM, YUriy Zhuravlev <
u.zhurav...@postgrespro.ru> wrote:

> On Thursday 22 October 2015 09:26:46 David Fetter wrote:
> > On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:
> > > Hello.
> > > Currently using nodeToString and stringToNode you can not pass a
> > > full plan. In this regard, what is the plan to fix it? Or in the
> > > under task parallel query does not have such a problem?
> > >
> > > > This turns out not to be straightforward to code, because we don't
> > > > have a generic plan tree walker,
> > >
> > > I have an inner development. I am using python analyzing header
> > > files and generates a universal walker (parser, paths ,executer and
> > > etc trees), as well as the serializer and deserializer to jsonb.
> > > Maybe I should publish this code?
> >
> > Please do.
> Tom Lane and Robert Haas are very unhappy with a python. Is there any
> reason?
>

Requirement of python with pycparser as build dependency is a
serious cataclysm. For instance, how many buildfarms will survive it?
This is why Tom and Robert are looking for ways to evade it.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

On Friday 23 October 2015 12:41:50 you wrote:
> Requirement of python with pycparser as build dependency is a
> serious cataclysm. For instance, how many buildfarms will survive it?
> This is why Tom and Robert are looking for ways to evade it.

I agree. But it is also a fact that Perl less suited for such development. 
Also at the moment Python is more common:
http://www.tiobe.com/index.php/content/paperinfo/tpci/index.html


-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
 wrote:
> On Thursday 22 October 2015 13:25:52 you wrote:
>> It would be more useful, if we're going to autogenerate code,
> Are we going to use autogenerate code?

I thought that's what you were proposing.  Process the struct
definitions and emit .c files.

>> to do it from the actual struct definitions.
> I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C
> code much more difficult. But my current generator just use the structure from
> the header files (by pycparser).

Anything that is part of the build process will have to be done in C or Perl.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

On Thursday 22 October 2015 13:25:52 you wrote:
> It would be more useful, if we're going to autogenerate code,
Are we going to use autogenerate code?

> to do it from the actual struct definitions.
I can gen xml/json from actual struct. I offered XML/JSON as the analysis of C 
code much more difficult. But my current generator just use the structure from 
the header files (by pycparser).

Thanks.
-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

On Thursday 22 October 2015 12:53:49 you wrote:
> On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev
> 
>  wrote:
> > Hello.
> > Currently using nodeToString and stringToNode you can not pass a full
> > plan. In this regard, what is the plan to fix it? Or in the under task
> > parallel query does not have such a problem?
> 
> It's already fixed.  See commits
> a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
> 9f1255ac859364a86264a67729dbd1a36dd63ff2.

Ahh. Thanks.

And then another question:
What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c, 
outfuncs.c from XML or JSON?
In order not to change the code in four places when changing nodes. 

-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Oct 22, 2015 at 1:19 PM, YUriy Zhuravlev
 wrote:
> And then another question:
> What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c,
> outfuncs.c from XML or JSON?
> In order not to change the code in four places when changing nodes.

It would be more useful, if we're going to autogenerate code, to do it
from the actual struct definitions.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
>>  wrote:
>>> I can gen xml/json from actual struct. I offered XML/JSON as the analysis 
>>> of C
>>> code much more difficult. But my current generator just use the structure 
>>> from
>>> the header files (by pycparser).
>
>> Anything that is part of the build process will have to be done in C or Perl.
>
> Yeah.  The bar for introducing new build tool requirements is very high;
> *way* higher than the likely benefit from not having to hand-maintain
> outfuncs.c et al.  If you wanna do this in Perl, fine, but we're not going
> to introduce a requirement to have Python to build just because somebody
> wants to write a tool in the latter not the former.
>
> Having said that, there is more knowledge embedded in the nodes/*.c files
> than there is in the nodes/*.h headers; an example being that there are
> certain fields that we deliberately don't dump and restore.  (This is
> still true despite Robert's recent changes to take opfuncid out of that
> class.)

Are those things, ah, documented somewhere?

> I'm not sure that you could get to a point where you were
> generating this stuff from anything that wasn't in essence an arcane
> representation of the .c files.  It might be slightly harder to make
> errors of omission that way, but I'm suspicious that that would come at
> the cost of a net loss of readability.

That is possible, but the current situation isn't good either.
Despite everybody's best efforts, we mess this up more than is really
desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
in a whole bunch of preceding commits, and I wonder if anybody else
would have found those if Noah hadn't.  It's just too easy to miss
these things today.  If generating the .c files isn't practical,
another alternative worth exploring would be a tool to cross-check
them against the .h files.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane  wrote:
>> I'm not sure that you could get to a point where you were
>> generating this stuff from anything that wasn't in essence an arcane
>> representation of the .c files.  It might be slightly harder to make
>> errors of omission that way, but I'm suspicious that that would come at
>> the cost of a net loss of readability.

> That is possible, but the current situation isn't good either.
> Despite everybody's best efforts, we mess this up more than is really
> desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
> in a whole bunch of preceding commits, and I wonder if anybody else
> would have found those if Noah hadn't.  It's just too easy to miss
> these things today.  If generating the .c files isn't practical,
> another alternative worth exploring would be a tool to cross-check
> them against the .h files.

Yeah, I could get on board with that.  It doesn't seem terribly hard:
just make sure that all fields mentioned in the struct declaration are
mentioned in each relevant routine.  (Cases where we intentionally skip
a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")

It would be nice if we could also check that the macro type is sane for
the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
But that would probably increase the difficulty very substantially for
just a small gain in error detection.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
>  wrote:
>> I can gen xml/json from actual struct. I offered XML/JSON as the analysis of 
>> C
>> code much more difficult. But my current generator just use the structure 
>> from
>> the header files (by pycparser).

> Anything that is part of the build process will have to be done in C or Perl.

Yeah.  The bar for introducing new build tool requirements is very high;
*way* higher than the likely benefit from not having to hand-maintain
outfuncs.c et al.  If you wanna do this in Perl, fine, but we're not going
to introduce a requirement to have Python to build just because somebody
wants to write a tool in the latter not the former.

Having said that, there is more knowledge embedded in the nodes/*.c files
than there is in the nodes/*.h headers; an example being that there are
certain fields that we deliberately don't dump and restore.  (This is
still true despite Robert's recent changes to take opfuncid out of that
class.)  I'm not sure that you could get to a point where you were
generating this stuff from anything that wasn't in essence an arcane
representation of the .c files.  It might be slightly harder to make
errors of omission that way, but I'm suspicious that that would come at
the cost of a net loss of readability.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane  wrote:
>>> I'm not sure that you could get to a point where you were
>>> generating this stuff from anything that wasn't in essence an arcane
>>> representation of the .c files.  It might be slightly harder to make
>>> errors of omission that way, but I'm suspicious that that would come at
>>> the cost of a net loss of readability.
>
>> That is possible, but the current situation isn't good either.
>> Despite everybody's best efforts, we mess this up more than is really
>> desirable. Commit b8fe12a83622b350dc6849f8bb933bd8a86c1424 fixed bugs
>> in a whole bunch of preceding commits, and I wonder if anybody else
>> would have found those if Noah hadn't.  It's just too easy to miss
>> these things today.  If generating the .c files isn't practical,
>> another alternative worth exploring would be a tool to cross-check
>> them against the .h files.
>
> Yeah, I could get on board with that.  It doesn't seem terribly hard:
> just make sure that all fields mentioned in the struct declaration are
> mentioned in each relevant routine.  (Cases where we intentionally skip
> a field could be handled by adding a no-op macro, eg "COPY_IGNORE(foo);")
>
> It would be nice if we could also check that the macro type is sane for
> the field datatype (eg, not use COPY_SCALAR_FIELD() for a pointer field).
> But that would probably increase the difficulty very substantially for
> just a small gain in error detection.

I actually think that's quite an easy mistake to make, so I'd be happy
if we could catch it.  But anything would be better than nothing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[David Fetter]

On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:
> Hello.
> Currently using nodeToString and stringToNode you can not pass a
> full plan. In this regard, what is the plan to fix it? Or in the
> under task parallel query does not have such a problem?
> 
> > This turns out not to be straightforward to code, because we don't
> > have a generic plan tree walker, 
> 
> I have an inner development. I am using python analyzing header
> files and generates a universal walker (parser, paths ,executer and
> etc trees), as well as the serializer and deserializer to jsonb.
> Maybe I should publish this code?

Please do.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[YUriy Zhuravlev]

Hello.
Currently using nodeToString and stringToNode you can not pass a full plan. In 
this regard, what is the plan to fix it? Or in the under task parallel query 
does not have such a problem?

> This turns out not to be straightforward to code, because we
> don't have a generic plan tree walker, 

I have an inner development. I am using python analyzing header files and 
generates a universal walker (parser, paths ,executer and etc trees), as well 
as the serializer and deserializer to jsonb. Maybe I should publish this code?

Thanks.

-- 
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev
 wrote:
> Hello.
> Currently using nodeToString and stringToNode you can not pass a full plan. In
> this regard, what is the plan to fix it? Or in the under task parallel query
> does not have such a problem?

It's already fixed.  See commits
a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
9f1255ac859364a86264a67729dbd1a36dd63ff2.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Wed, Sep 23, 2015 at 7:25 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane  wrote:
>>> Yeah, though I think of that as a longer-term issue, ie we could clean it
>>> up sometime later.
>
>> So, you're thinking of something as simple as the attached?
>
> Right.  I'll make a mental to-do to see about getting rid of set_opfuncid
> later.

Cool, thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Alvaro Herrera]

Tom Lane wrote:

> To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
> is on par with our very limited ability to alter the contents of
> an operator class.  In principle it would be nice, but the practical
> value is so small that it's not surprising it hasn't been done ---
> and we shouldn't continue to hold the door open for a simple way of
> implementing it when there are significant costs to doing so.

I think allowing an operator's implementation function to change would
be rather problematic, would it not?  There's no way to know whether the
semantic changes to stored rules would make sense, not least because the
person running ALTER OPERATOR wouldn't know (== has no easy way to find
out) what is being changed in the first place.

To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
to be implemented at all.

It's not like changing an operator's implementation is an oft-requested
feature anyway.

-- 
Álvaro Herrerahttp://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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Andres Freund]

On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
> Well, I can't vouch for what any human being on earth has thought
> about over a twenty-year period.  It's not intrinsically unreasonable
> in my mind to want to alter an operator to point at a different
> procedure.

Wouldn't we use plan invalidation to deal with that anyway?

Andres


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Andres Freund  writes:
> On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
>> Well, I can't vouch for what any human being on earth has thought
>> about over a twenty-year period.  It's not intrinsically unreasonable
>> in my mind to want to alter an operator to point at a different
>> procedure.

> Wouldn't we use plan invalidation to deal with that anyway?

Plan invalidation wouldn't help, because the obsolete data exists
on-disk in stored rules.  You'd have to run through the pg_rewrite
entries and update them.

To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class.  In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Sep 24, 2015 at 11:54 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
>>> Well, I can't vouch for what any human being on earth has thought
>>> about over a twenty-year period.  It's not intrinsically unreasonable
>>> in my mind to want to alter an operator to point at a different
>>> procedure.
>
>> Wouldn't we use plan invalidation to deal with that anyway?
>
> Plan invalidation wouldn't help, because the obsolete data exists
> on-disk in stored rules.  You'd have to run through the pg_rewrite
> entries and update them.
>
> To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
> is on par with our very limited ability to alter the contents of
> an operator class.  In principle it would be nice, but the practical
> value is so small that it's not surprising it hasn't been done ---
> and we shouldn't continue to hold the door open for a simple way of
> implementing it when there are significant costs to doing so.

Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it.  Or we could engineer something else.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Alvaro Herrera  writes:
> I think allowing an operator's implementation function to change would
> be rather problematic, would it not?  There's no way to know whether the
> semantic changes to stored rules would make sense, not least because the
> person running ALTER OPERATOR wouldn't know (== has no easy way to find
> out) what is being changed in the first place.

> To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
> to be implemented at all.

> It's not like changing an operator's implementation is an oft-requested
> feature anyway.

Well, the point is that usually anything you want in this line can be
accomplished by executing CREATE OR REPLACE FUNCTION on the operator's
function.  It's up to you that that doesn't create any interesting
semantic incompatibilities.  That would still be true for an ALTER
OPERATOR SET FUNCTION command: if you break it, you get to keep both
pieces.  But the availability of that alternative really cuts down
on the plausible use-cases for ALTER OPERATOR SET FUNCTION.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> Also, it's not like this change couldn't be UN-done at a future point.
> I mean, Tom didn't like the flag I added aesthetically, but if we
> needed it, we could have it.  Or we could engineer something else.

For the record: that's true for the patch you just committed.  But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Also, it's not like this change couldn't be UN-done at a future point.
>> I mean, Tom didn't like the flag I added aesthetically, but if we
>> needed it, we could have it.  Or we could engineer something else.
>
> For the record: that's true for the patch you just committed.  But once
> I remove the hopefully-now-dead planner support for recomputing opfuncid,
> it would get a lot more painful to reverse the decision.

True.  I think it's pretty wacky that we store the opfuncid in the
tree at all.  If somebody were to propose adding a dependent value of
that sort to a node type that didn't already have it, I suspect either
you or I would do our best to shoot that down.  The only possible
argument for having that in there at all is that the performance gains
from so doing are so large that we have no choice but to sacrifice a
principle to expediency.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane  wrote:
>> For the record: that's true for the patch you just committed.  But once
>> I remove the hopefully-now-dead planner support for recomputing opfuncid,
>> it would get a lot more painful to reverse the decision.

> True.  I think it's pretty wacky that we store the opfuncid in the
> tree at all.  If somebody were to propose adding a dependent value of
> that sort to a node type that didn't already have it, I suspect either
> you or I would do our best to shoot that down.  The only possible
> argument for having that in there at all is that the performance gains
> from so doing are so large that we have no choice but to sacrifice a
> principle to expediency.

Hm.  It might be interesting to try removing those node fields altogether
and see what it costs us.  Setting the field at parse time is zero-cost,
at least in the primary code path through make_op(), because we have our
hands on the pg_operator row at that time anyway.  (I have a vague
recollection that that was once not true, but it certainly is true now
and has been for a long time.)  Not having it would cost us one syscache
lookup per operator at execution start, and maybe more than that if there
are additional places that would need the function OID, which there
probably are --- volatility checks in the planner come to mind.  But
I'm not sure how significant that would be.  There are certainly plenty
of syscache lookups being done at plan startup anyway.

But this is mostly moot unless someone is actively planning to try to
implement ALTER OPERATOR SET FUNCTION.

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

[HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid.  This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change.  We'd want to look it up again rather than using the
previous value.

As previously discussed, parallel query will use outfuncs/readfuncs to
copy the Plan to be executed by a parallel worker to that worker.
That plan may contain expressions, and the round-trip through
outfuncs/readfuncs will lose their opfuncids.  In this case, that's
pretty annoying, if not outright wrong.  It's annoying because it
means that, after the worker reads the plan tree, it's got to iterate
over the whole thing and repeat the lookups of all the opfuncid
fields.  This turns out not to be straightforward to code, because we
don't have a generic plan tree walker, and even if we did, you still
need knowledge of which plan nodes have expressions inside which
fields, and we don't have a generic facility for that either: it's
there inside e.g. set_plan_refs, but not in a form other code can use.
Moreover, if we ever did have an ALTER OPERATOR command that could
change the pg_proc mapping, this would go from annoying to outright
broken, because it would be real bad if the worker and the leader came
to different conclusions about what opfuncid to use.  Maybe we could
add heavyweight locking to prevent that, but that would be expensive
and we surely don't have it today.

So I think we should abandon the approach Amit took, namely trying to
reset all of the opfuncids.  Instead, I think we should provide a
method of not throwing them out in the first place.  The attached
patch does by adding an "int flags" field to the relevant read
routines.  stringToNode() becomes a macro which passes
STRINGTONODE_INVALIDATE_OPFUNCID to stringToNodeExtended(), which is
one of the functions that now takes an additional "int flags"
argument.  If a caller doesn't wish to ignore opfuncid, they can call
stringToNodeExtended directly.  This way, existing stringToNode()
callers see no behavior change, but the parallel query code can easily
get the behavior that it wants.

Thoughts?  Better ideas?  Objections?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index 0dabfa7..87ece914 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -35,7 +35,7 @@ static char *pg_strtok_ptr = NULL;
  *	  returns a Node with a given legal ASCII representation
  */
 void *
-stringToNode(char *str)
+stringToNodeExtended(char *str, int flags)
 {
 	char	   *save_strtok;
 	void	   *retval;
@@ -50,7 +50,7 @@ stringToNode(char *str)
 
 	pg_strtok_ptr = str;		/* point pg_strtok at the string to read */
 
-	retval = nodeRead(NULL, 0); /* do the reading */
+	retval = nodeRead(NULL, 0, flags); /* do the reading */
 
 	pg_strtok_ptr = save_strtok;
 
@@ -275,7 +275,7 @@ nodeTokenType(char *token, int length)
  * this should only be invoked from within a stringToNode operation).
  */
 void *
-nodeRead(char *token, int tok_len)
+nodeRead(char *token, int tok_len, int flags)
 {
 	Node	   *result;
 	NodeTag		type;
@@ -293,7 +293,7 @@ nodeRead(char *token, int tok_len)
 	switch ((int) type)
 	{
 		case LEFT_BRACE:
-			result = parseNodeString();
+			result = parseNodeString(flags);
 			token = pg_strtok(_len);
 			if (token == NULL || token[0] != '}')
 elog(ERROR, "did not find '}' at end of input node");
@@ -359,7 +359,7 @@ nodeRead(char *token, int tok_len)
 		/* We have already scanned next token... */
 		if (token[0] == ')')
 			break;
-		l = lappend(l, nodeRead(token, tok_len));
+		l = lappend(l, nodeRead(token, tok_len, flags));
 		token = pg_strtok(_len);
 		if (token == NULL)
 			elog(ERROR, "unterminated List structure");
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ef88209..bf101ae 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -121,7 +121,7 @@
 #define READ_NODE_FIELD(fldname) \
 	token = pg_strtok();		/* skip :fldname */ \
 	(void) token;/* in case not used elsewhere */ \
-	local_node->fldname = nodeRead(NULL, 0)
+	local_node->fldname = nodeRead(NULL, 0, flags)
 
 /* Read a bitmapset field */
 #define READ_BITMAPSET_FIELD(fldname) \
@@ -222,7 +222,7 @@ _readBitmapset(void)
  * _readQuery
  */
 static Query *
-_readQuery(void)
+_readQuery(int flags)
 {
 	READ_LOCALS(Query);
 
@@ -266,7 +266,7 @@ _readQuery(void)
  * _readNotifyStmt
  */
 static NotifyStmt *
-_readNotifyStmt(void)
+_readNotifyStmt(int flags)
 {
 	READ_LOCALS(NotifyStmt);
 
@@ -280,7 +280,7 @@ _readNotifyStmt(void)
  * _readDeclareCursorStmt
  */
 static DeclareCursorStmt *

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Wed, Sep 23, 2015 at 4:31 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
>> DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
>> value in the newly-created node to InvalidOid.  This is because it
>> assumes we're reading the node from a pg_node_tree column in some
>> system catalog, and if in the future we wanted to allow an ALTER
>> OPERATOR command to change the pg_proc mapping, then the opfuncid
>> could change.  We'd want to look it up again rather than using the
>> previous value.
>
> Right, but considering that nobody has even thought about implementing
> such a command in the past twenty years, maybe we should just change
> the behavior of those read routines?

Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period.  It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.

But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too.  It would even save a
few cycles.  Would you also want to rip out the stuff that fixes up
opfuncid as dead code?  I assume yes, but sometimes I assume things
that are false.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> But if we're sure we don't want to support that, changing the behavior
> of the read routines would be fine with me, too.  It would even save a
> few cycles.  Would you also want to rip out the stuff that fixes up
> opfuncid as dead code?  I assume yes, but sometimes I assume things
> that are false.

Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.  I'm not sure right now that everyplace that initially
creates OpExpr etc. nodes is on board with having to supply opfuncid.
I do know that the main path through the parser provides 'em.  (So another
reason I don't like the current approach is that I doubt all code that
should theoretically be doing set_opfuncid() is actually doing so; it
would be too easy to miss the need for it in simple testing.)

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
> DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
> value in the newly-created node to InvalidOid.  This is because it
> assumes we're reading the node from a pg_node_tree column in some
> system catalog, and if in the future we wanted to allow an ALTER
> OPERATOR command to change the pg_proc mapping, then the opfuncid
> could change.  We'd want to look it up again rather than using the
> previous value.

Right, but considering that nobody has even thought about implementing
such a command in the past twenty years, maybe we should just change
the behavior of those read routines?  I've wondered for some time why
we don't just insist on the parser filling those nodes fully to begin
with, and get rid of the notion that assorted random places should
be expected to fix them up later.  This is one of the behaviors that
would have to change to support such a simplification.

> ... The attached
> patch does by adding an "int flags" field to the relevant read
> routines.

Ick.  TBH, this is just taking a bad design and putting another one
on top.

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Robert Haas]

On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> But if we're sure we don't want to support that, changing the behavior
>> of the read routines would be fine with me, too.  It would even save a
>> few cycles.  Would you also want to rip out the stuff that fixes up
>> opfuncid as dead code?  I assume yes, but sometimes I assume things
>> that are false.
>
> Yeah, though I think of that as a longer-term issue, ie we could clean it
> up sometime later.

So, you're thinking of something as simple as the attached?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index ef88209..08519ed 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -665,17 +665,6 @@ _readOpExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -696,17 +685,6 @@ _readDistinctExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -727,17 +705,6 @@ _readNullIfExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_OID_FIELD(opresulttype);
 	READ_BOOL_FIELD(opretset);
 	READ_OID_FIELD(opcollid);
@@ -758,17 +725,6 @@ _readScalarArrayOpExpr(void)
 
 	READ_OID_FIELD(opno);
 	READ_OID_FIELD(opfuncid);
-
-	/*
-	 * The opfuncid is stored in the textual format primarily for debugging
-	 * and documentation reasons.  We want to always read it as zero to force
-	 * it to be re-looked-up in the pg_operator entry.  This ensures that
-	 * stored rules don't have hidden dependencies on operators' functions.
-	 * (We don't currently support an ALTER OPERATOR command, but might
-	 * someday.)
-	 */
-	local_node->opfuncid = InvalidOid;
-
 	READ_BOOL_FIELD(useOr);
 	READ_OID_FIELD(inputcollid);
 	READ_NODE_FIELD(args);

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

Re: [HACKERS] clearing opfuncid vs. parallel query

[Tom Lane]

Robert Haas  writes:
> On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane  wrote:
>> Yeah, though I think of that as a longer-term issue, ie we could clean it
>> up sometime later.

> So, you're thinking of something as simple as the attached?

Right.  I'll make a mental to-do to see about getting rid of set_opfuncid
later.

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