Re: Commitfest Status: Sudden Death Overtime

All,

As you can probably tell, we are not ready to end the commitfest. (I
think Robert gave me this CF to show why even talking about a one-week
mini-fest is a fantasy. If so, successful.).

I've booted the patches which obviously aren't going to be immediately
ready. Nine patches are ready for committer. The rest fall into the
following buckets:

KAGAI's PATCHES

These are complex and need review by advanced hackers. They are also
often interdependant. As such, I'd like to automatically move his
patches to the next CF and ask that hackers who are engaged in working
on them continue to do so between CFs.

PATCHES WHICH I MOVED TO THE NEXT CF 9-2011:

* Collect frequency statistics and selectivity estimation for arrays
* Allow multiple Postgres clusters running on the same machine to
distinguish themselves in the event log
* lazy vxid locks

PATCHES WHICH HAVE BEEN UPDATED AND NEED FURTHER REVIEW:

* Cascade Replication

PATCHES STILL UNDER ACTIVE DISCUSSION:

* Add ability to constrain backend temporary file space

PATCHES I CAN'T FIGURE OUT THE STATUS OF:

* pg_comments system view
* Bugfix for XPATH() if expression returns a scalar value

So, down to a fairly limited list, except that we need a lot of committing.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Jul15, 2011, at 23:05 , Josh Berkus wrote:
> * Bugfix for XPATH() if expression returns a scalar value

Well, Peter Eisentraut seemed to disagree with my approach initially,
and seemed to prefer a separate function for XPATH expressions which
return a scalar value.

http://archives.postgresql.org/pgsql-hackers/2011-06/msg00401.php

I considered that, but came to the conclusion that it has problems
of it's own, described here:

http://archives.postgresql.org/pgsql-hackers/2011-06/msg00608.php

Peter stopped responding at that point, so I assumed that my argument
convinced him.

Radoslaw complained about the fact the results of scalar values
come back escaped from XPATH() with the patch applied (without it,
an empty array is returned) and wanted that changed - Basically the
same objection he had to my other patch which made sure text nodes
are properly escaped (The fine print here is that text nodes *aren't*
scalar values, they're nodes. What fun.). He did upgrade that other
patch to "Ready for Committer" despite his objections, though. I
don't know whether he wanted to do the same with this one or not,
and my inquiry was left unanswered

http://archives.postgresql.org/pgsql-hackers/2011-07/msg00783.php

I also don't know much code review the patch has received. I didn't
receive any complaints, but whether that reflects the quality of
the patch or the quantity of review I leave for someone else to judge.

I dunno what the best way forward is, but I'd hate to see this being
bumped to the next commit-fest.

best regards,
Florian Pflug

On Fri, Jul 15, 2011 at 5:05 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> As you can probably tell, we are not ready to end the commitfest.  (I
> think Robert gave me this CF to show why even talking about a one-week
> mini-fest is a fantasy.  If so, successful.).

Heh, heh. Well, it wasn't that premeditated, but I'm always glad to
reach a meeting of the minds. :-)

> These are complex and need review by advanced hackers.  They are also
> often interdependant.  As such, I'd like to automatically move his
> patches to the next CF and ask that hackers who are engaged in working
> on them continue to do so between CFs.

There are only two patches left and I think we really ought to try to
take a crack at doing something with them. Yeb is working on the
userspace access vector cache patch, which I think is going drag on
longer than we want keep the CommitFest open, but I'm OK with giving
it a day or two to shake out. Aside from the benefit of reviewing the
actual patch, I see that Yeb is pushing on KaiGai to do further work
on the documentation, which I think is of great value.

I will review the other patch - on shared object labels - RSN.

> PATCHES WHICH I MOVED TO THE NEXT CF 9-2011:
>
> * Collect frequency statistics and selectivity estimation for arrays
> * Allow multiple Postgres clusters running on the same machine to
> distinguish themselves in the event log
> * lazy vxid locks

I'm not clear on your criteria for moving these patches, as they seem
to be in somewhat different states. The first one is WIP, and it
doesn't really matter whether you move it to the next CommitFest or
just mark it returned with feedback. There is active development work
going on there, so it's not going to get committed at this point. But
the other two are basically just things we didn't get around to
reviewing.

