Thread: clearing opfuncid vs. parallel query
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
Attachment
Robert Haas <robertmhaas@gmail.com> 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
On Wed, Sep 23, 2015 at 4:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> 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
On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> 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
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Wed, Sep 23, 2015 at 7:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
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
Andres Freund <andres@anarazel.de> 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
On Thu, Sep 24, 2015 at 11:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@anarazel.de> 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
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> 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
Robert Haas <robertmhaas@gmail.com> 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
On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
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
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 <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev <u.zhuravlev@postgrespro.ru> 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
On Thursday 22 October 2015 12:53:49 you wrote: > On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev > > <u.zhuravlev@postgrespro.ru> 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
On Thu, Oct 22, 2015 at 1:19 PM, YUriy Zhuravlev <u.zhuravlev@postgrespro.ru> 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
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
On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev <u.zhuravlev@postgrespro.ru> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev
> <u.zhuravlev@postgrespro.ru> 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
On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Oct 22, 2015 at 1:40 PM, YUriy Zhuravlev >> <u.zhuravlev@postgrespro.ru> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Oct 22, 2015 at 4:32 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
> 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
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
On Fri, Oct 23, 2015 at 12:31 PM, YUriy Zhuravlev <u.zhuravlev@postgrespro.ru> wrote:
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
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
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
On Thu, Oct 22, 2015 at 05:14:29PM -0400, Robert Haas wrote:
> On Thu, Oct 22, 2015 at 5:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> 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.