Last gasp

On Thu, Apr 5, 2012 at 1:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

These patches aren't marked with a committer

FK arrays
ECPG fetch
foreign stats
command triggers
check function
parallel pg_dump

Does that mean myself or others should be claiming them for commit/reject?

I've been right in the middle of the command triggers stuff, so I
suppose I would pick that up for commit if it were ready, but there
hasn't been a new version this week unless I've missed it, and even if
the new version arrived right this minute, I don't think I or anyone
else can do a good job committing a patch of that size in the time
remaining. So I think it's time to formally put that one out of its
misery.

I think the ECPG fetch patch is about ready to go. Normally Michael
Meskes handles all ECPG patches, but I'm not sure what his schedule is
like. I'm not sure what the politics are of someone else touching
that code.

Heikki recently produced a revision of the check function patch, but
I'm not sure whether he's planning to commit that or whether it's a
demonstration of a broader rework he wants Pavel (or someone else) to
do. At any rate, I think it's up to him whether or not to commit
that.

I think Andrew is working on parallel pg_dump, so I suspect the rest
of us should stay out of the way. I've also looked at it extensively
and will work on it if Andrew isn't, but I don't think it's ready to
commit. A few preliminary pieces could go in, perhaps.

I am not sure what the state of the foreign stats patch is, or FK arrays.

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

On 05.04.2012 21:00, Robert Haas wrote:

Heikki recently produced a revision of the check function patch, but
I'm not sure whether he's planning to commit that or whether it's a
demonstration of a broader rework he wants Pavel (or someone else) to
do. At any rate, I think it's up to him whether or not to commit
that.

Yeah, I guess it's up to me, now that I've touched that code. I don't
feel ready to commit it yet. It needs some more cleanup, which I could
do, but frankly even with the refactoring I did I'm still not totally
happy with the way it works. I feel that there ought to be a less
duplicative approach, but I don't have any more concrete proposals.
Unless someone more motivated picks up the patch now, I think it needs
to be pushed to 9.3.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, Apr 5, 2012 at 7:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 5, 2012 at 1:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

These patches aren't marked with a committer

FK arrays
ECPG fetch
foreign stats
command triggers
check function
parallel pg_dump

Does that mean myself or others should be claiming them for commit/reject?

I've been right in the middle of the command triggers stuff, so I
suppose I would pick that up for commit if it were ready, but there
hasn't been a new version this week unless I've missed it, and even if
the new version arrived right this minute, I don't think I or anyone
else can do a good job committing a patch of that size in the time
remaining.  So I think it's time to formally put that one out of its
misery.

I think doing so will cause substantial misery for many users. I find
it hard to believe that such a simple concept hasn't managed to
produce some workable subset after months of work.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Thu, Apr 5, 2012 at 1:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

These patches aren't marked with a committer

I think the ECPG fetch patch is about ready to go. Normally Michael
Meskes handles all ECPG patches, but I'm not sure what his schedule is
like. I'm not sure what the politics are of someone else touching
that code.

I think we should leave that one for Michael. Frankly, none of the
rest of us pay enough attention to ecpg to be candidates to take
responsibility for nontrivial patches there.

In fact, for *most* of these patches the fact that they're still here
should give you pause about just committing them.

I am not sure what the state of the foreign stats patch is, or FK arrays.

I'm taking the foreign stats one; as I mentioned a bit ago, I think we
need to push an FDW ANALYZE hook into 9.2. I don't like the details of
what's in the current submission but I think it's fixable with not much
effort.

The FK arrays one I'm kind of queasy about. It's a cool-sounding idea
but I'm not convinced that all the corner-case details have been
adequately thought through, and I'm scared of being unable to fix any
such bugs in later versions because of backwards compatibility worries.
It'd be a lot better to be pushing this in at the start of a devel cycle
than the end.

Most of the rest of this stuff I'm about ready to put off to 9.3.
The risk/reward ratio for committing it on the last day of the last
9.2 fest doesn't look good.

regards, tom lane

On Thu, Apr 5, 2012 at 2:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Thu, Apr 5, 2012 at 7:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 5, 2012 at 1:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

These patches aren't marked with a committer

FK arrays
ECPG fetch
foreign stats
command triggers
check function
parallel pg_dump