With respect to the lazy vxid lock patch, it looks to me like Jeff has
done a bit of review, and I think the real question here is whether or
not we want to go ahead and make that change (with some stylistic and
cosmetic corrections) or wait until we have a more complex solution to
the spinlock contention problems mapped out. On a pgbench run with 8
clients on a 32-core machine, I see about a 2% speedup from that patch
on pgbench -S, and it grows to 8% at 32 clients. At 80 clients (but
still just a 32-core box), the results bounce around more, but taking
the median of three five-minute runs, it's an 11% improvement. To me,
that's enough to make it worth applying, especially considering that
what is 11% on today's master is, in raw TPS, equivalent to maybe 30%
of yesterday's master (prior to the fast relation lock patch being
applied). More, it seems pretty clear that this is the conceptually
right thing to do, even if it's going to require some work elsewhere
to file down all the rough edges thus exposed. If someone objects to
that, then OK, we should talk about that: but so far I don't think
anyone has expressed strong opposition: in which case I'd like to fix
it up and get it in.

With respect to the event-log patch, MauMau has resubmitted that to
the next CommitFest, so that's fine as far as it goes. But it might
make sense to see if we can twist Magnus's arm into re-reviewing it
now while it's fresh in his mind, rather than waiting another two or
three months.

> PATCHES WHICH HAVE BEEN UPDATED AND NEED FURTHER REVIEW:
>
> * Cascade Replication

No update.

> PATCHES STILL UNDER ACTIVE DISCUSSION:
>
> * Add ability to constrain backend temporary file space

Now committed, by Tom.

> PATCHES I CAN'T FIGURE OUT THE STATUS OF:
>
> * pg_comments system view

I reviewed this and marked it Returned with Feedback.

> * Bugfix for XPATH() if expression returns a scalar value

No update.

> So, down to a fairly limited list, except that we need a lot of committing.

We're down to three patches that fall into this category: two XML
patches by Florian, which have been somewhat derailed by an argument
about whether values of type xml should, in fact, be required to be
valid xml (I think you know my vote on this one...); and an
error-reporting patch that Tom weighed in on over the weekend. This
last suffers from the issue that it's not quite clear whether Tom is
going to do the work or whether he's expecting the submitter to do it.

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

On Fri, Jul 15, 2011 at 10:05 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> PATCHES WHICH HAVE BEEN UPDATED AND NEED FURTHER REVIEW:
>
> * Cascade Replication

Sorry, too busy reviewing to take note of this. I expect to commit
when its tested and ready.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> We're down to three patches that fall into this category: two XML
> patches by Florian, which have been somewhat derailed by an argument
> about whether values of type xml should, in fact, be required to be
> valid xml (I think you know my vote on this one...);

Yeah, it's not clear to me either which of those are still reasonable
candidates for committing as-is.

> and an
> error-reporting patch that Tom weighed in on over the weekend. This
> last suffers from the issue that it's not quite clear whether Tom is
> going to do the work or whether he's expecting the submitter to do it.

If you mean the business about allowing GUCs in postgresql.conf to be
applied even if there are semantic errors elsewhere, I'm just as happy
to let Alexey or Florian have a go at it first, if they want. The real
question at the moment is do we have consensus about changing that?
Because if we do, the submitted patch is certainly not something to
commit as-is, and should be marked Returned With Feedback.

regards, tom lane

Simon,

>> * Cascade Replication
>
> Sorry, too busy reviewing to take note of this. I expect to commit
> when its tested and ready.

So, would that be this commitfest, or next commitfest?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Robert,

>> * Collect frequency statistics and selectivity estimation for arrays
>> * Allow multiple Postgres clusters running on the same machine to
>> distinguish themselves in the event log
>> * lazy vxid locks
>
> I'm not clear on your criteria for moving these patches, as they seem
> to be in somewhat different states.

For the array criteria, it took me until the last week of the CF to find
a reviewer. That reviewer found a number of issues, and the submitter
submitted an updated patch. It looks quite likely that work on that
patch will get updated in the next couple weeks but not immediately.

For the eventlog, MauMau had submitted an updated patch, but all of our
Windows hackers had let me know that they were unavailable for the next
couple weeks for review or commit.

For lazy VXID locks, I'd the impression that we had an ongoing
discussion about whether we were going to apply it or not, and you were
on vacation for the last week of the CF.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Jul 18, 2011 at 4:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> and an
>> error-reporting patch that Tom weighed in on over the weekend.  This
>> last suffers from the issue that it's not quite clear whether Tom is
>> going to do the work or whether he's expecting the submitter to do it.
>
> If you mean the business about allowing GUCs in postgresql.conf to be
> applied even if there are semantic errors elsewhere, I'm just as happy
> to let Alexey or Florian have a go at it first, if they want.  The real
> question at the moment is do we have consensus about changing that?
> Because if we do, the submitted patch is certainly not something to
> commit as-is, and should be marked Returned With Feedback.

