PG10 transition tables, wCTEs and multiple operations on the same table
Since the subject of transition tables came up, I thought I'd test how this
case works:
=# create table qwr(a int);
CREATE TABLE
=# create function qq() returns trigger as $$ begin raise notice '%',
(select count(*) from oo); return null; end $$ language plpgsql;
CREATE FUNCTION
=# create trigger xx after insert on qwr referencing new table as oo for
each statement execute procedure qq();
CREATE TRIGGER
=# with t as (insert into qwr values (1)) insert into qwr values (2), (3);
NOTICE: 3
NOTICE: 3
INSERT 0 2
to me, this means that it doesn't work. Surely one of the trigger
invocations should say 1, and the other 2. Or was this intentional?
.m
On Fri, Jun 2, 2017 at 10:48 PM, Marko Tiikkaja <marko@joh.to> wrote:
Since the subject of transition tables came up, I thought I'd test how this
case works:=# create table qwr(a int);
CREATE TABLE=# create function qq() returns trigger as $$ begin raise notice '%',
(select count(*) from oo); return null; end $$ language plpgsql;
CREATE FUNCTION=# create trigger xx after insert on qwr referencing new table as oo for
each statement execute procedure qq();
CREATE TRIGGER=# with t as (insert into qwr values (1)) insert into qwr values (2), (3);
NOTICE: 3
NOTICE: 3
INSERT 0 2to me, this means that it doesn't work. Surely one of the trigger
invocations should say 1, and the other 2. Or was this intentional?
I would have expected that to be handled by separate transition tables
at different query levels, but evidently it isn't. The following
crashes:
create table table1(a int);
create table table2(a text);
create function trigfunc() returns trigger as $$ begin raise notice
'got: %', (select oo from oo); return null; end $$ language plpgsql;
create trigger trig1 after insert on table1 referencing new table as
oo for each statement execute procedure trigfunc();
create trigger trig2 after insert on table2 referencing new table as
oo for each statement execute procedure trigfunc();
with t as (insert into table1 values (1)) insert into table2 values ('hello');
I've run out of day today but will investigate.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 3, 2017 at 12:10 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
I would have expected that to be handled by separate transition tables
at different query levels, but evidently it isn't.
I am wondering if we need to wrap the execution of a modifying CTE in
AfterTriggerBeginQuery() and AfterTriggerEndQuery(). But I'm not sure
where, and it may be a couple of days before I can dig some more.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jun 3, 2017 at 1:20 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Sat, Jun 3, 2017 at 12:10 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:I would have expected that to be handled by separate transition tables
at different query levels, but evidently it isn't.I am wondering if we need to wrap the execution of a modifying CTE in
AfterTriggerBeginQuery() and AfterTriggerEndQuery(). But I'm not sure
where, and it may be a couple of days before I can dig some more.
So, afterTriggers.query_stack is used to handle the reentrancy that
results from triggers running further statements that might fire
triggers. It isn't used for dealing with extra ModifyTable nodes that
can appear in a plan because of wCTEs. Could it also be used for that
purpose? I think that would only work if it is the case that each
ModifyTable node begin and then runs to completion (ie no interleaving
of wCTE execution) and then its AS trigger fires, which I'm not sure
about. If that is the case, perhaps AfterTriggerBeginQuery and
AfterTriggerEndQuery could become the responsibility of
nodeModifyTable.c rather than execMain.c. I haven't tried this yet
and it may well be too ambitious at this stage.
Other ideas: (1) ban wCTEs that target relations with transition
tables in PG10, because we're out of time; (2) find an entirely new
way to keep track of the current active transition table(s) to replace
the current stack approach, such as including them in the
TransitionCaptureState object in the patch that I proposed to fix the
nearby inheritance bugs[1]/messages/by-id/CA+TgmoZzTBBAsEUh4MazAN7ga=8SsMC-Knp-6cetts9yNZUCcg@mail.gmail.com.
Stepping back and squinting a bit, both this and the inheritance bug
stem from a failure to handle multiple ModifyTable nodes in a plan,
but the solutions are approximately opposite: in the inheritance case,
the solution I propose is to teach them all to coordinate their tuple
capture so that it's in a common format, and in the wCTE case the
solution must surely be to ensure that their state is kept separate.
The question I'd like to figure out is whether the existing
AfterTriggerBeginQuery/AfterTriggerEndQuery stack is the right data
structure for that task, considering the control flow when CTEs are
executed, which I need to learn more about.
Thoughts?
[1]: /messages/by-id/CA+TgmoZzTBBAsEUh4MazAN7ga=8SsMC-Knp-6cetts9yNZUCcg@mail.gmail.com
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 2, 2017 at 8:46 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
So, afterTriggers.query_stack is used to handle the reentrancy that
results from triggers running further statements that might fire
triggers. It isn't used for dealing with extra ModifyTable nodes that
can appear in a plan because of wCTEs. Could it also be used for that
purpose? I think that would only work if it is the case that each
ModifyTable node begin and then runs to completion (ie no interleaving
of wCTE execution) and then its AS trigger fires, which I'm not sure
about.
I don't think we want to commit to anything that depends on a CTE
creating an optimization fence, although *maybe* that would be OK in
the case of DML as a CTE. That's a pretty special case; I'm not
sure whether the standard discusses it.
If that is the case, perhaps AfterTriggerBeginQuery and
AfterTriggerEndQuery could become the responsibility of
nodeModifyTable.c rather than execMain.c. I haven't tried this yet
and it may well be too ambitious at this stage.
In a world where one statement can contain multiple DML statements
within CTEs, that may make sense regardless of transition table
issues; but I agree we're past the time to be considering making
such a change for v10.
Other ideas: (1) ban wCTEs that target relations with transition
tables in PG10, because we're out of time;
Given that we haven't even discussed what to do about an UPDATE
statement with a CTE that updates the same table when there are
BEFORE EACH ROW UPDATE triggers on that table (which perhaps return
NULL for some but not all rows?), I'm not sure we've yet plumbed the
depths of this morass.
For example, for this:
drop table if exists t1 cascade;
create table t1 (c1 int not null, c2 text not null default '');
insert into t1 (c1) values (1), (2), (3), (4), (5);
drop function if exists t1_upd_func() cascade;
create function t1_upd_func()
returns trigger
language plpgsql
as $$
begin
if old.c1 between 2 and 4 then
new.c2 = old.c2 || ';' || old.c1::text || ',' || new.c1::text;
end if;
if old.c1 > 3 then
return null;
end if;
return new;
end;
$$;
create trigger t1_upd_update
before update
on t1
for each row execute procedure t1_upd_func();
with x as (update t1 set c1 = c1 - 1 returning c1)
update t1 set c1 = t1.c1 + 1 from x where x.c1 = t1.c1;
select * from t1;
... what is the correct result?
I get this:
c1 | c2
----+------
4 |
5 |
0 |
1 | ;2,1
2 | ;3,2
(5 rows)
It is definitely not what I expected, and seems wrong in multiple
ways from a logical perspective, looking at those as set-based
operations. (Tautologies about the procedural mechanism that
creates the result are not defenses of the result itself.)
Can we fix things exclusive of transition tables in this release?
If not, the only reasonable thing to do is to try to avoid further
complicating things with CTE/transition table usage until we sort
out the basics of triggers firing from CTE DML in the first place.
--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Jun 4, 2017 at 10:41 AM, Kevin Grittner <kgrittn@gmail.com> wrote:
On Fri, Jun 2, 2017 at 8:46 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:So, afterTriggers.query_stack is used to handle the reentrancy that
results from triggers running further statements that might fire
triggers. It isn't used for dealing with extra ModifyTable nodes that
can appear in a plan because of wCTEs. Could it also be used for that
purpose? I think that would only work if it is the case that each
ModifyTable node begin and then runs to completion (ie no interleaving
of wCTE execution) and then its AS trigger fires, which I'm not sure
about.I don't think we want to commit to anything that depends on a CTE
creating an optimization fence, although *maybe* that would be OK in
the case of DML as a CTE. That's a pretty special case; I'm not
sure whether the standard discusses it.
According to our manual the standard doesn't allow DML in CTEs. I
suppose it wouldn't make much sense without RETURNING, which is also
non-standard.
If that is the case, perhaps AfterTriggerBeginQuery and
AfterTriggerEndQuery could become the responsibility of
nodeModifyTable.c rather than execMain.c. I haven't tried this yet
and it may well be too ambitious at this stage.In a world where one statement can contain multiple DML statements
within CTEs, that may make sense regardless of transition table
issues; but I agree we're past the time to be considering making
such a change for v10.
Ok.
Other ideas: (1) ban wCTEs that target relations with transition
tables in PG10, because we're out of time;Given that we haven't even discussed what to do about an UPDATE
statement with a CTE that updates the same table when there are
BEFORE EACH ROW UPDATE triggers on that table (which perhaps return
NULL for some but not all rows?), I'm not sure we've yet plumbed the
depths of this morass.[...]
Can we fix things exclusive of transition tables in this release?
If not, the only reasonable thing to do is to try to avoid further
complicating things with CTE/transition table usage until we sort
out the basics of triggers firing from CTE DML in the first place.
At least it's well documented that the execution order is undefined,
but yeah triggers do make things a lot more complicated and I'm not
sure I understand to what extent that is unavoidable once you decide
to allow DML in CTEs.
In the meantime, it seems like you agree that rejecting wCTEs that
affect tables with triggers with transition tables is the best
response to this bug report? Do you think that parse analysis is the
right time to do the check? Here's a first attempt at that.
Later, for the next cycle, I think we should investigate storing the
transition tables in ModifyTableState rather than in query_stack, and
then passing them to the Exec*Triggers() functions as arguments
(possibly inside TransitionCaptureState, if you accept my proposal for
the inheritance bug).
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
reject-wcte-with-transition-tables.patchapplication/octet-stream; name=reject-wcte-with-transition-tables.patchDownload+60-0
On Sat, Jun 3, 2017 at 10:39 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
In the meantime, it seems like you agree that rejecting wCTEs that
affect tables with triggers with transition tables is the best
response to this bug report? Do you think that parse analysis is the
right time to do the check? Here's a first attempt at that.
I'm starting to like the approach of reverting the entire transition
tables patch. Failing to consider the possibility of a plan with
multiple ModifyTable nodes seems like a pretty fundamental design
mistake, and I'm not eager either to ship this with that broken or try
to fix it at this stage of the release cycle.
--
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
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Jun 3, 2017 at 10:39 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:In the meantime, it seems like you agree that rejecting wCTEs that
affect tables with triggers with transition tables is the best
response to this bug report? Do you think that parse analysis is the
right time to do the check? Here's a first attempt at that.
FWIW, parse analysis is surely NOT the time for such a check. Triggers
might get added to a table between analysis and execution. I think you
might have to do it during executor startup.
I'm starting to like the approach of reverting the entire transition
tables patch. Failing to consider the possibility of a plan with
multiple ModifyTable nodes seems like a pretty fundamental design
mistake, and I'm not eager either to ship this with that broken or try
to fix it at this stage of the release cycle.
Postponing the feature to v11 might be a viable solution. We don't
have any other major work that depends on it do we?
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
On Mon, Jun 5, 2017 at 10:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I'm starting to like the approach of reverting the entire transition
tables patch. Failing to consider the possibility of a plan with
multiple ModifyTable nodes seems like a pretty fundamental design
mistake, and I'm not eager either to ship this with that broken or try
to fix it at this stage of the release cycle.Postponing the feature to v11 might be a viable solution. We don't
have any other major work that depends on it do we?
I don't think anything that depends on it was committed to v10.
--
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
On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Sat, Jun 3, 2017 at 10:39 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:In the meantime, it seems like you agree that rejecting wCTEs that
affect tables with triggers with transition tables is the best
response to this bug report? Do you think that parse analysis is the
right time to do the check? Here's a first attempt at that.FWIW, parse analysis is surely NOT the time for such a check. Triggers
might get added to a table between analysis and execution. I think you
might have to do it during executor startup.
Hmm. My understanding: In analyzeCTE(), where the check is done in
that patch, we hold a lock on the relation because we have a
RangeTblEntry. That means that no triggers can be added concurrently
in the case where you go on to execute the plan. If you save the plan
for later execution, triggers created between then and lock
reacquisition (because creating triggers touches pg_class) cause
reanalysis. This is not fundamentally different from altering the
table. For example, with that patch:
postgres=# prepare ps as with t as (insert into table1 values (1))
insert into table2 values ('hello');
PREPARE
postgres=# create trigger trig1 after insert on table1 referencing new
table as oo for each row execute procedure trigfunc();
CREATE TRIGGER
postgres=# execute ps;
ERROR: WITH clause cannot modify a table that has triggers with
transition tables
What am I missing?
I'm starting to like the approach of reverting the entire transition
tables patch. Failing to consider the possibility of a plan with
multiple ModifyTable nodes seems like a pretty fundamental design
mistake, and I'm not eager either to ship this with that broken or try
to fix it at this stage of the release cycle.
Not handling wCTEs or inheritance definitely counts as a howler, but
we seem tantalisingly close and it would be a shame to push the whole
feature back IMHO. I really hope we can move on to talking about
incremental matviews in the next cycle, which is why I've jumped on
every problem report so far.
I'm still waiting to hear from Kevin whether he thinks what I proposed
for the inheritance bug has legs. If so, my vote from the peanut
gallery would be to keep transition tables in the tree but block wCTEs
with transition tables, and then add support in the next release
(which I'd be happy to work on). Support will certainly be needed
ASAP, because it'd suck if incremental matview maintenance didn't work
just because you use wCTEs, and the 'action at a distance' factor
isn't a great user experience (adding a new trigger can in general
break existing queries, but breaking them just because they use wCTEs
is arguably surprising). On the other hand, if he thinks that the
inheritance bug is not adequately fixed by my proposal (with minor
tweaks) and needs a completely different approach, then I'm out (but
will be happy to help work on this stuff for PG11).
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
FWIW, parse analysis is surely NOT the time for such a check. Triggers
might get added to a table between analysis and execution. I think you
might have to do it during executor startup.
Hmm. My understanding: In analyzeCTE(), where the check is done in
that patch, we hold a lock on the relation because we have a
RangeTblEntry. That means that no triggers can be added concurrently
in the case where you go on to execute the plan. If you save the plan
for later execution, triggers created between then and lock
reacquisition (because creating triggers touches pg_class) cause
reanalysis.
No, they don't necessarily. One case that will certainly not be
covered by such a test is if the wCTE is an UPDATE or DELETE on
an inheritance hierarchy: parse analysis has no idea whether the
target table has children at all, let alone whether any of the
children have triggers that use transition tables. You really want
the test to happen after the planner has expanded inheritance.
Another problem is that the result of parse analysis is frozen for much
longer than you're thinking about in the case of stored rules or views.
We currently disallow views that contain writable CTEs, but I do not
think there's such a prohibition for rules.
More generally, the problem I've got with this proposal is that it causes
the presence of triggers to be significant much earlier in query execution
than they used to be. I'm concerned that this may break optimizations
that are already in place somewhere, or that we might want to add 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
On Tue, Jun 6, 2017 at 10:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Tue, Jun 6, 2017 at 2:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
FWIW, parse analysis is surely NOT the time for such a check. Triggers
might get added to a table between analysis and execution. I think you
might have to do it during executor startup.Hmm. My understanding: In analyzeCTE(), where the check is done in
that patch, we hold a lock on the relation because we have a
RangeTblEntry. That means that no triggers can be added concurrently
in the case where you go on to execute the plan. If you save the plan
for later execution, triggers created between then and lock
reacquisition (because creating triggers touches pg_class) cause
reanalysis.No, they don't necessarily. One case that will certainly not be
covered by such a test is if the wCTE is an UPDATE or DELETE on
an inheritance hierarchy: parse analysis has no idea whether the
target table has children at all, let alone whether any of the
children have triggers that use transition tables. You really want
the test to happen after the planner has expanded inheritance.
If Kevin accepts my other patch or at least the approach, we won't
allow any child table triggers with transition tables, so we wouldn't
need to consider them here (triggers on the named result table see
tuples collected from children, but ROW triggers attached to children
can't have transition tables and STATEMENT triggers attached to
children don't fire). Though those decisions probably wouldn't be
future-proof, we'd only want this block in place until we had support
for wCTEs.
Another problem is that the result of parse analysis is frozen for much
longer than you're thinking about in the case of stored rules or views.
We currently disallow views that contain writable CTEs, but I do not
think there's such a prohibition for rules.
Ugh. Right. I can indeed circumvent the above check with a wCTE
hiding in a rule, so parse analysis is not the right time for the
check. The whole rewrite area is clearly something I need to go and
learn about. Thanks for the explanation.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 4 June 2017 at 06:41, Kevin Grittner <kgrittn@gmail.com> wrote:
On Fri, Jun 2, 2017 at 8:46 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:So, afterTriggers.query_stack is used to handle the reentrancy that
results from triggers running further statements that might fire
triggers. It isn't used for dealing with extra ModifyTable nodes that
can appear in a plan because of wCTEs. Could it also be used for that
purpose? I think that would only work if it is the case that each
ModifyTable node begin and then runs to completion (ie no interleaving
of wCTE execution) and then its AS trigger fires, which I'm not sure
about.I don't think we want to commit to anything that depends on a CTE
creating an optimization fence, although *maybe* that would be OK in
the case of DML as a CTE. That's a pretty special case; I'm not
sure whether the standard discusses it.
It's definitely fine to require a fence for wCTEs. They're an
extension to the standard, and it's pretty much necessary that we not
pull up / push down across them since we don't want to affect the
side-effects (row changes). If there are any cases where it's safe,
they'll take some careful thought.
It's only standard CTEs (SELECT-based) that I think matter for the
optimisation fence behaviour.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Tue, Jun 6, 2017 at 12:58 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 4 June 2017 at 06:41, Kevin Grittner <kgrittn@gmail.com> wrote:
On Fri, Jun 2, 2017 at 8:46 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:So, afterTriggers.query_stack is used to handle the reentrancy that
results from triggers running further statements that might fire
triggers. It isn't used for dealing with extra ModifyTable nodes that
can appear in a plan because of wCTEs. Could it also be used for that
purpose? I think that would only work if it is the case that each
ModifyTable node begin and then runs to completion (ie no interleaving
of wCTE execution) and then its AS trigger fires, which I'm not sure
about.I don't think we want to commit to anything that depends on a CTE
creating an optimization fence, although *maybe* that would be OK in
the case of DML as a CTE. That's a pretty special case; I'm not
sure whether the standard discusses it.It's definitely fine to require a fence for wCTEs. They're an
extension to the standard, and it's pretty much necessary that we not
pull up / push down across them since we don't want to affect the
side-effects (row changes). If there are any cases where it's safe,
they'll take some careful thought.It's only standard CTEs (SELECT-based) that I think matter for the
optimisation fence behaviour.
After sleeping on it, I don't think we need to make that decision here
though. I think it's better to just move the tuplestores into
ModifyTableState so that each embedded DML statement has its own, and
have ModifyTable pass them to the trigger code explicitly. I think
I'd like to do that via the TransitionCaptureState object that I
proposed elsewhere, but I'll hold off on doing anything until I hear
from interested committers on which way we're going here, time being
short.
Call me an anti-globalisation (of variables) protestor.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Nice as it would be to add a SQL standard feature and advance the
effort to get to incremental maintenance of materialized views, and
much as I really appreciate the efforts Thomas has put into trying
to solve these problems, I agree that it is best to revert the
feature. It took years to get an in-depth review, then I was asked
not to commit it because others were working on patches that would
conflict. That just doesn't leave enough time to address these
issues before release. Fundamentally, I'm not sure that there is a
level of interest sufficient to support the effort.
I'll give it a few days for objections before reverting.
--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 6, 2017 at 10:25 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
Nice as it would be to add a SQL standard feature and advance the
effort to get to incremental maintenance of materialized views, and
much as I really appreciate the efforts Thomas has put into trying
to solve these problems, I agree that it is best to revert the
feature. It took years to get an in-depth review, then I was asked
not to commit it because others were working on patches that would
conflict. That just doesn't leave enough time to address these
issues before release. Fundamentally, I'm not sure that there is a
level of interest sufficient to support the effort.I'll give it a few days for objections before reverting.
I can only say that the lack of this feature comes up on a weekly basis on
IRC, and a lot of people would be disappointed to see it reverted.
.m
On Tue, Jun 6, 2017 at 3:41 PM, Marko Tiikkaja <marko@joh.to> wrote:
On Tue, Jun 6, 2017 at 10:25 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
It took years to get an in-depth review, then I was asked
not to commit it because others were working on patches that would
conflict. That just doesn't leave enough time to address these
issues before release. Fundamentally, I'm not sure that there is a
level of interest sufficient to support the effort.
I can only say that the lack of this feature comes up on a weekly basis on
IRC, and a lot of people would be disappointed to see it reverted.
Well, at PGCon I talked with someone who worked on the
implementation in Oracle 20-some years ago. He said they had a team
of 20 people full time working on the feature for years to get it
working. Now, I think the PostgreSQL community is a little lighter
on its feet, but without more active participation from others than
there has been so far, I don't intend to take another run at it.
I'll be happy to participate at such point that it's not treated as
a second-class feature set. Barring that, anyone else who wants to
take the patch and run with it is welcome to do so.
--
Kevin Grittner
VMware vCenter Server
https://www.vmware.com/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 6, 2017 at 4:25 PM, Kevin Grittner <kgrittn@gmail.com> wrote:
Nice as it would be to add a SQL standard feature and advance the
effort to get to incremental maintenance of materialized views, and
much as I really appreciate the efforts Thomas has put into trying
to solve these problems, I agree that it is best to revert the
feature. It took years to get an in-depth review, then I was asked
not to commit it because others were working on patches that would
conflict. That just doesn't leave enough time to address these
issues before release. Fundamentally, I'm not sure that there is a
level of interest sufficient to support the effort.
I find it a little unfair that you are blaming the community for not
taking enough interest in this feature when other people are posting
bug fixes for the feature which you can't find time to review (or even
respond to with an i'm-too-busy email) for lengthy periods of time.
Previously, I took the time to review and commit fixes for several
reported fixes which Thomas developed, while you ignored all of the
relevant threads. Now, he's posted another patch which you said you
would review by last Friday or at the latest by Monday and which you
have not reviewed. He's also suggested that the fix for this issue
probably relies on that one, so getting that patch reviewed is
relevant to this thread also.
I think I'd like to walk back my earlier statements about reverting
this patch just a little bit. Although putting the tuplestore at the
wrong level does seem like a fairly significant design mistake, Thomas
more or less convinced me yesterday on a Skype call that relocating it
to the ModifyTable node might not be that much work. If it's a
150-line patch, it'd probably be less disruptive to install a fix than
to revert the whole thing (and maybe put it back in again, in some
future release). On the other hand, if you aren't willing to write,
review, or commit any further patches to fix bugs in this feature,
then it can really only stay in the tree if somebody else is willing
to assume completely responsibility for it going forward.
So, are you willing and able to put any effort into this, like say
reviewing the patch Thomas posted, and if so when and how much? If
you're just done and you aren't going to put any more work into
maintaining it (for whatever reasons), then I think you should say so
straight out. People shouldn't have to guess whether you're going to
maintain your patch or not.
--
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
On Mon, Jun 5, 2017 at 6:40 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
After sleeping on it, I don't think we need to make that decision here
though. I think it's better to just move the tuplestores into
ModifyTableState so that each embedded DML statement has its own, and
have ModifyTable pass them to the trigger code explicitly.
I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
case -- one for updated tuples, and the other for inserted tuples.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 6, 2017 at 3:47 PM, Peter Geoghegan <pg@bowt.ie> wrote:
I suppose you'll need two tuplestores for the ON CONFLICT DO UPDATE
case -- one for updated tuples, and the other for inserted tuples.
Also, ISTM that the code within ENRMetadataGetTupDesc() probably
requires more explanation, resource management wise.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