Does that mean myself or others should be claiming them for commit/reject?

I've been right in the middle of the command triggers stuff, so I
suppose I would pick that up for commit if it were ready, but there
hasn't been a new version this week unless I've missed it, and even if
the new version arrived right this minute, I don't think I or anyone
else can do a good job committing a patch of that size in the time
remaining.  So I think it's time to formally put that one out of its
misery.

I think doing so will cause substantial misery for many users. I find
it hard to believe that such a simple concept hasn't managed to
produce some workable subset after months of work.

I am not interested in relitigating on this thread what has already
been extensively discussed nearby. Dimitri and I agreed on numerous
changes to try to make the behavior sane, and those changes were
suggested and agreed to for good reason. We didn't agree on every
point, of course, but we did agree on most of it, and there is no
patch that implements what was agreed. Even if there were, there is
not time to review and commit a heavily revised version of a >1000
line patch before tomorrow, and any suggestion to the contrary is just
plain wrong.

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

2012/4/5 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>:

On 05.04.2012 21:00, Robert Haas wrote:

Heikki recently produced a revision of the check function patch, but
I'm not sure whether he's planning to commit that or whether it's a
demonstration of a broader rework he wants Pavel (or someone else) to
do.  At any rate, I think it's up to him whether or not to commit
that.

Yeah, I guess it's up to me, now that I've touched that code. I don't feel
ready to commit it yet. It needs some more cleanup, which I could do, but
frankly even with the refactoring I did I'm still not totally happy with the
way it works. I feel that there ought to be a less duplicative approach, but
I don't have any more concrete proposals. Unless someone more motivated
picks up the patch now, I think it needs to be pushed to 9.3.