I'm not totally convinced. The proposed patch is pretty small, and
seems to stand on its own two feet. I don't hear anyone objecting to
your proposed plan, but OTOH it doesn't strike me as such a good plan
that we should reject all other improvements in the meantime. Maybe
I'm missing something...

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

On Jul18, 2011, at 22:19 , Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> We're down to three patches that fall into this category: two XML
>> patches by Florian, which have been somewhat derailed by an argument
>> about whether values of type xml should, in fact, be required to be
>> valid xml (I think you know my vote on this one...);
>
> Yeah, it's not clear to me either which of those are still reasonable
> candidates for committing as-is.

There are actually three XML-related patches from me in the queue.
I'll try to give an overview of their states - as perceived by me,
of course. If people want to comment on this, I suggest to do that
that either on the existing threads from these patches or on new ones,
but not on this one - lest confusion ensues.

* Bugfix for XPATH() if text or attribute nodes are selected
https://commitfest.postgresql.org/action/patch_view?id=580

Makes XPATH() return valid XML if text or attribute are selected.

I'm not aware of any issues with that one, other than Radoslaw's
unhappiness about the change of behaviour. Since the current behaviour
is very clearly broken (as in dump, reload, ka-Woom), I'm not prepared
to accept that as a show-stopper.

The question about whether values of type XML should or should
not be in fact valid XML is a red herring. That question has long
ago been settles by the XML type's input function, which has a pretty
clear and consistent idea about what constitutes a valid value of type
XML. The patch simply makes XPATH() abide by those rules, instead of
making up rules of it's own pretty nilly-willy.

* Bugfix for XPATH() if expression returns a scalar value
https://commitfest.postgresql.org/action/patch_view?id=565

Makes XPATH() support XPath expressions which compute a scalar value,
not a node set (i.e. apply a top-level function such as name()).
Currently, we return an empty array in that case.

Peter Eisentraut initially seemed to prefer creating separate functions
for that (XPATH_STRING, ...). I argued against that, on the grounds that
that'd make it impossible to evaluate user-supplied XPath expression (since
you wouldn't know which function to use). I haven't heard back from him
after that argument. Radoslaw liked the patch, but wanted the quoting
removed - same theme as with the other patch.

* XML error handling improvement to fix XPATH bug
https://commitfest.postgresql.org/action/patch_view?id=579

Like that first patch, it fixes a bug where XPATH() returns invalid
XML. The cause is completely different though. Here, libxml is (partially)
at fault - it's parsing methods always return a document instance if
a document is *structurally* valid, even if it contains semantic error
like invalid namespace references. But it isn't fully prepared to
actually handle these inconsistent documents - for example, when asked
to render a namespace URI as text, it unconditionally assumes it doesn't
have to escape it, because it may not contain special characters anway.
Which, if course, isn't necessarily true for structurally valid but
semantically invalid documents...
valid...

Fixing that wasn't possible without a major rewrite of the XML support's
error handling - one must use the modern version of libxml's error handling
infrastructure to actually be able to detect these semantic error reliably
and distinguish them from mere warnings.

Noah Misch has reviewed that patch pretty extensively (Thanks again, Noah!),
and I've resolved all his concerns regarding the implementation. I haven't
seen a single argument *against* applying this so far.

best regards,
Florian Pflug

On Jul18, 2011, at 22:19 , Tom Lane wrote:
>> and an
>> error-reporting patch that Tom weighed in on over the weekend. This
>> last suffers from the issue that it's not quite clear whether Tom is
>> going to do the work or whether he's expecting the submitter to do it.
>
> If you mean the business about allowing GUCs in postgresql.conf to be
> applied even if there are semantic errors elsewhere, I'm just as happy
> to let Alexey or Florian have a go at it first, if they want. The real
> question at the moment is do we have consensus about changing that?
> Because if we do, the submitted patch is certainly not something to
> commit as-is, and should be marked Returned With Feedback.

Just to avoid false expectations here: I'd be happy to review further
versions of this patch, but I won't write them.

best regards,
Florian Pflug