I played with more general plpgpsm walker, but I am sure, so it is
wrong way - request for checking and dumping, and releasing plans are
too different. So there are not too much possibilities :(

The main issue is in design of exec nodes. I have no idea how to move
checking to there without increasing complexity in pl_exec.c. Some are
relative simply, but "case", "if", "block" are not.

Other idea is moving plpgsql_check_function to contrib.

Regards

Pavel

--
 Heikki Linnakangas
 EnterpriseDB   http://www.enterprisedb.com

t

On Thu, Apr 5, 2012 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

In fact, for *most* of these patches the fact that they're still here
should give you pause about just committing them.

Um, yeah.

I am not sure what the state of the foreign stats patch is, or FK arrays.

I'm taking the foreign stats one; as I mentioned a bit ago, I think we
need to push an FDW ANALYZE hook into 9.2.  I don't like the details of
what's in the current submission but I think it's fixable with not much
effort.

Cool.

The FK arrays one I'm kind of queasy about.  It's a cool-sounding idea
but I'm not convinced that all the corner-case details have been
adequately thought through, and I'm scared of being unable to fix any
such bugs in later versions because of backwards compatibility worries.
It'd be a lot better to be pushing this in at the start of a devel cycle
than the end.

I've been feeling that that patch has been suffering from a lack of
reviewer attention, which is a real shame, because I think the
functionality is indeed really cool. But I haven't looked at it
enough to know what kind of shape it's in.

Most of the rest of this stuff I'm about ready to put off to 9.3.
The risk/reward ratio for committing it on the last day of the last
9.2 fest doesn't look good.

+1.

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

Excerpts from Simon Riggs's message of jue abr 05 14:28:54 -0300 2012:

These patches aren't marked with a committer

FK arrays
ECPG fetch
foreign stats
command triggers
check function
parallel pg_dump

Does that mean myself or others should be claiming them for commit/reject?

The FK locking patch isn't on this list; however, I'm hijacking this
thread to say that some benchmarking runs we tried weren't all that
great, showing 9% performance degradation on stock pgbench -- i.e. a
large hit that will harm everybody even if they are not using FKs at
all. I'm thus setting the patch returned with feedback, which is sure
to make several hackers happy and tons of users unhappy.

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

On Thu, Apr 5, 2012 at 7:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The FK arrays one I'm kind of queasy about.  It's a cool-sounding idea
but I'm not convinced that all the corner-case details have been
adequately thought through, and I'm scared of being unable to fix any
such bugs in later versions because of backwards compatibility worries.
It'd be a lot better to be pushing this in at the start of a devel cycle
than the end.

OK, that's clear. I would have taken it, but not now.

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

On Thu, Apr 5, 2012 at 2:37 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

The FK locking patch isn't on this list; however, I'm hijacking this
thread to say that some benchmarking runs we tried weren't all that
great, showing 9% performance degradation on stock pgbench -- i.e.  a
large hit that will harm everybody even if they are not using FKs at
all.  I'm thus setting the patch returned with feedback, which is sure
to make several hackers happy and tons of users unhappy.

Ouch! That's a real bummer. It makes me glad that you tested it, but
I can't say I'm happy about the outcome. Did you get in any insight
into where the regression is coming from?

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

On Thu, Apr 5, 2012 at 7:37 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

The FK locking patch isn't on this list;

We'd agreed you were the assigned committer, hence not on list.

however, I'm hijacking this
thread to say that some benchmarking runs we tried weren't all that
great, showing 9% performance degradation on stock pgbench -- i.e.  a
large hit that will harm everybody even if they are not using FKs at
all.  I'm thus setting the patch returned with feedback, which is sure
to make several hackers happy and tons of users unhappy.

I've done what I can to alter that, but I think its the right decision
at this point. I would say its been the largest and most subtle patch
submitted, so please don't be down by that.

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

On Thu, 2012-04-05 at 14:27 -0400, Robert Haas wrote:

On Thu, Apr 5, 2012 at 2:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On Thu, Apr 5, 2012 at 7:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Apr 5, 2012 at 1:28 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

These patches aren't marked with a committer

FK arrays
ECPG fetch
foreign stats
command triggers
check function
parallel pg_dump

Does that mean myself or others should be claiming them for commit/reject?

I've been right in the middle of the command triggers stuff, so I
suppose I would pick that up for commit if it were ready, but there
hasn't been a new version this week unless I've missed it, and even if
the new version arrived right this minute, I don't think I or anyone
else can do a good job committing a patch of that size in the time
remaining. So I think it's time to formally put that one out of its
misery.

I think doing so will cause substantial misery for many users. I find
it hard to believe that such a simple concept hasn't managed to
produce some workable subset after months of work.

I am not interested in relitigating on this thread what has already
been extensively discussed nearby. Dimitri and I agreed on numerous
changes to try to make the behavior sane,

To me it looked like the scope of the patch started to suddenly expand
exponentially a few days ago from a simple COMMAND TRIGGERS, which would
have finally enabled trigger-based or "logical" replication systems to
do full replication to something recursive which would attempt to cover
all weird combinations of commands triggering other commands for which
there is no real use-case in view, except a suggestion "don't do it" :)

The latest patch (v18) seemed quite ok for its original intended
purpose.

On Thu, Apr 05, 2012 at 02:23:03PM -0400, Tom Lane wrote:

I think the ECPG fetch patch is about ready to go. Normally Michael
Meskes handles all ECPG patches, but I'm not sure what his schedule is
like. I'm not sure what the politics are of someone else touching
that code.

I think we should leave that one for Michael. Frankly, none of the
rest of us pay enough attention to ecpg to be candidates to take
responsibility for nontrivial patches there.

I will take care of this over the next couple days. Is the patch that Zoltan
send last Friday the latest version?

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

On Thu, 2012-04-05 at 20:46 +0200, Hannu Krosing wrote:

On Thu, 2012-04-05 at 14:27 -0400, Robert Haas wrote:

On Thu, Apr 5, 2012 at 2:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

...

I think doing so will cause substantial misery for many users. I find
it hard to believe that such a simple concept hasn't managed to
produce some workable subset after months of work.

I am not interested in relitigating on this thread what has already
been extensively discussed nearby. Dimitri and I agreed on numerous
changes to try to make the behavior sane,

To me it looked like the scope of the patch started to suddenly expand
exponentially a few days ago from a simple COMMAND TRIGGERS, which would
have finally enabled trigger-based or "logical" replication systems to
do full replication to something recursive which would attempt to cover
all weird combinations of commands triggering other commands for which
there is no real use-case in view, except a suggestion "don't do it" :)

The latest patch (v18) seemed quite ok for its original intended
purpose.

Sorry, i hit "send!" too early.

Would it be possible to put some "command trigger hooks" in a few
strategic places so that some trigger-like functionality could be loaded
at run time, mainly with a view of writing DDL replication
'non-triggers' , mostly based on current v18 code, but of course without
all the nice CREATE TRIGGER syntax ?

perhaps created with a pg_create_command_trigger(...)

that is something in the line of how Full Text Indexing was done for a
long time.

On 04/05/2012 02:00 PM, Robert Haas wrote:

I think Andrew is working on parallel pg_dump, so I suspect the rest
of us should stay out of the way. I've also looked at it extensively
and will work on it if Andrew isn't, but I don't think it's ready to
commit. A few preliminary pieces could go in, perhaps.

I am working on it when I get time, but I am slammed. It would probably
take me a couple of full days to do a thorough code review, and finding
that is hard.

cheers

andrew

On Thu, Apr 5, 2012 at 2:46 PM, Hannu Krosing <hannu@krosing.net> wrote:

To me it looked like the scope of the patch started to suddenly expand
exponentially a few days ago from a simple COMMAND TRIGGERS, which would
have finally enabled trigger-based or "logical" replication systems to
do full replication to something recursive which would attempt to cover
all weird combinations of commands triggering other commands for which
there is no real use-case in view, except a suggestion "don't do it" :)

The latest patch (v18) seemed quite ok for its original intended
purpose.

OK, so here we go, rehashing the discussion we already had on thread A
on thread B. The particular issue you are mentioning there was not
the reason that patch isn't going to end up in 9.2. If the only thing
the patch had needed was a little renaming and syntax cleanup, I would
have done it myself (or Dimitri would have) and I would have committed
it. That is not the problem, or at least it's not the only problem.
There are at least two other major issues:

- The patch as posted fires triggers at unpredictable times depending
on which command you're executing. Some things that are really
sub-commands fire triggers anyway as if they were toplevel commands;
others don't; whether or not it happens in a particular case is
determined by implementation details rather than by any consistent
principle of operation. In the cases where triggers do fire, they
don't always fire at the same place in the execution sequence.

- The patch isn't safe if the triggers try to execute DDL on the
object being modified. It's not exactly clear what misbehavior will
result in every case, but it is clear that that it hasn't really been
thought about.

Now, if anyone who was actually following the conversation thought
these things were not problems, they could have written back to the
relevant thread and said, hey, I don't mind if the trigger firing
behavior changes every time someone does any refactoring of our
badly-written DDL code and if the server blows up in random ways when
someone does something unexpected in the trigger that's OK with me
too. Maybe not surprisingly, no one said that. Two people wrote into
that thread after my latest round of reviewing and both of them
disagreed with only minor points of my review, and we had a technical
discussion about those issues. But showing up after the fact and
acting as if there were no serious issues found during that review is
either disingenuous or a sign that you didn't really read the thread.

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

On Thu, Apr 5, 2012 at 2:58 PM, Hannu Krosing <hannu@krosing.net> wrote:

On Thu, 2012-04-05 at 20:46 +0200, Hannu Krosing wrote:

On Thu, 2012-04-05 at 14:27 -0400, Robert Haas wrote:

On Thu, Apr 5, 2012 at 2:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

...

I think doing so will cause substantial misery for many users. I find
it hard to believe that such a simple concept hasn't managed to
produce some workable subset after months of work.

I am not interested in relitigating on this thread what has already
been extensively discussed nearby.  Dimitri and I agreed on numerous
changes to try to make the behavior sane,

To me it looked like the scope of the patch started to suddenly expand
exponentially a few days ago from a simple COMMAND TRIGGERS, which would
have finally enabled trigger-based or "logical" replication systems to
do full replication to something recursive which would attempt to cover
all weird combinations of commands triggering other commands for which
there is no real use-case in view, except a suggestion "don't do it" :)

The latest patch (v18) seemed quite ok for its original intended
purpose.

Sorry, i hit "send!" too early.

Would it be possible to put some "command trigger hooks" in a few
strategic places so that some trigger-like functionality could be loaded
at run time, mainly with a view of writing DDL replication
'non-triggers' , mostly based on current v18 code, but of course without
all the nice CREATE TRIGGER syntax ?