On Mon, 2011-07-18 at 15:59 -0400, Robert Haas wrote:
> On a pgbench run with 8
> clients on a 32-core machine, I see about a 2% speedup from that patch
> on pgbench -S, and it grows to 8% at 32 clients. At 80 clients (but
> still just a 32-core box), the results bounce around more, but taking
> the median of three five-minute runs, it's an 11% improvement. To me,
> that's enough to make it worth applying, especially considering that
> what is 11% on today's master is, in raw TPS, equivalent to maybe 30%
> of yesterday's master (prior to the fast relation lock patch being
> applied). More, it seems pretty clear that this is the conceptually
> right thing to do, even if it's going to require some work elsewhere
> to file down all the rough edges thus exposed. If someone objects to
> that, then OK, we should talk about that: but so far I don't think
> anyone has expressed strong opposition: in which case I'd like to fix
> it up and get it in.

Agreed. I certainly like the concept of the lazy vxid patch.

Regards,
Jeff Davis

On 2011-07-18 21:59, Robert Haas wrote:
> There are only two patches left and I think we really ought to try to
> take a crack at doing something with them. Yeb is working on the
> userspace access vector cache patch, which I think is going drag on
> longer than we want keep the CommitFest open, but I'm OK with giving
> it a day or two to shake out.
At the end if this day I'm nearing the 'my head a splode' moment, which
is more caused by trying to get my brain around selinux and it's
postgresql policy, than the actual patch to review. I've verified that
the patch works as indicated by KaiGai-san, but reading and
understanding the code to say anything useful about it will take a few
more hours, which will be tomorrow.

regards,
Yeb

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jul 18, 2011 at 4:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If you mean the business about allowing GUCs in postgresql.conf to be
>> applied even if there are semantic errors elsewhere, I'm just as happy
>> to let Alexey or Florian have a go at it first, if they want. The real
>> question at the moment is do we have consensus about changing that?
>> Because if we do, the submitted patch is certainly not something to
>> commit as-is, and should be marked Returned With Feedback.

> I'm not totally convinced. The proposed patch is pretty small, and
> seems to stand on its own two feet. I don't hear anyone objecting to
> your proposed plan, but OTOH it doesn't strike me as such a good plan
> that we should reject all other improvements in the meantime. Maybe
> I'm missing something...

To me, the proposed patch adds another layer of contortionism on top of
code that's already logically messy. I find it pretty ugly, and would
prefer to try to simplify the code before not after we attempt to deal
with the feature the patch wants to add.

regards, tom lane

Excerpts from Tom Lane's message of mar jul 19 12:09:24 -0400 2011:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, Jul 18, 2011 at 4:19 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> If you mean the business about allowing GUCs in postgresql.conf to be
> >> applied even if there are semantic errors elsewhere, I'm just as happy
> >> to let Alexey or Florian have a go at it first, if they want. The real
> >> question at the moment is do we have consensus about changing that?
> >> Because if we do, the submitted patch is certainly not something to
> >> commit as-is, and should be marked Returned With Feedback.
>
> > I'm not totally convinced. The proposed patch is pretty small, and
> > seems to stand on its own two feet. I don't hear anyone objecting to
> > your proposed plan, but OTOH it doesn't strike me as such a good plan
> > that we should reject all other improvements in the meantime. Maybe
> > I'm missing something...
>
> To me, the proposed patch adds another layer of contortionism on top of
> code that's already logically messy. I find it pretty ugly, and would
> prefer to try to simplify the code before not after we attempt to deal
> with the feature the patch wants to add.

+1. Alexey stated that he would get back on this patch for reworks.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

[ having now read the relevant threads a bit more carefully ... ]

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On Jul18, 2011, at 22:19 , Tom Lane wrote:
>> Yeah, it's not clear to me either which of those are still reasonable
>> candidates for committing as-is.

> There are actually three XML-related patches from me in the queue.
> I'll try to give an overview of their states - as perceived by me,
> of course. If people want to comment on this, I suggest to do that
> that either on the existing threads from these patches or on new ones,
> but not on this one - lest confusion ensues.

> * Bugfix for XPATH() if text or attribute nodes are selected
> https://commitfest.postgresql.org/action/patch_view?id=580

> Makes XPATH() return valid XML if text or attribute are selected.

> I'm not aware of any issues with that one, other than Radoslaw's
> unhappiness about the change of behaviour. Since the current behaviour
> is very clearly broken (as in dump, reload, ka-Woom), I'm not prepared
> to accept that as a show-stopper.

I think we ought to apply this one. I agree that returning invalid XML
is not acceptable.