I certainly think that would be a possible way forward, but I don't
think we should try to engineer that in the next 24 hours. Had the
original goals of the patch been somewhat more modest, I think we
could have gotten it into 9.2, but there's no time to rethink the
scope of the patch now. With all respect for Dimitri and his *very*
hard work on this subject, submitting a brand new major feature to the
last CommitFest is not really a great way to get it committed,
especially given that we didn't have consensus on the design before he
started coding. There is every reason to think that we can get this
feature into 9.3 with some more work, but it's not ready yet, and
wishing won't make it so.

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

Excerpts from Robert Haas's message of jue abr 05 15:40:17 -0300 2012:

On Thu, Apr 5, 2012 at 2:37 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

The FK locking patch isn't on this list; however, I'm hijacking this
thread to say that some benchmarking runs we tried weren't all that
great, showing 9% performance degradation on stock pgbench -- i.e.  a
large hit that will harm everybody even if they are not using FKs at
all.  I'm thus setting the patch returned with feedback, which is sure
to make several hackers happy and tons of users unhappy.

Ouch! That's a real bummer. It makes me glad that you tested it, but
I can't say I'm happy about the outcome. Did you get in any insight
into where the regression is coming from?

Not really -- after reaching that conclusion I dropped immediate work on
the patch to do other stuff (like checking whether there's any other
patch I can help with in commitfest). I will resume work later, for a
(hopefully early) 9.3 submission.

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

On Thu, 2012-04-05 at 15:02 -0400, Robert Haas wrote:

On Thu, Apr 5, 2012 at 2:46 PM, Hannu Krosing <hannu@krosing.net> wrote:

To me it looked like the scope of the patch started to suddenly expand
exponentially a few days ago from a simple COMMAND TRIGGERS, which would
have finally enabled trigger-based or "logical" replication systems to
do full replication to something recursive which would attempt to cover
all weird combinations of commands triggering other commands for which
there is no real use-case in view, except a suggestion "don't do it" :)

The latest patch (v18) seemed quite ok for its original intended
purpose.

OK, so here we go, rehashing the discussion we already had on thread A
on thread B. The particular issue you are mentioning there was not
the reason that patch isn't going to end up in 9.2. If the only thing
the patch had needed was a little renaming and syntax cleanup, I would
have done it myself (or Dimitri would have) and I would have committed
it. That is not the problem, or at least it's not the only problem.
There are at least two other major issues:

- The patch as posted fires triggers at unpredictable times depending
on which command you're executing. Some things that are really
sub-commands fire triggers anyway as if they were toplevel commands;
others don't; whether or not it happens in a particular case is
determined by implementation details rather than by any consistent
principle of operation. In the cases where triggers do fire, they
don't always fire at the same place in the execution sequence.

For me it would be enough to know if the trigger is fired by top-level
command or not.

In worst case I could probably detect it myself, just give me something
to hang the detection code to .

- The patch isn't safe if the triggers try to execute DDL on the
object being modified. It's not exactly clear what misbehavior will
result in every case, but it is clear that that it hasn't really been
thought about.

It never seemed important for me, as the only thing I was ever expecting
to do in a command trigger was inserting rows in one unrelated table.

To me DDL-triggered-by-DDL seemed like a very bad idea anyway.

But as you seemed to be envisioning some use-cases for that I did not
object to you working it out.

Now, if anyone who was actually following the conversation thought
these things were not problems, they could have written back to the
relevant thread and said, hey, I don't mind if the trigger firing
behavior changes every time someone does any refactoring of our
badly-written DDL code and if the server blows up in random ways when
someone does something unexpected in the trigger that's OK with me
too.

I don't mind if the trigger firing behavior changes every time someone
does any refactoring of our badly-written DDL code

Here :)

Maybe not surprisingly, no one said that. Two people wrote into
that thread after my latest round of reviewing and both of them
disagreed with only minor points of my review, and we had a technical
discussion about those issues. But showing up after the fact and
acting as if there were no serious issues found during that review is
either disingenuous or a sign that you didn't really read the thread.

As there are other ways to blow up the backend, i did not object to you
either working out a better solution or leaving it as it is.

I am speaking up now as this is the first time I am told I have to wait
another year for this feature to arrive.