> * Bugfix for XPATH() if expression returns a scalar value
> https://commitfest.postgresql.org/action/patch_view?id=565

> Makes XPATH() support XPath expressions which compute a scalar value,
> not a node set (i.e. apply a top-level function such as name()).
> Currently, we return an empty array in that case.

> Peter Eisentraut initially seemed to prefer creating separate functions
> for that (XPATH_STRING, ...). I argued against that, on the grounds that
> that'd make it impossible to evaluate user-supplied XPath expression (since
> you wouldn't know which function to use). I haven't heard back from him
> after that argument.

I find your argument against XPATH_STRING() a bit unconvincing.
The use case for that is not where you are trying to evaluate an
unknown, arbitrary XPath expression; it's where you are evaluating an
expression that you *know* returns string (ie, it has name() or some
other string returning function at top level) and you'd like a string,
thanks, not something that you have to de-escape in order to get a
string.

Of course this use-case could also be addressed by providing a de-escape
function, but I don't really think that de_escape(xpath(...)[1]) is a
particularly friendly or efficient notation.

Now, these statements are not arguments against the patch --- as is,
XPATH() is entirely useless for expressions that return scalars, and
with the patch it at least does something usable. So I think we should
apply it. But there is room here for more function(s) to provide more
convenient functionality for the special case of expressions returning
strings. I'd be inclined to provide xpath_number and xpath_bool too,
although those are less essential since a cast will get the job done.

There's some code-style aspects I don't care for in the submitted patch,
but I'll go clean those up and commit it.

regards, tom lane

On Jul20, 2011, at 23:35 , Tom Lane wrote:
> I find your argument against XPATH_STRING() a bit unconvincing.
> The use case for that is not where you are trying to evaluate an
> unknown, arbitrary XPath expression; it's where you are evaluating an
> expression that you *know* returns string (ie, it has name() or some
> other string returning function at top level) and you'd like a string,
> thanks, not something that you have to de-escape in order to get a
> string.

Hm, maybe I didn't make myself clear. I'm not against having
special-case functions like XPATH_STRING() at all, as long as there's
a general-purpuse function like XPATH() that deal with every expression
you throw at it.

There's a small additional concern, though, which is that there's an
XPath 2.0 spec out there, and it modifies the type system and data model
rather heavily. So before we go adding functions, it'd probably be wise
to check that we're not painting ourselves into a corner.

> Of course this use-case could also be addressed by providing a de-escape
> function, but I don't really think that de_escape(xpath(...)[1]) is a
> particularly friendly or efficient notation.

Yeah, that's pretty ugly. Having XMLESCAPE/XMLUNESCAPE (with types
TEXT -> XML and XML -> TEXT) would probably still be a good idea though,
even if it no replacement for XPATH_STRING().

> Now, these statements are not arguments against the patch --- as is,
> XPATH() is entirely useless for expressions that return scalars, and
> with the patch it at least does something usable. So I think we should
> apply it.

Cool, so we're on the same page.

> But there is room here for more function(s) to provide more
> convenient functionality for the special case of expressions returning
> strings. I'd be inclined to provide xpath_number and xpath_bool too,
> although those are less essential since a cast will get the job done.

No objection here. I do have a number of XML-related stuff that I'd like
to do for 9.2, actually. I'll write up a proposal once this commit-fest
is over, and I'll include XPATH_STRINGS and friends.

> There's some code-style aspects I don't care for in the submitted patch,
> but I'll go clean those up and commit it.

I'd offer to fix them, but I somehow have the feeling that you're already
in the middle of it ;-)

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> There's a small additional concern, though, which is that there's an
> XPath 2.0 spec out there, and it modifies the type system and data model
> rather heavily. So before we go adding functions, it'd probably be wise
> to check that we're not painting ourselves into a corner.

Good point. Has anybody here looked at that?

regards, tom lane

On Thu, 2011-07-21 at 00:21 +0200, Florian Pflug wrote:
> There's a small additional concern, though, which is that there's an
> XPath 2.0 spec out there, and it modifies the type system and data model
> rather heavily. So before we go adding functions, it'd probably be wise
> to check that we're not painting ourselves into a corner.

Why not just write XPATH2() function conforming to XPath 2.0 spec if
the new spec is substancially different ?

--
-------
Hannu Krosing
PostgreSQL Infinite Scalability and Performance Consultant
PG Admin Book: http://www.2ndQuadrant.com/books/