Re: [HACKERS] delta relations in AFTER triggers
On Thu, May 4, 2017 at 4:23 PM, Thomas Munro wrote: > We can't possibly support transition tables on TRUNCATE (the whole > point of TRUNCATE is not to inspect all the rows so we can't collect > them), and we already reject ROW triggers on TRUNCATE, so we should > reject transition tables on STATEMENT triggers for TRUNCATE at > creation time too. See attached. Thoughts? Committed, with the addition of a regression test, the inclusion of which in future bug fixes of this sort I recommend. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, May 4, 2017 at 5:51 AM, Thomas Munro wrote: > Reproduced here. The stack looks like this: > > frame #3: 0x00010f06f8b0 > postgres`ExceptionalCondition(conditionName="!(readptr->eflags & > 0x0002)", errorType="FailedAssertion", fileName="tuplestore.c", > lineNumber=1237) + 128 at assert.c:54 > frame #4: 0x00010f0cbc85 > postgres`tuplestore_rescan(state=0x7ff219840200) + 85 at > tuplestore.c:1237 > frame #5: 0x00010eced9b1 > postgres`ExecReScanNamedTuplestoreScan(node=0x7ff21d007840) + 81 > at nodeNamedtuplestorescan.c:197 > frame #6: 0x00010eca46a6 > postgres`ExecReScan(node=0x7ff21d007840) + 822 at execAmi.c:216 > frame #7: 0x00010ece7eca > postgres`ExecNestLoop(node=0x7ff21d006310) + 538 at > nodeNestloop.c:148 > > I think the problem is that the tuplestore read pointer hasn't been > opened with the "rewindable" flag. It works for me with the attached. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sat, May 06, 2017 at 05:34:46AM +, Noah Misch wrote: > On Fri, May 05, 2017 at 08:23:33AM +1200, Thomas Munro wrote: > > On Fri, May 5, 2017 at 12:39 AM, Neha Sharma > > wrote: > > > While testing the feature we encountered one more crash,below is the > > > scenario to reproduce. > > > > > > create table t1 ( a int); > > > create table t2 ( a int); > > > insert into t1 values (11),(12),(13); > > > > > > create or replace function my_trig() returns trigger > > > language plpgsql as $my_trig$ > > > begin > > > insert into t2(select a from new_table); > > > RETURN NEW; > > > end; > > > $my_trig$; > > > > > > create trigger my_trigger > > > after truncate or update on t1 > > > referencing new table as new_table old table as oldtab > > > for each statement > > > execute procedure my_trig(); > > > > > > truncate t1; > > > server closed the connection unexpectedly > > > This probably means the server terminated abnormally > > > before or while processing the request. > > > The connection to the server was lost. Attempting reset: Failed. > > > > Thanks. Reproduced here. The stack looks like this: > > > > frame #3: 0x000103e5e8b0 > > postgres`ExceptionalCondition(conditionName="!((trigdata->tg_event) > > & 0x0003) == 0x) || (((trigdata->tg_event) & 0x0003) > > == 0x0002) || (((trigdata->tg_event) & 0x0003) == 0x0001)) > > && (((trigdata->tg_event) & 0x0018) == 0x) && > > !(trigdata->tg_event & 0x0020) && !(trigdata->tg_event & > > 0x0040)) || (trigdata->tg_oldtable == ((void*)0) && > > trigdata->tg_newtable == ((void*)0)))", errorType="FailedAssertion", > > fileName="trigger.c", lineNumber=2045) + 128 at assert.c:54 > > frame #4: 0x000103a6f542 > > postgres`ExecCallTriggerFunc(trigdata=0x7fff5c40bad0, tgindx=0, > > finfo=0x7fd8ba0817b8, instr=0x, > > per_tuple_context=0x7fd8b906f928) + 258 at trigger.c:2039 > > frame #5: 0x000103a754ed > > postgres`AfterTriggerExecute(event=0x7fd8ba092460, > > rel=0x0001043fd9c0, trigdesc=0x7fd8ba068758, > > finfo=0x7fd8ba0817b8, instr=0x, > > per_tuple_context=0x7fd8b906f928, > > trig_tuple_slot1=0x, > > trig_tuple_slot2=0x) + 1469 at trigger.c:3860 > > frame #6: 0x000103a73080 > > postgres`afterTriggerInvokeEvents(events=0x7fd8ba07fb00, > > firing_id=1, estate=0x7fd8ba090440, delete_ok='\x01') + 592 at > > trigger.c:4051 > > frame #7: 0x000103a72b7b > > postgres`AfterTriggerEndQuery(estate=0x7fd8ba090440) + 203 at > > trigger.c:4227 > > frame #8: 0x000103a498aa > > postgres`ExecuteTruncate(stmt=0x7fd8ba059f40) + 2026 at > > tablecmds.c:1485 > > > > There's an assertion that it's (one of INSERT, UPDATE, DELETE, an > > AFTER trigger, not deferred) *or* there are no transition tables. > > Here it's TRUNCATE and there are transition tables, so it fails: > > > > /* > > * Protect against code paths that may fail to initialize transition > > table > > * info. > > */ > > Assert(((TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) || > > TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event) || > > TRIGGER_FIRED_BY_DELETE(trigdata->tg_event)) && > > TRIGGER_FIRED_AFTER(trigdata->tg_event) && > > !(trigdata->tg_event & AFTER_TRIGGER_DEFERRABLE) && > > !(trigdata->tg_event & AFTER_TRIGGER_INITDEFERRED)) || > >(trigdata->tg_oldtable == NULL && trigdata->tg_newtable == > > NULL)); > > > > > > We can't possibly support transition tables on TRUNCATE (the whole > > point of TRUNCATE is not to inspect all the rows so we can't collect > > them), and we already reject ROW triggers on TRUNCATE, so we should > > reject transition tables on STATEMENT triggers for TRUNCATE at > > creation time too. See attached. Thoughts? > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Kevin, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/
Re: [HACKERS] delta relations in AFTER triggers
On Sat, May 06, 2017 at 05:33:24AM +, Noah Misch wrote: > On Thu, May 04, 2017 at 09:51:03PM +1200, Thomas Munro wrote: > > On Thu, May 4, 2017 at 9:12 PM, Prabhat Sahu > > wrote: > > > I have been testing this for a while and observed a server crash while > > > referencing table column value in a trigger procedure for AFTER DELETE > > > trigger. > > > > > > -- Steps to reproduce: > > > CREATE TABLE t1(c1 int); > > > CREATE TABLE t2(cc1 int); > > > INSERT INTO t1 VALUES (10); > > > INSERT INTO t2 VALUES (10); > > > > > > CREATE OR REPLACE FUNCTION trig_func() RETURNS trigger AS > > > $$ BEGIN > > > DELETE FROM t1 WHERE c1 IN (select OLD.cc1 from my_old); > > > RETURN OLD; > > > END; $$ LANGUAGE PLPGSQL; > > > > > > CREATE TRIGGER trg1 > > > AFTER DELETE ON t2 > > > REFERENCING OLD TABLE AS my_old > > > FOR EACH ROW > > > EXECUTE PROCEDURE trig_func(); > > > > > > DELETE FROM t2 WHERE cc1 =10; > > > server closed the connection unexpectedly > > > This probably means the server terminated abnormally > > > before or while processing the request. > > > The connection to the server was lost. Attempting reset: Failed. > > > > Reproduced here. The stack looks like this: > > > > frame #3: 0x00010f06f8b0 > > postgres`ExceptionalCondition(conditionName="!(readptr->eflags & > > 0x0002)", errorType="FailedAssertion", fileName="tuplestore.c", > > lineNumber=1237) + 128 at assert.c:54 > > frame #4: 0x00010f0cbc85 > > postgres`tuplestore_rescan(state=0x7ff219840200) + 85 at > > tuplestore.c:1237 > > frame #5: 0x00010eced9b1 > > postgres`ExecReScanNamedTuplestoreScan(node=0x7ff21d007840) + 81 > > at nodeNamedtuplestorescan.c:197 > > frame #6: 0x00010eca46a6 > > postgres`ExecReScan(node=0x7ff21d007840) + 822 at execAmi.c:216 > > frame #7: 0x00010ece7eca > > postgres`ExecNestLoop(node=0x7ff21d006310) + 538 at > > nodeNestloop.c:148 > > > > I think the problem is that the tuplestore read pointer hasn't been > > opened with the "rewindable" flag. It works for me with the attached. > > [Action required within three days. This is a generic notification.] > > The above-described topic is currently a PostgreSQL 10 open item. Kevin, > since you committed the patch believed to have created it, you own this open > item. If some other commit is more relevant or if this does not belong as a > v10 open item, please let us know. Otherwise, please observe the policy on > open item ownership[1] and send a status update within three calendar days of > this message. Include a date for your subsequent status update. Testers may > discover new open items at any time, and I want to plan to get them all fixed > well in advance of shipping v10. Consequently, I will appreciate your efforts > toward speedy resolution. Thanks. > > [1] > https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com This PostgreSQL 10 open item is past due for your status update. Kindly send a status update within 24 hours, and include a date for your subsequent status update. Refer to the policy on open item ownership: https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, May 05, 2017 at 08:23:33AM +1200, Thomas Munro wrote: > On Fri, May 5, 2017 at 12:39 AM, Neha Sharma > wrote: > > While testing the feature we encountered one more crash,below is the > > scenario to reproduce. > > > > create table t1 ( a int); > > create table t2 ( a int); > > insert into t1 values (11),(12),(13); > > > > create or replace function my_trig() returns trigger > > language plpgsql as $my_trig$ > > begin > > insert into t2(select a from new_table); > > RETURN NEW; > > end; > > $my_trig$; > > > > create trigger my_trigger > > after truncate or update on t1 > > referencing new table as new_table old table as oldtab > > for each statement > > execute procedure my_trig(); > > > > truncate t1; > > server closed the connection unexpectedly > > This probably means the server terminated abnormally > > before or while processing the request. > > The connection to the server was lost. Attempting reset: Failed. > > Thanks. Reproduced here. The stack looks like this: > > frame #3: 0x000103e5e8b0 > postgres`ExceptionalCondition(conditionName="!((trigdata->tg_event) > & 0x0003) == 0x) || (((trigdata->tg_event) & 0x0003) > == 0x0002) || (((trigdata->tg_event) & 0x0003) == 0x0001)) > && (((trigdata->tg_event) & 0x0018) == 0x) && > !(trigdata->tg_event & 0x0020) && !(trigdata->tg_event & > 0x0040)) || (trigdata->tg_oldtable == ((void*)0) && > trigdata->tg_newtable == ((void*)0)))", errorType="FailedAssertion", > fileName="trigger.c", lineNumber=2045) + 128 at assert.c:54 > frame #4: 0x000103a6f542 > postgres`ExecCallTriggerFunc(trigdata=0x7fff5c40bad0, tgindx=0, > finfo=0x7fd8ba0817b8, instr=0x, > per_tuple_context=0x7fd8b906f928) + 258 at trigger.c:2039 > frame #5: 0x000103a754ed > postgres`AfterTriggerExecute(event=0x7fd8ba092460, > rel=0x0001043fd9c0, trigdesc=0x7fd8ba068758, > finfo=0x7fd8ba0817b8, instr=0x, > per_tuple_context=0x7fd8b906f928, > trig_tuple_slot1=0x, > trig_tuple_slot2=0x) + 1469 at trigger.c:3860 > frame #6: 0x000103a73080 > postgres`afterTriggerInvokeEvents(events=0x7fd8ba07fb00, > firing_id=1, estate=0x7fd8ba090440, delete_ok='\x01') + 592 at > trigger.c:4051 > frame #7: 0x000103a72b7b > postgres`AfterTriggerEndQuery(estate=0x7fd8ba090440) + 203 at > trigger.c:4227 > frame #8: 0x000103a498aa > postgres`ExecuteTruncate(stmt=0x7fd8ba059f40) + 2026 at > tablecmds.c:1485 > > There's an assertion that it's (one of INSERT, UPDATE, DELETE, an > AFTER trigger, not deferred) *or* there are no transition tables. > Here it's TRUNCATE and there are transition tables, so it fails: > > /* > * Protect against code paths that may fail to initialize transition table > * info. > */ > Assert(((TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) || > TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event) || > TRIGGER_FIRED_BY_DELETE(trigdata->tg_event)) && > TRIGGER_FIRED_AFTER(trigdata->tg_event) && > !(trigdata->tg_event & AFTER_TRIGGER_DEFERRABLE) && > !(trigdata->tg_event & AFTER_TRIGGER_INITDEFERRED)) || >(trigdata->tg_oldtable == NULL && trigdata->tg_newtable == NULL)); > > > We can't possibly support transition tables on TRUNCATE (the whole > point of TRUNCATE is not to inspect all the rows so we can't collect > them), and we already reject ROW triggers on TRUNCATE, so we should > reject transition tables on STATEMENT triggers for TRUNCATE at > creation time too. See attached. Thoughts? [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Kevin, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, May 04, 2017 at 09:51:03PM +1200, Thomas Munro wrote: > On Thu, May 4, 2017 at 9:12 PM, Prabhat Sahu > wrote: > > I have been testing this for a while and observed a server crash while > > referencing table column value in a trigger procedure for AFTER DELETE > > trigger. > > > > -- Steps to reproduce: > > CREATE TABLE t1(c1 int); > > CREATE TABLE t2(cc1 int); > > INSERT INTO t1 VALUES (10); > > INSERT INTO t2 VALUES (10); > > > > CREATE OR REPLACE FUNCTION trig_func() RETURNS trigger AS > > $$ BEGIN > > DELETE FROM t1 WHERE c1 IN (select OLD.cc1 from my_old); > > RETURN OLD; > > END; $$ LANGUAGE PLPGSQL; > > > > CREATE TRIGGER trg1 > > AFTER DELETE ON t2 > > REFERENCING OLD TABLE AS my_old > > FOR EACH ROW > > EXECUTE PROCEDURE trig_func(); > > > > DELETE FROM t2 WHERE cc1 =10; > > server closed the connection unexpectedly > > This probably means the server terminated abnormally > > before or while processing the request. > > The connection to the server was lost. Attempting reset: Failed. > > Reproduced here. The stack looks like this: > > frame #3: 0x00010f06f8b0 > postgres`ExceptionalCondition(conditionName="!(readptr->eflags & > 0x0002)", errorType="FailedAssertion", fileName="tuplestore.c", > lineNumber=1237) + 128 at assert.c:54 > frame #4: 0x00010f0cbc85 > postgres`tuplestore_rescan(state=0x7ff219840200) + 85 at > tuplestore.c:1237 > frame #5: 0x00010eced9b1 > postgres`ExecReScanNamedTuplestoreScan(node=0x7ff21d007840) + 81 > at nodeNamedtuplestorescan.c:197 > frame #6: 0x00010eca46a6 > postgres`ExecReScan(node=0x7ff21d007840) + 822 at execAmi.c:216 > frame #7: 0x00010ece7eca > postgres`ExecNestLoop(node=0x7ff21d006310) + 538 at > nodeNestloop.c:148 > > I think the problem is that the tuplestore read pointer hasn't been > opened with the "rewindable" flag. It works for me with the attached. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Kevin, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, May 5, 2017 at 12:39 AM, Neha Sharma wrote: > While testing the feature we encountered one more crash,below is the > scenario to reproduce. > > create table t1 ( a int); > create table t2 ( a int); > insert into t1 values (11),(12),(13); > > create or replace function my_trig() returns trigger > language plpgsql as $my_trig$ > begin > insert into t2(select a from new_table); > RETURN NEW; > end; > $my_trig$; > > create trigger my_trigger > after truncate or update on t1 > referencing new table as new_table old table as oldtab > for each statement > execute procedure my_trig(); > > truncate t1; > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. Thanks. Reproduced here. The stack looks like this: frame #3: 0x000103e5e8b0 postgres`ExceptionalCondition(conditionName="!((trigdata->tg_event) & 0x0003) == 0x) || (((trigdata->tg_event) & 0x0003) == 0x0002) || (((trigdata->tg_event) & 0x0003) == 0x0001)) && (((trigdata->tg_event) & 0x0018) == 0x) && !(trigdata->tg_event & 0x0020) && !(trigdata->tg_event & 0x0040)) || (trigdata->tg_oldtable == ((void*)0) && trigdata->tg_newtable == ((void*)0)))", errorType="FailedAssertion", fileName="trigger.c", lineNumber=2045) + 128 at assert.c:54 frame #4: 0x000103a6f542 postgres`ExecCallTriggerFunc(trigdata=0x7fff5c40bad0, tgindx=0, finfo=0x7fd8ba0817b8, instr=0x, per_tuple_context=0x7fd8b906f928) + 258 at trigger.c:2039 frame #5: 0x000103a754ed postgres`AfterTriggerExecute(event=0x7fd8ba092460, rel=0x0001043fd9c0, trigdesc=0x7fd8ba068758, finfo=0x7fd8ba0817b8, instr=0x, per_tuple_context=0x7fd8b906f928, trig_tuple_slot1=0x, trig_tuple_slot2=0x) + 1469 at trigger.c:3860 frame #6: 0x000103a73080 postgres`afterTriggerInvokeEvents(events=0x7fd8ba07fb00, firing_id=1, estate=0x7fd8ba090440, delete_ok='\x01') + 592 at trigger.c:4051 frame #7: 0x000103a72b7b postgres`AfterTriggerEndQuery(estate=0x7fd8ba090440) + 203 at trigger.c:4227 frame #8: 0x000103a498aa postgres`ExecuteTruncate(stmt=0x7fd8ba059f40) + 2026 at tablecmds.c:1485 There's an assertion that it's (one of INSERT, UPDATE, DELETE, an AFTER trigger, not deferred) *or* there are no transition tables. Here it's TRUNCATE and there are transition tables, so it fails: /* * Protect against code paths that may fail to initialize transition table * info. */ Assert(((TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) || TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event) || TRIGGER_FIRED_BY_DELETE(trigdata->tg_event)) && TRIGGER_FIRED_AFTER(trigdata->tg_event) && !(trigdata->tg_event & AFTER_TRIGGER_DEFERRABLE) && !(trigdata->tg_event & AFTER_TRIGGER_INITDEFERRED)) || (trigdata->tg_oldtable == NULL && trigdata->tg_newtable == NULL)); We can't possibly support transition tables on TRUNCATE (the whole point of TRUNCATE is not to inspect all the rows so we can't collect them), and we already reject ROW triggers on TRUNCATE, so we should reject transition tables on STATEMENT triggers for TRUNCATE at creation time too. See attached. Thoughts? > Log file and core dump attached for reference. Thanks! Just by the way, it'd be better to post just an interesting stack trace fragment rather than a core file, because core files can't really be used without the exact executable that you built. -- Thomas Munro http://www.enterprisedb.com no-transition-tables-for-truncate.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, May 4, 2017 at 9:12 PM, Prabhat Sahu wrote: > I have been testing this for a while and observed a server crash while > referencing table column value in a trigger procedure for AFTER DELETE > trigger. > > -- Steps to reproduce: > CREATE TABLE t1(c1 int); > CREATE TABLE t2(cc1 int); > INSERT INTO t1 VALUES (10); > INSERT INTO t2 VALUES (10); > > CREATE OR REPLACE FUNCTION trig_func() RETURNS trigger AS > $$ BEGIN > DELETE FROM t1 WHERE c1 IN (select OLD.cc1 from my_old); > RETURN OLD; > END; $$ LANGUAGE PLPGSQL; > > CREATE TRIGGER trg1 > AFTER DELETE ON t2 > REFERENCING OLD TABLE AS my_old > FOR EACH ROW > EXECUTE PROCEDURE trig_func(); > > DELETE FROM t2 WHERE cc1 =10; > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > The connection to the server was lost. Attempting reset: Failed. Reproduced here. The stack looks like this: frame #3: 0x00010f06f8b0 postgres`ExceptionalCondition(conditionName="!(readptr->eflags & 0x0002)", errorType="FailedAssertion", fileName="tuplestore.c", lineNumber=1237) + 128 at assert.c:54 frame #4: 0x00010f0cbc85 postgres`tuplestore_rescan(state=0x7ff219840200) + 85 at tuplestore.c:1237 frame #5: 0x00010eced9b1 postgres`ExecReScanNamedTuplestoreScan(node=0x7ff21d007840) + 81 at nodeNamedtuplestorescan.c:197 frame #6: 0x00010eca46a6 postgres`ExecReScan(node=0x7ff21d007840) + 822 at execAmi.c:216 frame #7: 0x00010ece7eca postgres`ExecNestLoop(node=0x7ff21d006310) + 538 at nodeNestloop.c:148 I think the problem is that the tuplestore read pointer hasn't been opened with the "rewindable" flag. It works for me with the attached. -- Thomas Munro http://www.enterprisedb.com fix-named-tuplestore-rescan.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
> > Great. Thanks. I wonder if there is some way we can automatically > include code fragments in the documentation without keeping them in > sync manually. > > In whatever extra docs you add, could you include an example of an INSERT ON CONFLICT, and potentially a CTE query that does two operations on the same table. I'm not clear on what to expect when a statement does a mix of INSERT, UPDATE, and DELETE? Will there be multiple firings of the trigger in a single statement, or will the before/after sets be mashed together regardless of which part of the query generated it?
Re: [HACKERS] delta relations in AFTER triggers
On Wed, Apr 5, 2017 at 11:49 AM, Kevin Grittner wrote: > Worked on the docs some more and then pushed it. > > Nice job cutting the number of *.[ch] lines by 30 while adding support for > the other three core PLs. :-) Great. Thanks. I wonder if there is some way we can automatically include code fragments in the documentation without keeping them in sync manually. Now it looks like I have no more excuses for putting off reading the papers you've shared[1][2] about incremental matview algorithms! Looking forward to helping out with the next steps in that project if I can. [1] http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.40.2254&rep=rep1&type=pdf [2] http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.31.3208&rep=rep1&type=pdf -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Apr 3, 2017 at 7:16 PM, Thomas Munro wrote: > On Tue, Apr 4, 2017 at 3:41 AM, Kevin Grittner wrote: >> On Mon, Apr 3, 2017 at 8:59 AM, Tom Lane wrote: >>> Thomas Munro writes: Or perhaps the code to inject trigger data transition tables into SPI (a near identical code block these three patches) should be somewhere common so that each PLs would only need to call a function. If so, where should that go? >>> >>> spi.c? >> >> Until now, trigger.c didn't know about SPI, and spi.c didn't know >> about triggers. The intersection was left to referencing code, like >> PLs. Is there any other common code among the PLs dealing with this >> intersection? If so, maybe a new triggerspi.c file (or >> spitrigger.c?) would make sense. Possibly it could make sense from >> a code structure PoV even for a single function, but it seems kinda >> iffy for just this function. As far as I can see it comes down to >> adding it to spi.c or creating a new file -- or just duplicating >> these 30-some lines of code to every PL. > > Ok, how about SPI_register_trigger_data(TriggerData *)? Or any name > you prefer... I didn't suggest something as specific as > SPI_register_transition_tables because think it's plausible that > someone might want to implement SQL standard REFERENCING OLD/NEW ROW > AS and make that work in all PLs too one day, and this would be the > place to do that. > > See attached, which add adds the call to all four built-in PLs. Thoughts? Worked on the docs some more and then pushed it. Nice job cutting the number of *.[ch] lines by 30 while adding support for the other three core PLs. :-) -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Apr 4, 2017 at 3:41 AM, Kevin Grittner wrote: > On Mon, Apr 3, 2017 at 8:59 AM, Tom Lane wrote: >> Thomas Munro writes: >>> Or perhaps the code to inject trigger data transition tables into SPI >>> (a near identical code block these three patches) should be somewhere >>> common so that each PLs would only need to call a function. If so, >>> where should that go? >> >> spi.c? > > Until now, trigger.c didn't know about SPI, and spi.c didn't know > about triggers. The intersection was left to referencing code, like > PLs. Is there any other common code among the PLs dealing with this > intersection? If so, maybe a new triggerspi.c file (or > spitrigger.c?) would make sense. Possibly it could make sense from > a code structure PoV even for a single function, but it seems kinda > iffy for just this function. As far as I can see it comes down to > adding it to spi.c or creating a new file -- or just duplicating > these 30-some lines of code to every PL. Ok, how about SPI_register_trigger_data(TriggerData *)? Or any name you prefer... I didn't suggest something as specific as SPI_register_transition_tables because think it's plausible that someone might want to implement SQL standard REFERENCING OLD/NEW ROW AS and make that work in all PLs too one day, and this would be the place to do that. See attached, which add adds the call to all four built-in PLs. Thoughts? -- Thomas Munro http://www.enterprisedb.com transition-tables-for-all.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Apr 3, 2017 at 8:59 AM, Tom Lane wrote: > Thomas Munro writes: >> Or perhaps the code to inject trigger data transition tables into SPI >> (a near identical code block these three patches) should be somewhere >> common so that each PLs would only need to call a function. If so, >> where should that go? > > spi.c? Until now, trigger.c didn't know about SPI, and spi.c didn't know about triggers. The intersection was left to referencing code, like PLs. Is there any other common code among the PLs dealing with this intersection? If so, maybe a new triggerspi.c file (or spitrigger.c?) would make sense. Possibly it could make sense from a code structure PoV even for a single function, but it seems kinda iffy for just this function. As far as I can see it comes down to adding it to spi.c or creating a new file -- or just duplicating these 30-some lines of code to every PL. -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Thomas Munro writes: > Or perhaps the code to inject trigger data transition tables into SPI > (a near identical code block these three patches) should be somewhere > common so that each PLs would only need to call a function. If so, > where should that go? spi.c? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Apr 3, 2017 at 3:50 PM, Thomas Munro wrote: > Please also find attached a rebased patch to add pl/python support, > and new equivalent patches for pl/perl and pl/tcl. I am planning to > add these to PG11 CF1, unless you think we should be more aggressive > given the extra time? Or perhaps the code to inject trigger data transition tables into SPI (a near identical code block these three patches) should be somewhere common so that each PLs would only need to call a function. If so, where should that go? -- 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
Re: [HACKERS] delta relations in AFTER triggers
Happy to see this committed! And thanks for the co-author credit,
which is a generous exaggeration.
I was still a bit confused about this and poked at it a bit:
On Wed, Mar 8, 2017 at 1:28 PM, Kevin Grittner wrote:
>> /*
>> + * Capture the NEW and OLD transition TABLE tuplestores (if specified for
>> + * this trigger).
>> + */
>> + if (trigdata->tg_newtable || trigdata->tg_oldtable)
>> + {
>> + estate.queryEnv = create_queryEnv();
>> + if (trigdata->tg_newtable)
>> + {
>> + Enr enr = palloc(sizeof(EnrData));
>> +
>> + enr->md.name = trigdata->tg_trigger->tgnewtable;
>> + enr->md.tupdesc = trigdata->tg_relation->rd_att;
>> + enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable);
>> + enr->reldata = trigdata->tg_newtable;
>> + register_enr(estate.queryEnv, enr);
>> + SPI_register_relation(enr);
>> + }
>>
>> Why do we we have to call register_enr and also SPI_register_relation here?
>
> Essentially, because plpgsql does some things through SPI and some
> things not. Both cases are covered.
We're maintaining two different QueryEnvironment objects here, one
inside the SPI module and another in plpgsql_EState. I think that's
done only so that we have one to inject into the portal in
exec_dynquery_with_params, so that EXECUTE 'SELECT * FROM
' can work.
That raises the question: shouldn't SPI_cursor_open just do that
itself using the SPI connection's current QueryEnvironment? That
would make SPI_cursor_open consistent with SPI_execute_plan, and also
benefit handlers for other PLs that would otherwise have to do similar
double-bookkeeping. See attached patch showing what I mean.
Please also find attached a rebased patch to add pl/python support,
and new equivalent patches for pl/perl and pl/tcl. I am planning to
add these to PG11 CF1, unless you think we should be more aggressive
given the extra time?
--
Thomas Munro
http://www.enterprisedb.com
spi-portal-open-with-query-env.patch
Description: Binary data
transition-plpython-v1.patch
Description: Binary data
transition-plperl-v1.patch
Description: Binary data
transition-pltcl-v1.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
> > > That, and I suspect that people will start using this infrastructure > > for some very cool projects. > > Yeah, I was excited to see this committed. It practically begs to be > used for plpgsql TABLE valued variables backed by tuplestores. > (also very excited about this!)
Re: [HACKERS] delta relations in AFTER triggers
On 1 April 2017 at 02:29, David Fetter wrote: > On Fri, Mar 31, 2017 at 12:20:51PM -0500, Kevin Grittner wrote: >> On Thu, Mar 30, 2017 at 11:51 AM, Kevin Grittner wrote: >> >> > New version attached. It needs some of these problem cases added >> > to the testing, and a mention in the docs that only C and plpgsql >> > triggers can use the feature so far. I'll add those tomorrow. >> >> Done and attached. >> >> Now the question is, should it be pushed? > > Yes. Among other things, that'll get it buildfarm tested and give > people interested in other PLs better visibility. > > That, and I suspect that people will start using this infrastructure > for some very cool projects. Yeah, I was excited to see this committed. It practically begs to be used for plpgsql TABLE valued variables backed by tuplestores. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 31, 2017 at 12:20:51PM -0500, Kevin Grittner wrote: > On Thu, Mar 30, 2017 at 11:51 AM, Kevin Grittner wrote: > > > New version attached. It needs some of these problem cases added > > to the testing, and a mention in the docs that only C and plpgsql > > triggers can use the feature so far. I'll add those tomorrow. > > Done and attached. > > Now the question is, should it be pushed? Yes. Among other things, that'll get it buildfarm tested and give people interested in other PLs better visibility. That, and I suspect that people will start using this infrastructure for some very cool projects. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 31, 2017 at 12:58 PM, Robert Haas wrote: > On Fri, Mar 31, 2017 at 1:50 PM, Tom Lane wrote: >> I would vote for calling the struct typedef EphemeralNamedRelation and >> using the abbreviation ENR (capitalized that way, not as Enr) in related >> function names, where that seemed sensible. I really do *not* like >> "Enr" as a global name. > > Yeah, I had the same thought about capitalization but wasn't sure if > it was worth suggesting. But since Tom did, +1 from me. Will do. -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 31, 2017 at 1:50 PM, Tom Lane wrote: > Kevin Grittner writes: >> On Thu, Mar 23, 2017 at 7:14 PM, Thomas Munro >> wrote: >>> my only other comment would be a bikeshed issue: >>> Enr isn't a great name for a struct. > >> I know, but EphemeralNamedRelation starts to get kinda long, >> especially when making the normal sorts of concatenations. I >> started making the change and balked when I saw things like >> EphemeralNamedRelationMetadataData and a function named >> EphemeralNamedRelationMetadataGetTupDesc() in place of >> EnrmdGetTupDesc(). A 40 character function name make for a lot of >> line-wrapping to stay within pgindent limits. Any suggestions? > > I would vote for calling the struct typedef EphemeralNamedRelation and > using the abbreviation ENR (capitalized that way, not as Enr) in related > function names, where that seemed sensible. I really do *not* like > "Enr" as a global name. Yeah, I had the same thought about capitalization but wasn't sure if it was worth suggesting. But since Tom did, +1 from me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Kevin Grittner writes: > On Thu, Mar 23, 2017 at 7:14 PM, Thomas Munro > wrote: >> my only other comment would be a bikeshed issue: >> Enr isn't a great name for a struct. > I know, but EphemeralNamedRelation starts to get kinda long, > especially when making the normal sorts of concatenations. I > started making the change and balked when I saw things like > EphemeralNamedRelationMetadataData and a function named > EphemeralNamedRelationMetadataGetTupDesc() in place of > EnrmdGetTupDesc(). A 40 character function name make for a lot of > line-wrapping to stay within pgindent limits. Any suggestions? I would vote for calling the struct typedef EphemeralNamedRelation and using the abbreviation ENR (capitalized that way, not as Enr) in related function names, where that seemed sensible. I really do *not* like "Enr" as a global name. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Mar 30, 2017 at 11:51 AM, Kevin Grittner wrote: > New version attached. It needs some of these problem cases added to > the testing, and a mention in the docs that only C and plpgsql > triggers can use the feature so far. I'll add those tomorrow. Done and attached. Now the question is, should it be pushed? It's been through just about every CF in the last three years with little modification, and finally got a thorough enough review in this CF that I think it can be considered. Here are the numbers: 85 files changed, 2266 insertions(+), 132 deletions(-) Of that, 70 lines are the plpgsql implementation (which I should probably push separately), about 200 lines are docs and 623 lines are new regression tests. Most of the rest only comes into play if the feature is used. This adds support for SQL standard sub-feature, although only in triggers written in C and plpgsql. (Other PLs will probably require fewer lines than plpgsql.) It also provides infrastructure needed to get incremental maintenance of materialized views based on just simple declarative DDL. Tom has expressed hope that it could be used to improve performance and memory usage for AFTER triggers, and I believe it can, but that that should be a follow-on patch. It might provide the basis of set-based statement-level enforcement of referential integrity, with the regression tests providing a rough proof of concept. My inclination is to push it late today, but be ready to revert if there are any hard-to-fix surprise problems missed in review and testing; but if the general preference is to hold it for version 11, that's OK with me, too. -- Kevin Grittner transition-v14.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Mar 23, 2017 at 7:14 PM, Thomas Munro wrote: > my only other comment would be a bikeshed issue: > Enr isn't a great name for a struct. I know, but EphemeralNamedRelation starts to get kinda long, especially when making the normal sorts of concatenations. I started making the change and balked when I saw things like EphemeralNamedRelationMetadataData and a function named EphemeralNamedRelationMetadataGetTupDesc() in place of EnrmdGetTupDesc(). A 40 character function name make for a lot of line-wrapping to stay within pgindent limits. Any suggestions? -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Mar 23, 2017 at 11:36 PM, Thomas Munro wrote: > One more thought: should this be allowed? > > postgres=# create table mytab (i int) partition by list (i); > CREATE TABLE > postgres=# create table mytab1 partition of mytab for values in (42); > CREATE TABLE > postgres=# create function my_trigger_function() returns trigger as $$ > begin end; $$ language plpgsql; > CREATE FUNCTION > postgres=# create trigger my_trigger after update on mytab referencing > old table as my_old for each statement execute procedure > my_trigger_function(); > CREATE TRIGGER > Perhaps the moral equivalent should be possible for statement triggers > with transition tables, and that already works with your patch as far > as I know. So I think your patch probably just needs to reject them > on partitioned tables. > [patch provided] Yeah, that looks good. Included in next patch version. On Sun, Mar 26, 2017 at 6:39 AM, Thomas Munro wrote: > BTW I had to make the following change to your v12 because of commit b8d7f053: Yeah, I ran into that, too, and used exactly the same fix. On Sun, Mar 26, 2017 at 6:39 AM, Thomas Munro wrote: > On Fri, Mar 24, 2017 at 1:14 PM, Thomas Munro >> When PlanCacheRelCallback runs, I don't think it understands that >> these named tuplestore RangeTblEntry objects are dependent on the >> subject table. Could that be fixed like this? >> >> @@ -2571,6 +2582,9 @@ extract_query_dependencies_walker(Node *node, >> PlannerInfo *context) >> if (rte->rtekind == RTE_RELATION) >> context->glob->relationOids = >> >> lappend_oid(context->glob->relationOids, rte->relid); >> + else if (rte->rtekind == RTE_NAMEDTUPLESTORE) >> + context->glob->relationOids = >> + >> lappend_oid(context->glob->relationOids, [subject table's OID]); > > I'm not sure if this is the right approach and it may have style > issues, but it does fix the crashing in the ALTER TABLE case I > reported: see attached patch which applies on top of your v12. I had been working along similar lines, but had not gotten it working. Merged your version and mine, taking the best of both. :-) Thanks for the reviews and the fixes! New version attached. It needs some of these problem cases added to the testing, and a mention in the docs that only C and plpgsql triggers can use the feature so far. I'll add those tomorrow. -- Kevin Grittner transition-v13.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 24, 2017 at 1:14 PM, Thomas Munro wrote: > On Tue, Mar 14, 2017 at 7:51 AM, Kevin Grittner wrote: >> On Sun, Mar 12, 2017 at 4:08 PM, Thomas Munro >> wrote: >>> I found a new way to break it: run the trigger function so >>> that the plan is cached by plpgsql, then ALTER TABLE incompatibly, >>> then run the trigger function again. See attached. >> >> [...] >> >> I expected that existing mechanisms would have forced re-planning of >> a trigger function if the table the function was attached to was >> altered. Either that was a bit "optimistic", or the old TupleDesc >> is used for the new plan. Will track down which it is, and fix it. > > When PlanCacheRelCallback runs, I don't think it understands that > these named tuplestore RangeTblEntry objects are dependent on the > subject table. Could that be fixed like this? > > @@ -2571,6 +2582,9 @@ extract_query_dependencies_walker(Node *node, > PlannerInfo *context) > if (rte->rtekind == RTE_RELATION) > context->glob->relationOids = > > lappend_oid(context->glob->relationOids, rte->relid); > + else if (rte->rtekind == RTE_NAMEDTUPLESTORE) > + context->glob->relationOids = > + > lappend_oid(context->glob->relationOids, [subject table's OID]); I'm not sure if this is the right approach and it may have style issues, but it does fix the crashing in the ALTER TABLE case I reported: see attached patch which applies on top of your v12. BTW I had to make the following change to your v12 because of commit b8d7f053: /* * initialize child expressions */ -scanstate->ss.ps.targetlist = (List *) -ExecInitExpr((Expr *) node->scan.plan.targetlist, - (PlanState *) scanstate); -scanstate->ss.ps.qual = (List *) -ExecInitExpr((Expr *) node->scan.plan.qual, - (PlanState *) scanstate); +scanstate->ss.ps.qual = +ExecInitQual(node->scan.plan.qual, (PlanState *) scanstate); -- Thomas Munro http://www.enterprisedb.com experimental-fix-for-transition-table-invalidation.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 24, 2017 at 5:36 PM, Thomas Munro wrote: > On Fri, Mar 24, 2017 at 1:14 PM, Thomas Munro > wrote: >> If that's fixed and the permissions question can be waved away by >> saying it's the same as the per-row situation, my only other comment >> would be a bikeshed issue: Enr isn't a great name for a struct. > > One more thought: should this be allowed? > > postgres=# create table mytab (i int) partition by list (i); > CREATE TABLE > postgres=# create table mytab1 partition of mytab for values in (42); > CREATE TABLE > postgres=# create function my_trigger_function() returns trigger as $$ > begin end; $$ language plpgsql; > CREATE FUNCTION > postgres=# create trigger my_trigger after update on mytab referencing > old table as my_old for each statement execute procedure > my_trigger_function(); > CREATE TRIGGER On second thoughts, that's actually arguably a bug in committed code. What do you think about the attached patch? -- Thomas Munro http://www.enterprisedb.com no-transition-tables-for-partitioned-tables.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 24, 2017 at 1:14 PM, Thomas Munro wrote: > If that's fixed and the permissions question can be waved away by > saying it's the same as the per-row situation, my only other comment > would be a bikeshed issue: Enr isn't a great name for a struct. One more thought: should this be allowed? postgres=# create table mytab (i int) partition by list (i); CREATE TABLE postgres=# create table mytab1 partition of mytab for values in (42); CREATE TABLE postgres=# create function my_trigger_function() returns trigger as $$ begin end; $$ language plpgsql; CREATE FUNCTION postgres=# create trigger my_trigger after update on mytab referencing old table as my_old for each statement execute procedure my_trigger_function(); CREATE TRIGGER I haven't really looked into the interaction of triggers and the new partition feature very much but it looks like the intention is that you shouldn't be allowed to do something that would need access to the actual row data from a trigger that is attached to the top-level partition: postgres=# create trigger my_trigger before update on mytab for each row execute procedure my_trigger_function(); ERROR: "mytab" is a partitioned table DETAIL: Partitioned tables cannot have ROW triggers. By the same logic, I guess that we shouldn't allow transition table triggers to be attached to the top level partitioned table, because it can't really work. You can attach ROW triggers to the concrete tables that hold real data, which makes sense because they actually have data to capture and feed to the trigger function: postgres=# create trigger my_trigger before update on mytab1 for each row execute procedure my_trigger_function(); CREATE TRIGGER Perhaps the moral equivalent should be possible for statement triggers with transition tables, and that already works with your patch as far as I know. So I think your patch probably just needs to reject them on partitioned tables. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Mar 14, 2017 at 7:51 AM, Kevin Grittner wrote: > On Sun, Mar 12, 2017 at 4:08 PM, Thomas Munro > wrote: >> I found a new way to break it: run the trigger function so >> that the plan is cached by plpgsql, then ALTER TABLE incompatibly, >> then run the trigger function again. See attached. > > [...] > > I expected that existing mechanisms would have forced re-planning of > a trigger function if the table the function was attached to was > altered. Either that was a bit "optimistic", or the old TupleDesc > is used for the new plan. Will track down which it is, and fix it. When PlanCacheRelCallback runs, I don't think it understands that these named tuplestore RangeTblEntry objects are dependent on the subject table. Could that be fixed like this? @@ -2571,6 +2582,9 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context) if (rte->rtekind == RTE_RELATION) context->glob->relationOids = lappend_oid(context->glob->relationOids, rte->relid); + else if (rte->rtekind == RTE_NAMEDTUPLESTORE) + context->glob->relationOids = + lappend_oid(context->glob->relationOids, [subject table's OID]); } > I'll post a new patch once I figure out the dropped column on the > cached function plan. If that's fixed and the permissions question can be waved away by saying it's the same as the per-row situation, my only other comment would be a bikeshed issue: Enr isn't a great name for a struct. Very keen to see this feature in PostgreSQL 10! -- 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
Re: [HACKERS] delta relations in AFTER triggers
Kevin Grittner wrote: > >> What is necessary to indicate an additional SQL feature covered? > > > > I assume you're talking about information_schema.sql_features > > I had forgotten we had that in a table. I was thinking more of the docs: > > https://www.postgresql.org/docs/current/static/features.html > > I guess if we change one, we had better change the other. (Or are > they generated off a common source?) See src/backend/catalog/sql_features.txt -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Mar 12, 2017 at 4:08 PM, Thomas Munro wrote: > I found a new way to break it: run the trigger function so > that the plan is cached by plpgsql, then ALTER TABLE incompatibly, > then run the trigger function again. See attached. The first part doesn't seem so bad. Using the transition table in a FOR EACH STATEMENT trigger you get: test=# update hoge set name = (name::text || name::text)::integer; ERROR: attribute 2 has wrong type DETAIL: Table has type integer, but query expects text. CONTEXT: SQL statement "SELECT (SELECT string_agg(id || '=' || name, ',') FROM d)" PL/pgSQL function hoge_upd_func() line 3 at RAISE ... while putting each row on its own line from a FOR EACH ROW trigger you get: test=# update hoge set name = (name::text || name::text)::integer; ERROR: type of parameter 15 (integer) does not match that when preparing the plan (text) CONTEXT: SQL statement "SELECT (SELECT string_agg(old.id || '=' || old.name, ','))" PL/pgSQL function hoge_upd_func() line 3 at RAISE Does anyone think the first message needs improvement? If so, to what? Obviously the next part is a problem. With the transition table we get a segfault: test=# -- now drop column 'name' test=# alter table hoge drop column name; ALTER TABLE test=# update hoge set id = id; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. While the row-oriented query manages to dodge it: test=# alter table hoge drop column name; ALTER TABLE test=# update hoge set id = id; ERROR: record "old" has no field "name" CONTEXT: SQL statement "SELECT (SELECT string_agg(old.id || '=' || old.name, ','))" PL/pgSQL function hoge_upd_func() line 3 at RAISE I expected that existing mechanisms would have forced re-planning of a trigger function if the table the function was attached to was altered. Either that was a bit "optimistic", or the old TupleDesc is used for the new plan. Will track down which it is, and fix it. >> Do you suppose we should have all PLs that are part of the base >> distro covered? > > I vote for doing that in Postgres 11. My pl/python patch[1] may be a > useful starting point, but I haven't submitted it in this CF and > nobody has shown up with pl/tcl or pl/perl versions. OK, but we'd better add something to the docs saying that only C and plpgsql triggers are supported in v10. >> What is necessary to indicate an additional SQL feature covered? > > I assume you're talking about information_schema.sql_features I had forgotten we had that in a table. I was thinking more of the docs: https://www.postgresql.org/docs/current/static/features.html I guess if we change one, we had better change the other. (Or are they generated off a common source?) > a couple of thoughts occurred to me when looking for > references to transition tables in an old draft standard I have. > These are both cases where properties of the subject table should > probably also affect access to the derived transition tables: > > * What privileges implications are there for transition tables? I'm > wondering about column and row level privileges; for example, if you > can't see a column in the subject table, I'm guessing you shouldn't be > allowed to see it in the transition table either, but I'm not sure. I'll see how that works in FOR EACH ROW triggers. We should match that, I think. Keep in mind that not just anyone can put a trigger on a table. > * In future we could consider teaching it about functional > dependencies as required by the spec; if you can SELECT id, name FROM > GROUP BY id, I believe you should be able to SELECT > id, name FROM GROUP BY id, but currently you can't. Interesting idea. I'll post a new patch once I figure out the dropped column on the cached function plan. -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Mar 10, 2017 at 11:48 AM, Kevin Grittner wrote: > On Tue, Mar 7, 2017 at 6:28 PM, Kevin Grittner wrote: > >> New patch attached. > > And bit-rotted less than 24 hours later by fcec6caa. > > New patch attached just to fix bit-rot. > > That conflicting patch might be a candidate to merge into the new > Ephemeral Named Relation provided by my patch, for more flexibility > and extensibility... Thanks. I found a new way to break it: run the trigger function so that the plan is cached by plpgsql, then ALTER TABLE incompatibly, then run the trigger function again. See attached. On Wed, Mar 8, 2017 at 1:28 PM, Kevin Grittner wrote: >>> I was looking for omissions that would cause some kind of statements >>> to miss out on ENRs arbitrarily. It seemed to me that >>> parse_analyze_varparams should take a QueryEnvironment, mirroring >>> parse_analyze, so that PrepareQuery could pass it in. Otherwise, >>> PREPARE wouldn't see ENRs. Is there any reason why SPI_prepare should >>> see them but PREPARE not? >> >> Any thoughts about that? > > Do you see any way to test that code, or would it be dead code there > "just in case" we later decided to do something that needed it? I'm > not a big fan of the latter. I've had to spend too much time > maintaining and/or ripping out code that fits that description. I guess you could test it by reaching PREPARE and EXECUTE via dynamic SQL inside a plpgsql function (ie EXECUTE 'EXECUTE ...'). Really I was just trying to be thorough and examine every path into the parser and analyser to make sure they all supported the new QueryEnvironment argument. When I found that the PREPARE path didn't, my first thought was that there may be PLs that wouldn't be able to take advantage of plan reuse any other way, but I see that all the built-in PLs expose SPI_prepare, so that isn't a problem for them. You're probably right that it's not actually very useful. We've recorded this obscure omission in the archives. > Miscellanea: > > Do you suppose we should have all PLs that are part of the base > distro covered? I vote for doing that in Postgres 11. My pl/python patch[1] may be a useful starting point, but I haven't submitted it in this CF and nobody has shown up with pl/tcl or pl/perl versions. > What is necessary to indicate an additional SQL feature covered? I assume you're talking about information_schema.sql_features, and I see you've created a new thread to talk about that. I'm not sure about that, but a couple of thoughts occurred to me when looking for references to transition tables in an old draft standard I have. These are both cases where properties of the subject table should probably also affect access to the derived transition tables: * What privileges implications are there for transition tables? I'm wondering about column and row level privileges; for example, if you can't see a column in the subject table, I'm guessing you shouldn't be allowed to see it in the transition table either, but I'm not sure. * In future we could consider teaching it about functional dependencies as required by the spec; if you can SELECT id, name FROM GROUP BY id, I believe you should be able to SELECT id, name FROM GROUP BY id, but currently you can't. [1] https://www.postgresql.org/message-id/CAEepm=3wvmpmz3bkftk2kcnd9kr7hxpz2skj8sfzx_vsute...@mail.gmail.com -- Thomas Munro http://www.enterprisedb.com DROP TABLE IF EXISTS hoge; CREATE TABLE hoge ( id int primary key, name text ); CREATE OR REPLACE FUNCTION hoge_upd_func() RETURNS TRIGGER LANGUAGE plpgsql AS $$ BEGIN RAISE WARNING 'old table = %, new table = %', (SELECT string_agg(id || '=' || name, ',') FROM d), (SELECT string_agg(id || '=' || name, ',') FROM i); RETURN NULL; END; $$; CREATE TRIGGER hoge_upd_trigger AFTER UPDATE ON hoge REFERENCING OLD TABLE AS d NEW TABLE AS i FOR EACH STATEMENT EXECUTE PROCEDURE hoge_upd_func(); insert into hoge values (1, '1'), (2, '2'), (3, '3'); update hoge set name = name || name; -- now change 'name' to an integer to see what happens... alter table hoge alter column name type int using name::integer; update hoge set name = (name::text || name::text)::integer; -- at this point we get an error message: -- ERROR: attribute 2 has wrong type -- DETAIL: Table has type integer, but query expects text. -- That error ^ can be cleared by recreating the function hoge_upd_func. -- now drop column 'name' alter table hoge drop column name; update hoge set id = id; -- segfault! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Mar 7, 2017 at 6:28 PM, Kevin Grittner wrote: > New patch attached. And bit-rotted less than 24 hours later by fcec6caa. New patch attached just to fix bit-rot. That conflicting patch might be a candidate to merge into the new Ephemeral Named Relation provided by my patch, for more flexibility and extensibility... -- Kevin Grittner transition-v12.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Feb 20, 2017 at 7:44 PM, Thomas Munro
wrote:
> Was it intentional that this test doesn't include any statements that
> reach the case where the trigger does RAISE EXCEPTION 'RI error'?
> From the output generated there doesn't seem to be any evidence that
> the triggers run at all, though I know from playing around with this
> that they do
Tests expanded to cover more. Some bugs found and fixed in the process. :-/
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
>
> These copyright messages are missing 3 years' worth of bugfixes.
Those were only in files created with the initial patch in 2014. No
bug fixes missing. Updated the years to 2017, though.
> +SPI_get_caller_relation(const char *name)
>
> Do we need this function? It's not used by the implementation.
Good point. It seemed useful way back when, but since no uses for
it emerged, it should be removed until such time (if any) that it
would be useful.
> +typedef struct NamedTuplestoreScan
> +{
> + Scan scan;
> + char *enrname;
> +} NamedTuplestoreScan;
>
> Nearly plan node structs always have a comment for the members below
> 'scan'; I think this needs one too because 'enrname' is not
> self-explanatory.
Done.
> /*
> + * Capture the NEW and OLD transition TABLE tuplestores (if specified for
> + * this trigger).
> + */
> + if (trigdata->tg_newtable || trigdata->tg_oldtable)
> + {
> + estate.queryEnv = create_queryEnv();
> + if (trigdata->tg_newtable)
> + {
> + Enr enr = palloc(sizeof(EnrData));
> +
> + enr->md.name = trigdata->tg_trigger->tgnewtable;
> + enr->md.tupdesc = trigdata->tg_relation->rd_att;
> + enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable);
> + enr->reldata = trigdata->tg_newtable;
> + register_enr(estate.queryEnv, enr);
> + SPI_register_relation(enr);
> + }
>
> Why do we we have to call register_enr and also SPI_register_relation here?
Essentially, because plpgsql does some things through SPI and some
things not. Both cases are covered.
> On Mon, Dec 19, 2016 at 4:35 AM, Thomas Munro
> wrote:
>> On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner wrote:
>>> There were a few changes Thomas included in the version he posted,
>>> without really delving into an explanation for those changes. Some
>>> or all of them are likely to be worthwhile, but I would rather
>>> incorporate them based on explicit discussion, so this version
>>> doesn't do much other than generalize the interface a little,
>>> change some names, and add more regression tests for the new
>>> feature. (The examples I worked up for the rough proof of concept
>>> of enforcement of RI through set logic rather than row-at-a-time
>>> navigation were the basis for the new tests, so the idea won't get
>>> totally lost.) Thomas, please discuss each suggested change (e.g.,
>>> the inclusion of the query environment in the parameter list of a
>>> few more functions).
>>
>> I was looking for omissions that would cause some kind of statements
>> to miss out on ENRs arbitrarily. It seemed to me that
>> parse_analyze_varparams should take a QueryEnvironment, mirroring
>> parse_analyze, so that PrepareQuery could pass it in. Otherwise,
>> PREPARE wouldn't see ENRs. Is there any reason why SPI_prepare should
>> see them but PREPARE not?
>
> Any thoughts about that?
Do you see any way to test that code, or would it be dead code there
"just in case" we later decided to do something that needed it? I'm
not a big fan of the latter. I've had to spend too much time
maintaining and/or ripping out code that fits that description.
On Mon, Feb 20, 2017 at 9:43 PM, Thomas Munro
wrote:
> On Tue, Feb 21, 2017 at 7:14 AM, Thomas Munro
> wrote:
>> On Fri, Jan 20, 2017 at 2:49 AM, Kevin Grittner wrote:
>>> Attached is v9 which fixes bitrot from v8. No other changes.
>>>
>>> Still needs review.
>
> Based on a suggestion from Robert off-list I tried inserting into a
> delta relation from a trigger function and discovered that it
> segfaults
Fixed. Such an attempt now generates something like this:
ERROR: relation "d" cannot be the target of a modifying statement
CONTEXT: SQL statement "INSERT INTO d VALUES (100, 100, 'x')"
PL/pgSQL function level2_table_bad_usage_func() line 3 at SQL statement
New patch attached.
Miscellanea:
Do you suppose we should have all PLs that are part of the base
distro covered?
What is necessary to indicate an additional SQL feature covered?
--
Kevin Grittner
transition-v11.diff.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Mar 2, 2017 at 9:04 AM, David Steele wrote: > On 2/20/17 10:43 PM, Thomas Munro wrote: >> Based on a suggestion from Robert off-list I tried inserting into a >> delta relation from a trigger function and discovered that it >> segfaults: >> >> * frame #0: 0x0001057705a6 >> postgres`addRangeTableEntryForRelation(pstate=0x7fa58186a4d0, >> rel=0x, alias=0x, inh='\0', >> inFromCl='\0') + 70 at parse_relation.c:1280 [opt] >> frame #1: 0x00010575bbda >> postgres`setTargetTable(pstate=0x7fa58186a4d0, >> relation=0x7fa58186a098, inh=, alsoSource='\0', >> requiredPerms=1) + 90 at parse_clause.c:199 [opt] >> frame #2: 0x000105738530 postgres`transformStmt [inlined] >> transformInsertStmt(pstate=) + 69 at analyze.c:540 [opt] >> frame #3: 0x0001057384eb >> postgres`transformStmt(pstate=, parseTree=) >> + 2411 at analyze.c:279 [opt] >> frame #4: 0x000105737a42 >> postgres`transformTopLevelStmt(pstate=, >> parseTree=0x7fa58186a438) + 18 at analyze.c:192 [opt] >> frame #5: 0x0001059408d0 >> postgres`pg_analyze_and_rewrite_params(parsetree=0x7fa58186a438, >> query_string="insert into d values (100, 100, 'x')", >> parserSetup=(plpgsql.so`plpgsql_parser_setup at pl_comp.c:1017), >> parserSetupArg=0x7fa58185c2a0, queryEnv=0x7fa581857798) + 128 >> at postgres.c:706 [opt] > > Do you know when you will have a new patch ready? > > This looks like an interesting and important feature but I think it's > going to have to come together quickly if it's going to make it into v10. I hope to post a new version addressing review comments by Monday (6 March). -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Hi Kevin, On 2/20/17 10:43 PM, Thomas Munro wrote: > On Tue, Feb 21, 2017 at 7:14 AM, Thomas Munro > wrote: >> On Fri, Jan 20, 2017 at 2:49 AM, Kevin Grittner wrote: >>> Attached is v9 which fixes bitrot from v8. No other changes. >>> >>> Still needs review. > > Based on a suggestion from Robert off-list I tried inserting into a > delta relation from a trigger function and discovered that it > segfaults: > > * frame #0: 0x0001057705a6 > postgres`addRangeTableEntryForRelation(pstate=0x7fa58186a4d0, > rel=0x, alias=0x, inh='\0', > inFromCl='\0') + 70 at parse_relation.c:1280 [opt] > frame #1: 0x00010575bbda > postgres`setTargetTable(pstate=0x7fa58186a4d0, > relation=0x7fa58186a098, inh=, alsoSource='\0', > requiredPerms=1) + 90 at parse_clause.c:199 [opt] > frame #2: 0x000105738530 postgres`transformStmt [inlined] > transformInsertStmt(pstate=) + 69 at analyze.c:540 [opt] > frame #3: 0x0001057384eb > postgres`transformStmt(pstate=, parseTree=) > + 2411 at analyze.c:279 [opt] > frame #4: 0x000105737a42 > postgres`transformTopLevelStmt(pstate=, > parseTree=0x7fa58186a438) + 18 at analyze.c:192 [opt] > frame #5: 0x0001059408d0 > postgres`pg_analyze_and_rewrite_params(parsetree=0x7fa58186a438, > query_string="insert into d values (100, 100, 'x')", > parserSetup=(plpgsql.so`plpgsql_parser_setup at pl_comp.c:1017), > parserSetupArg=0x7fa58185c2a0, queryEnv=0x7fa581857798) + 128 > at postgres.c:706 [opt] Do you know when you will have a new patch ready? This looks like an interesting and important feature but I think it's going to have to come together quickly if it's going to make it into v10. Thanks, -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Feb 21, 2017 at 7:14 AM, Thomas Munro wrote: > On Fri, Jan 20, 2017 at 2:49 AM, Kevin Grittner wrote: >> Attached is v9 which fixes bitrot from v8. No other changes. >> >> Still needs review. Based on a suggestion from Robert off-list I tried inserting into a delta relation from a trigger function and discovered that it segfaults: * frame #0: 0x0001057705a6 postgres`addRangeTableEntryForRelation(pstate=0x7fa58186a4d0, rel=0x, alias=0x, inh='\0', inFromCl='\0') + 70 at parse_relation.c:1280 [opt] frame #1: 0x00010575bbda postgres`setTargetTable(pstate=0x7fa58186a4d0, relation=0x7fa58186a098, inh=, alsoSource='\0', requiredPerms=1) + 90 at parse_clause.c:199 [opt] frame #2: 0x000105738530 postgres`transformStmt [inlined] transformInsertStmt(pstate=) + 69 at analyze.c:540 [opt] frame #3: 0x0001057384eb postgres`transformStmt(pstate=, parseTree=) + 2411 at analyze.c:279 [opt] frame #4: 0x000105737a42 postgres`transformTopLevelStmt(pstate=, parseTree=0x7fa58186a438) + 18 at analyze.c:192 [opt] frame #5: 0x0001059408d0 postgres`pg_analyze_and_rewrite_params(parsetree=0x7fa58186a438, query_string="insert into d values (100, 100, 'x')", parserSetup=(plpgsql.so`plpgsql_parser_setup at pl_comp.c:1017), parserSetupArg=0x7fa58185c2a0, queryEnv=0x7fa581857798) + 128 at postgres.c:706 [opt] -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Jan 20, 2017 at 2:49 AM, Kevin Grittner wrote:
> Attached is v9 which fixes bitrot from v8. No other changes.
>
> Still needs review.
This patch still applies, builds cleanly after a small modification,
includes regression tests and the tests past. The modification I
needed to make was due to this compile error:
nodeNamedtuplestorescan.c:154:19: error: no member named
'ps_TupFromTlist' in 'struct PlanState'
scanstate->ss.ps.ps_TupFromTlist = false;
Commit ea15e18677fc2eff3135023e27f69ed8821554ed got rid of that member
of PlanState and I assume based on other changes in that commit that
the thing to do was simply to remove that line. Having done that, it
builds cleanly.
+INSERT INTO level1_table(level1_no)
+ SELECT generate_series(1,200);
+INSERT INTO level2_table(level2_no, parent_no)
+ SELECT level2_no, level2_no / 50 + 1 AS parent_no
+FROM generate_series(1,) level2_no;
+INSERT INTO all_level_status(level, node_no, status)
+ SELECT 1, level1_no, 0 FROM level1_table;
+INSERT INTO all_level_status(level, node_no, status)
+ SELECT 2, level2_no, 0 FROM level2_table;
+INSERT INTO level1_table(level1_no)
+ SELECT generate_series(201,1000);
+DELETE FROM level1_table WHERE level1_no BETWEEN 201 AND 1000;
+DELETE FROM level1_table WHERE level1_no BETWEEN 1 AND 10010;
+SELECT count(*) FROM level1_table;
+ count
+---
+ 200
+(1 row)
Was it intentional that this test doesn't include any statements that
reach the case where the trigger does RAISE EXCEPTION 'RI error'?
>From the output generated there doesn't seem to be any evidence that
the triggers run at all, though I know from playing around with this
that they do:
postgres=# delete from level1_table where level1_no = 42;
ERROR: RI error
CONTEXT: PL/pgSQL function level1_table_ri_parent_del_func() line 6 at RAISE
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
These copyright messages are missing 3 years' worth of bugfixes.
+SPI_get_caller_relation(const char *name)
Do we need this function? It's not used by the implementation. If it
does have a good use for end-users, then perhaps it should be called
something like SPI_get_registered_relation, to make it clear that it
will return whatever you registered with SPI_register_relation,
instead of introducing this 'caller' terminology?
+typedef struct NamedTuplestoreScan
+{
+ Scan scan;
+ char *enrname;
+} NamedTuplestoreScan;
Nearly plan node structs always have a comment for the members below
'scan'; I think this needs one too because 'enrname' is not
self-explanatory.
/*
+ * Capture the NEW and OLD transition TABLE tuplestores (if specified for
+ * this trigger).
+ */
+ if (trigdata->tg_newtable || trigdata->tg_oldtable)
+ {
+ estate.queryEnv = create_queryEnv();
+ if (trigdata->tg_newtable)
+ {
+ Enr enr = palloc(sizeof(EnrData));
+
+ enr->md.name = trigdata->tg_trigger->tgnewtable;
+ enr->md.tupdesc = trigdata->tg_relation->rd_att;
+ enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable);
+ enr->reldata = trigdata->tg_newtable;
+ register_enr(estate.queryEnv, enr);
+ SPI_register_relation(enr);
+ }
Why do we we have to call register_enr and also SPI_register_relation here?
On Mon, Dec 19, 2016 at 4:35 AM, Thomas Munro
wrote:
> On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner wrote:
>> There were a few changes Thomas included in the version he posted,
>> without really delving into an explanation for those changes. Some
>> or all of them are likely to be worthwhile, but I would rather
>> incorporate them based on explicit discussion, so this version
>> doesn't do much other than generalize the interface a little,
>> change some names, and add more regression tests for the new
>> feature. (The examples I worked up for the rough proof of concept
>> of enforcement of RI through set logic rather than row-at-a-time
>> navigation were the basis for the new tests, so the idea won't get
>> totally lost.) Thomas, please discuss each suggested change (e.g.,
>> the inclusion of the query environment in the parameter list of a
>> few more functions).
>
> I was looking for omissions that would cause some kind of statements
> to miss out on ENRs arbitrarily. It seemed to me that
> parse_analyze_varparams should take a QueryEnvironment, mirroring
> parse_analyze, so that PrepareQuery could pass it in. Otherwise,
> PREPARE wouldn't see ENRs. Is there any reason why SPI_prepare should
> see them but PREPARE not?
Any thoughts about that?
More soon.
--
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
Re: [HACKERS] delta relations in AFTER triggers
On Sat, Jan 21, 2017 at 6:37 AM, Kevin Grittner wrote: > On Fri, Jan 20, 2017 at 2:08 PM, Nico Williams wrote: >> On Fri, Jan 20, 2017 at 01:37:33PM -0600, Kevin Grittner wrote: > >>> There is currently plenty of room for pseudo-MV implementations, >>> and may be for a while. It's a good indication of the need for the >>> feature in core. An implementation in the guts of core can have >>> advantages that nothing else can, of course. For example, for >>> eager application of the deltas, nothing will be able to beat >>> capturing tuples already in RAM and being looked at for possible >>> trigger firing into a RAM-with-spill-to-disk tuplestore. >> >> BTW, automatic updates of certain types of MVs should be easy: add >> constraints based on NEW/OLD rows from synthetic triggers to the >> underlying query. > > Convincing me that this is a good idea for actual MVs, versus > pseudo-MVs using tables, would be an uphill battle. I recognize > the need to distinguish between MVs which contain recursive CTEs in > their definitions and MVs that don't, so that the DRed algorithm > can be used for the former and the counting algorithm for the > latter; but firing triggers for row-at-a-time maintenance is not > going to be efficient for very many cases, and the cost of > identifying those cases to handle them differently is probably > going to exceed any gains. Comparative benchmarks, once there is > an implementation using set-based techniques, could potentially > convince me; but there's not much point arguing about it before > that exists. :-) I have moved this patch to the next CF. It would be nice to progress in this topic in PG10. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Updating MATERIALIZED VIEWs (Re: [HACKERS] delta relations in AFTER triggers)
On 1/20/17 5:38 PM, Nico Williams wrote: If these triggers could be automatically generated, that sure would be nice, but some control would be needed over when to update the MV vs. mark it as needing a refresh. FWIW, pg_classy[1], which is still a work in progress, would allow for that. The idea is that you define a code template which you can then call with arguments (such as the name of a matview table), and those arguments get put into the template before executing the resulting SQL. Most of the basic framework for that is in place; I just need to finish code that will allow for the extension to track arbitrary database objects that were created. 1: https://github.com/decibel/pg_classy/blob/master/doc/pg_classy.asc -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Updating MATERIALIZED VIEWs (Re: [HACKERS] delta relations in AFTER triggers)
[Looking at your patch I see that it's not quite related to MVs, so I'm
changing the Subject. Apologies for the noise.]
[Responding out of order.]
On Fri, Jan 20, 2017 at 03:37:20PM -0600, Kevin Grittner wrote:
> On Fri, Jan 20, 2017 at 2:08 PM, Nico Williams wrote:
> > BTW, automatic updates of certain types of MVs should be easy: add
> > constraints based on NEW/OLD rows from synthetic triggers to the
> > underlying query.
>
> Convincing me that this is a good idea for actual MVs, versus
> pseudo-MVs using tables, would be an uphill battle. [...]
I don't think it's necessary, and I would not insist on it.
My alternative MV implementation lets _me_ choose when to update an MV
synchronously, and when to defer refreshes, by using [hand-coded]
triggers. This is good enough for me.
If these triggers could be automatically generated, that sure would be
nice, but some control would be needed over when to update the MV vs.
mark it as needing a refresh.
> > Our intention is to contribute this. We're willing to sign
> > reasonable contribution agreements.
>
> Posting a patch to these lists constitutes an assertion that you
> have authority to share the IP, and are doing so. Referencing a
> URL is a bit iffy, since it doesn't leave an archival copy of the
> contribution under the community's control.
Fair enough. I'll post the source file itself. I've not done the work
of properly integrating it because I need to gauge interest first,
before dedicating a lot of effort to it.
> I am dubious, though, of the approach in general, as stated above.
I'm using this _now_. With a caveat:
a) the trigger functions needed to either mark an MV as needing a
refresh, or else to update it directly, are hand-coded, and
b) I chose which operations yield synchronous MV updates and which defer
to a refresh.
The MV, in my scheme, is really just a table with triggers that update a
deltas table the same way that a refresh would. A refresh locks the
table, disables those triggers, populates another table with the current
output of the underlying view, compares to the previous materialization,
and lastly generates, records, and applies deltas to the
materialization.
To give an example, adding a user to a group -> generally fast; deleting
a user (and thus all their group memberships) -> potentially very slow.
The "add a user to a group" case can then yield near real-time updates
of external caches, while the other case results in a deferred REFRESH
so as to not slow down the current transaction. The deferred REFRESH is
not deferred too long, so the net effect is still very fast updates of
external caches.
> > However, there is a bug in the query planner that prevents this
> > from being very fast. At some point I want to tackle that bug.
>
> What bug is that?
I... asked for help on the IRC #postgresql channel. I never posted here
about it.
Revisiting it now... the main problem was query _preparation time_, not
execution time. So perhaps not so bad. Still, it's worth looking into.
The query was something like this:
SELECT v.data->'verb_name' || '^' || (r.data->'named_acl_name') AS
grant_name,
grantee.right_entity_id AS grantee_id
FROM relationships grantee
JOIN relationships grant ON
grantee.left_entity_id = grant.right_entity_id AND
grantee.relationship_type_id IN (10421, 10431, 13591, 13921)
AND grant.relationship_type_id = 10331
JOIN relationships perm_actions ON
grantee.left_entity_id = perm_actions.right_entity_id AND
perm_actions.relationship_type_id = 10381
JOIN relationships verb_in_vs ON
verb_in_vs.right_entity_id = perm_actions.left_entity_id AND
verb_in_vs.relationship_type_id = 10371
JOIN entities v ON v.id = verb_in_vs.left_entity_id
JOIN entities r ON r.id = grant.left_entity_id;
(This query uses a bit of an EAV schema. There's an "entities" table
with an hstore column for storing attributes ("data") and another
table, "relationships" that has (relationship_type_id, left_entity_id,
right_entity_id) columns and which is indexed by both, left_entity_id
and right_entity_id. EAV schemas hide relevant information from the
query planner, so there is that.)
The query plan for this is about as fast as one could hope. After all,
it has to scan many of the rows.
Now suppose we were adding a new 'grantee' and wanted to generate the
additions that would result in the MV. We could add this constraint to
the query:
WHERE grantee.left_entity_id = NEW.left_entity_id AND
grantee.right_entity_id = NEW.right_entity_id;
Now we've basically [almost] fully-specified the primary key for the
grantee table source.
The resulting query plan is actually pretty good. It has the grantee
table source as the first table source in the inner-most loop.
If I re-write the query using WITH CTEs to get a similarly good plan,
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Jan 20, 2017 at 2:08 PM, Nico Williams wrote: > On Fri, Jan 20, 2017 at 01:37:33PM -0600, Kevin Grittner wrote: >> There is currently plenty of room for pseudo-MV implementations, >> and may be for a while. It's a good indication of the need for the >> feature in core. An implementation in the guts of core can have >> advantages that nothing else can, of course. For example, for >> eager application of the deltas, nothing will be able to beat >> capturing tuples already in RAM and being looked at for possible >> trigger firing into a RAM-with-spill-to-disk tuplestore. > > BTW, automatic updates of certain types of MVs should be easy: add > constraints based on NEW/OLD rows from synthetic triggers to the > underlying query. Convincing me that this is a good idea for actual MVs, versus pseudo-MVs using tables, would be an uphill battle. I recognize the need to distinguish between MVs which contain recursive CTEs in their definitions and MVs that don't, so that the DRed algorithm can be used for the former and the counting algorithm for the latter; but firing triggers for row-at-a-time maintenance is not going to be efficient for very many cases, and the cost of identifying those cases to handle them differently is probably going to exceed any gains. Comparative benchmarks, once there is an implementation using set-based techniques, could potentially convince me; but there's not much point arguing about it before that exists. :-) > However, there is a bug in the query planner that prevents this > from being very fast. At some point I want to tackle that bug. What bug is that? > Basically, the planner does not notice that a table source in a > join has a lookup key sufficiently well-specified by those additional > constraints that it should be the first table source in the outermost > loop. Is that a description of what you see as the bug? Can you give an example, to clarify the point? I am dubious, though, of the approach in general, as stated above. >> I don't have time to review what you've done right now, but will >> save that link to look at later, if you give permission to borrow >> from it (with proper attribution, of course) if there is something >> that can advance what I'm doing. If such permission is not >> forthcoming, I will probably avoid looking at it, to avoid any >> possible copyright issues. > > Our intention is to contribute this. We're willing to sign > reasonable contribution agreements. Posting a patch to these lists constitutes an assertion that you have authority to share the IP, and are doing so. Referencing a URL is a bit iffy, since it doesn't leave an archival copy of the contribution under the community's control. > I'd appreciate a review, for sure. Thanks! Would it be possible to get your approach running using tables and/or (non-materialized) views as an extension? A trigger-based way to maintain pseudo-MVs via triggers might make an interesting extension, possibly even included in contrib if it could be shown to have advantages over built-in MVs for some non-trivial applications. > There's a gotcha w.r.t. NULL columns, but it affects the built-in > REFRESH as well, IIRC. The commentary in our implementation > discusses that in more detail. Could you report that on a new thread on the lists? I've seen comments about such a "gotcha", but am not clear on the details. It probably deserves its own thread. Once understood, we can probably fix it. Thanks! -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, Jan 20, 2017 at 01:37:33PM -0600, Kevin Grittner wrote: > On Thu, Jan 19, 2017 at 4:14 PM, Nico Williams wrote: > > Reviews welcome! I will review. > There is currently plenty of room for pseudo-MV implementations, > and may be for a while. It's a good indication of the need for the > feature in core. An implementation in the guts of core can have > advantages that nothing else can, of course. For example, for > eager application of the deltas, nothing will be able to beat > capturing tuples already in RAM and being looked at for possible > trigger firing into a RAM-with-spill-to-disk tuplestore. BTW, automatic updates of certain types of MVs should be easy: add constraints based on NEW/OLD rows from synthetic triggers to the underlying query. However, there is a bug in the query planner that prevents this from being very fast. At some point I want to tackle that bug. Basically, the planner does not notice that a table source in a join has a lookup key sufficiently well-specified by those additional constraints that it should be the first table source in the outermost loop. > I don't have time to review what you've done right now, but will > save that link to look at later, if you give permission to borrow > from it (with proper attribution, of course) if there is something > that can advance what I'm doing. If such permission is not > forthcoming, I will probably avoid looking at it, to avoid any > possible copyright issues. Our intention is to contribute this. We're willing to sign reasonable contribution agreements. I'd appreciate a review, for sure. Thanks! > > Incidentally, it's really nice that PG has some "higher order" SQL > > features that make this sort of thing easier. In particular, here, row > > values and record types, and being able to refer to a table as a column > > of the table's record type. > > Yeah, I found that quite handy in developing the REFRESH feature, > and expect to be using it in incremental maintenance. Indeed, I *copied* the pattern. However, I didn't have to generate SELECT statements that include column names, as I was able to just compare table source row values. There's a gotcha w.r.t. NULL columns, but it affects the built-in REFRESH as well, IIRC. The commentary in our implementation discusses that in more detail. Nico -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Jan 19, 2017 at 4:14 PM, Nico Williams wrote: > I hope what I've done about delta relations will be mostly irrelevant > given your patch (which I've not looked at in detail), Reviews welcome! > but just FYI, > I've built an alternate, all-SQL-coded materialized view system that > captures deltas between refreshes and deltas from direct DMLs of the > materialized view: There is currently plenty of room for pseudo-MV implementations, and may be for a while. It's a good indication of the need for the feature in core. An implementation in the guts of core can have advantages that nothing else can, of course. For example, for eager application of the deltas, nothing will be able to beat capturing tuples already in RAM and being looked at for possible trigger firing into a RAM-with-spill-to-disk tuplestore. > https://github.com/twosigma/postgresql-contrib/blob/master/pseudo_mat_views.sql > > There are some good ideas there, IMO, even if that implementation were > useless because of your patch. I don't have time to review what you've done right now, but will save that link to look at later, if you give permission to borrow from it (with proper attribution, of course) if there is something that can advance what I'm doing. If such permission is not forthcoming, I will probably avoid looking at it, to avoid any possible copyright issues. > Incidentally, it's really nice that PG has some "higher order" SQL > features that make this sort of thing easier. In particular, here, row > values and record types, and being able to refer to a table as a column > of the table's record type. Yeah, I found that quite handy in developing the REFRESH feature, and expect to be using it in incremental maintenance. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Attached is v10 which fixes bitrot from v9 caused by ea15e186. Still needs review. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company transition-v10.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sat, Dec 17, 2016 at 08:15:49PM -0600, Kevin Grittner wrote: > On Sun, Dec 4, 2016 at 11:35 PM, Haribabu Kommi > wrote: > > Moved to next CF with "waiting on author" status. > > [...] I hope what I've done about delta relations will be mostly irrelevant given your patch (which I've not looked at in detail), but just FYI, I've built an alternate, all-SQL-coded materialized view system that captures deltas between refreshes and deltas from direct DMLs of the materialized view: https://github.com/twosigma/postgresql-contrib/blob/master/pseudo_mat_views.sql There are some good ideas there, IMO, even if that implementation were useless because of your patch. Incidentally, it's really nice that PG has some "higher order" SQL features that make this sort of thing easier. In particular, here, row values and record types, and being able to refer to a table as a column of the table's record type. Nico -- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Attached is v9 which fixes bitrot from v8. No other changes. Still needs review. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company transition-v9.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner wrote: > Patch v8 ... FWIW here's that plpython patch, adjusted to apply on top of your latest patch. -- Thomas Munro http://www.enterprisedb.com delta-relations-plpython.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Dec 18, 2016 at 3:15 PM, Kevin Grittner wrote: > On Sun, Dec 4, 2016 at 11:35 PM, Haribabu Kommi > wrote: > >> Moved to next CF with "waiting on author" status. > > Patch v8 attempts to address the issues explicitly raised in > Thomas Munro's review. An opaque query environment is created > that, for now, only passes through ephemeral named relations, of > which the only initial implementation is named tuplestores; but > the techniques are intended to support both other types of > ephemeral named relations and environmental properties (things > affecting parsing, planning, and execution that are not coming from > the system catalog) besides ENRs. There is no clue in the access > to the QE whether something is, for example, stored in a list or a > hash table. That's on purpose, so that the addition of other > properties or changes to their implementation doesn't affect the > calling code. +1 > There were a few changes Thomas included in the version he posted, > without really delving into an explanation for those changes. Some > or all of them are likely to be worthwhile, but I would rather > incorporate them based on explicit discussion, so this version > doesn't do much other than generalize the interface a little, > change some names, and add more regression tests for the new > feature. (The examples I worked up for the rough proof of concept > of enforcement of RI through set logic rather than row-at-a-time > navigation were the basis for the new tests, so the idea won't get > totally lost.) Thomas, please discuss each suggested change (e.g., > the inclusion of the query environment in the parameter list of a > few more functions). I was looking for omissions that would cause some kind of statements to miss out on ENRs arbitrarily. It seemed to me that parse_analyze_varparams should take a QueryEnvironment, mirroring parse_analyze, so that PrepareQuery could pass it in. Otherwise, PREPARE wouldn't see ENRs. Is there any reason why SPI_prepare should see them but PREPARE not? Should we attempt to detect if the tupledesc changes incompatibly between planning and execution? > Changed to "Needs review" status. + enr->md.enrtuples = tuplestore_tuple_count(trigdata->tg_newtable); I was wondering about this. As I understand it, plans for statements in plpgsql functions are automatically cached. Is it OK for the number of tuples on the first invocation of the trigger in a given session to determine the estimate used for the plan? I suppose that is the case with regular tables, so maybe it is. + register_enr(estate.queryEnv, enr); + SPI_register_relation(enr); Here plpgsql calls both register_enr and SPI_register_relation. Yet SPI_register_relation calls register_enr too, so is this a mistake? Also, the return code isn't checked. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On 22 Nov. 2016 03:01, "Thomas Munro" wrote: How about a QueryEnvironment (as shown in the patch I posted) that contains a list of NamedTuplestore pointers (the SpiRelation struct in the patch I posted, renamed) and in future perhaps lists of other ephemeral/transient objects that we want to expose to SQL? Very good idea. Sooner or later someone will probably want query-scoped variables like MS-SQL and MySQL for example.
Re: [HACKERS] delta relations in AFTER triggers
On 22 Nov. 2016 01:05, "Kevin Grittner" wrote: Right, I think we should assume that there will be other ways people want to use parts of what is done here, including building tuplestores through other means and referencing them in queries. Yes. PL/pgsql table-valued variables for one thing.
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Dec 4, 2016 at 11:35 PM, Haribabu Kommi wrote: > Moved to next CF with "waiting on author" status. Patch v8 attempts to address the issues explicitly raised in Thomas Munro's review. An opaque query environment is created that, for now, only passes through ephemeral named relations, of which the only initial implementation is named tuplestores; but the techniques are intended to support both other types of ephemeral named relations and environmental properties (things affecting parsing, planning, and execution that are not coming from the system catalog) besides ENRs. There is no clue in the access to the QE whether something is, for example, stored in a list or a hash table. That's on purpose, so that the addition of other properties or changes to their implementation doesn't affect the calling code. There were a few changes Thomas included in the version he posted, without really delving into an explanation for those changes. Some or all of them are likely to be worthwhile, but I would rather incorporate them based on explicit discussion, so this version doesn't do much other than generalize the interface a little, change some names, and add more regression tests for the new feature. (The examples I worked up for the rough proof of concept of enforcement of RI through set logic rather than row-at-a-time navigation were the basis for the new tests, so the idea won't get totally lost.) Thomas, please discuss each suggested change (e.g., the inclusion of the query environment in the parameter list of a few more functions). Changed to "Needs review" status. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company transition-v8.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Wed, Nov 23, 2016 at 10:02 AM, Jim Nasby wrote: > On 11/21/16 3:49 AM, Craig Ringer wrote: > >> After going through that experience, I now agree with Kevin: an >>> > interface where a new SPI interface lets PLs push a named tuplestore >>> > into the SPI connection to make it available to SQL seems like the >>> > simplest and tidiest way. >>> >> That also offers a handy step on the path toward table-valued >> variables and pipelined functions, both of which would be _really_ >> nice for PL/PgSQL users. >> > > FWIW, I expect at some point we'd like the ability to index tuplestores as > well. Moved to next CF with "waiting on author" status. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] delta relations in AFTER triggers
On 11/21/16 3:49 AM, Craig Ringer wrote: After going through that experience, I now agree with Kevin: an > interface where a new SPI interface lets PLs push a named tuplestore > into the SPI connection to make it available to SQL seems like the > simplest and tidiest way. That also offers a handy step on the path toward table-valued variables and pipelined functions, both of which would be _really_ nice for PL/PgSQL users. FWIW, I expect at some point we'd like the ability to index tuplestores as well. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Nov 22, 2016 at 7:29 AM, Kevin Grittner wrote: > On Mon, Nov 21, 2016 at 11:29 AM, Alvaro Herrera > wrote: > >>> When I used the word "cache" here, I was thinking more of this >>> English language definition: >>> >>> a : a hiding place especially for concealing and preserving >>>provisions or implements >>> b : a secure place of storage >>> >>> The intent being to emphasize that there is not one public >>> "registry" of such objects, but context-specific collections where >>> references are tucked away when they become available for later use >>> in the only the appropriate context. > >> How about "stash"? According to my reading of Merriam-Webster's >> definition, "stash" mostly appears to be the thing that is stored >> (hidden), rather than the place it's stored in, but one of the >> definitions is "hiding place", and "cache" is listed as a synonym. > > "Stash" seems better that "cache" or "registry", especially since > many programmers these days seem to associate "cache" with > pass-through proxy techniques. I first became familiar with the > term "cache" while reading Jack London, and tend to retain some > association with the more general definition. Clearly I am in the > minority on that here. I was suggesting something like QueryEnvironment because it focuses on its role, not that fact that there are things stored in it. It's conceptually like the environment in an interpreter, which is some kind of namespace into which objects are bound by name. My suggestion "SpiRelation" now seems a bit short sighted in light of your comments, so I retract that bit. How about a QueryEnvironment (as shown in the patch I posted) that contains a list of NamedTuplestore pointers (the SpiRelation struct in the patch I posted, renamed) and in future perhaps lists of other ephemeral/transient objects that we want to expose to SQL? We would possibly have more than one list because SQL is not "Lisp-1" in nature: relations and functions and other kinds of object exist in different namespaces, though there may need to be polymorphism among kinds of named relations in the same list, so perhaps NamedTuplestore should be a node with a tag. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Nov 21, 2016 at 11:29 AM, Alvaro Herrera wrote: >> When I used the word "cache" here, I was thinking more of this >> English language definition: >> >> a : a hiding place especially for concealing and preserving >>provisions or implements >> b : a secure place of storage >> >> The intent being to emphasize that there is not one public >> "registry" of such objects, but context-specific collections where >> references are tucked away when they become available for later use >> in the only the appropriate context. > How about "stash"? According to my reading of Merriam-Webster's > definition, "stash" mostly appears to be the thing that is stored > (hidden), rather than the place it's stored in, but one of the > definitions is "hiding place", and "cache" is listed as a synonym. "Stash" seems better that "cache" or "registry", especially since many programmers these days seem to associate "cache" with pass-through proxy techniques. I first became familiar with the term "cache" while reading Jack London, and tend to retain some association with the more general definition. Clearly I am in the minority on that here. http://ereimer.net/20080706/13586_erC720.htm -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Kevin Grittner wrote: > On Mon, Nov 21, 2016 at 1:05 AM, Thomas Munro > wrote: > > Also, Tsrcache is strangely named: it's not exactly a cache, it's > > more of a registry. > > When I used the word "cache" here, I was thinking more of this > English language definition: > > a : a hiding place especially for concealing and preserving >provisions or implements > b : a secure place of storage > > The intent being to emphasize that there is not one public > "registry" of such objects, but context-specific collections where > references are tucked away when they become available for later use > in the only the appropriate context. Eventually, when these are > used for some of the less "eager" timings of materialized view > maintenance, they may be set aside for relatively extended periods > (i.e., minutes or maybe even hours) before being used. Neither > "registry" nor "cache" seems quite right; maybe someone can think > of a word with more accurate semantics. How about "stash"? According to my reading of Merriam-Webster's definition, "stash" mostly appears to be the thing that is stored (hidden), rather than the place it's stored in, but one of the definitions is "hiding place", and "cache" is listed as a synonym. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Nov 21, 2016 at 12:04 PM, Kevin Grittner wrote: >> Also, Tsrcache is strangely named: it's not exactly a cache, it's >> more of a registry. > > When I used the word "cache" here, I was thinking more of this > English language definition: > > a : a hiding place especially for concealing and preserving >provisions or implements > b : a secure place of storage > > The intent being to emphasize that there is not one public > "registry" of such objects, but context-specific collections where > references are tucked away when they become available for later use > in the only the appropriate context. Eventually, when these are > used for some of the less "eager" timings of materialized view > maintenance, they may be set aside for relatively extended periods > (i.e., minutes or maybe even hours) before being used. Neither > "registry" nor "cache" seems quite right; maybe someone can think > of a word with more accurate semantics. I complained about the use of "cache" in this name before, and I still think that it is off-base. I'm not saying there isn't some definition of the word that could cover what you're doing here, but it's not the definition that is going to pop to mind for people reading the code. I think "registry" would be OK; the fact that there is a registry does not mean it is a global registry; it can be a registry of ephemeral relations specific to that query. I'm sure there are other good choices, too, but, please, not cache! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Thanks for the review! Will respond further after reviewing your suggested patches; this is a quick response just to the contents of the email. On Mon, Nov 21, 2016 at 1:05 AM, Thomas Munro wrote: > Both ways have attracted criticism: the first involves touching > basically every core function that might eventually parse, plan or > execute a query to make it accept a Tsrcache and pass that on, and the > second involves a bunch of Rube Goldberg machine-like > callback/variable/parameter code. Just to quantify "touching basically every core function that might...", the number of functions which have a change in signature (adding one parameter) is seven. No more; no less. > I spent some time investigating whether a third way would be viable: > use ParamListInfo's setupParser hook and add an analogous one for the > executor, so that there is no registry equivalent to Tsrcache, but > also no dealing with param slots or (in plpgsql's case) new kinds of > variables. Instead, there would just be two callbacks: one for asking > the tuplestore provider for metadata (statistics, TupleDesc) at > planning time, and another for asking for the tuplestore by name at > execution time. One problem with this approach is that it requires > using the SPI_*_with_paramlist interfaces, not the more commonly used > arg-based versions, and requires using a ParamListInfo when you > otherwise have no reason to because you have no parameters. Also, > dealing with callbacks to register your tuplestore supplier is a > little clunky. More seriously, requiring providers of those callbacks > to write code that directly manipulates ParserState and EState and > calls addRangeTableEntryXXX seems like a modularity violation -- > especially for PLs that are less integrated with core Postgres code > than plpgsql. I got this more or less working, but didn't like it > much and didn't think it would pass muster. ok > After going through that experience, I now agree with Kevin: an > interface where a new SPI interface lets PLs push a named tuplestore > into the SPI connection to make it available to SQL seems like the > simplest and tidiest way. I do have some feedback and suggestions > though: > > 1. Naming: Using tuplestores in AfterTriggersData make perfect sense > to me but I don't think it follows that the exact thing we should > expose to the executor to allow these to be scanned is a > TuplestoreScan. We have other nodes that essentially scan a > tuplestore like WorkTableScan and Material but they have names that > tell you what they are for. This is for scanning data that has either > been conjured up or passed on by SPI client code and exposed to SQL > queries. How about SpiRelationScan, SpiDataScan, SpiRelVarScan, ? I think an SPI centered approach is the wrong way to go. I feel that an SPI *interface* will be very useful, and probably save thousands of lines of fragile code which would otherwise be blurring the lines of the layering, but I feel there there should be a lower-level interface, and the SPI interface should use that to provide a convenience layer. In particular, I suspect that some uses of these named tuplestore relations will initially use SPI for convenience of development, but may later move some of that code to dealing with parse trees, for performance. Ideally, the execution plan would be identical whether or not SPI was involved, so naming implying the involvement of SPI would be misleading. NamedTuplestoreScan might be an improvement over just TuplestoreScan. > Also, Tsrcache is strangely named: it's not exactly a cache, it's > more of a registry. When I used the word "cache" here, I was thinking more of this English language definition: a : a hiding place especially for concealing and preserving provisions or implements b : a secure place of storage The intent being to emphasize that there is not one public "registry" of such objects, but context-specific collections where references are tucked away when they become available for later use in the only the appropriate context. Eventually, when these are used for some of the less "eager" timings of materialized view maintenance, they may be set aside for relatively extended periods (i.e., minutes or maybe even hours) before being used. Neither "registry" nor "cache" seems quite right; maybe someone can think of a word with more accurate semantics. > 2. Scope of changes: If we're going to touch functions all over the > source tree to get the Tsrcache where it needs to be for parsing and > execution, then I wonder if we should consider thinking a bit more > about where this is going. What if we had a thing called a > QueryEnvironment, and we passed a pointer to that into to all those > places, and it could contain the named tuplestore registry? I agree. I had a version building on the Tsrcache approach which differentiated between three levels of generality: Ephemeral Relations, a subclass of that call Named Ephemeral Re
Re: [HACKERS] delta relations in AFTER triggers
On 21 November 2016 at 15:05, Thomas Munro wrote: > After going through that experience, I now agree with Kevin: an > interface where a new SPI interface lets PLs push a named tuplestore > into the SPI connection to make it available to SQL seems like the > simplest and tidiest way. That also offers a handy step on the path toward table-valued variables and pipelined functions, both of which would be _really_ nice for PL/PgSQL users. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Oct 30, 2016 at 10:35 AM, Kevin Grittner wrote: > On Sun, Oct 2, 2016 at 11:20 PM, Michael Paquier > wrote: >> Not as big as I thought, only 2k when both patches are combined... The >> patch without noapi in its name needs to be applied first, and after >> the patch with noapi can be applied. >> 60 files changed, 2073 insertions(+), 63 deletions(-) >> Moved to next CF. > > In an attempt to make this patch more digestible for reviewers, I > have split it up as follows: > > transition-c-triggers-only-v7.diff > 17 files changed, 581 insertions(+), 47 deletions(-) > > This part is virtually unchanged (just curing bit-rot) since > August, 2014, when I believe I had addressed all issues raised by > reviewers. It does provide a barely usable feature, since the > syntax for transition tables is added and tuplestores are created > when needed (and only when needed), with references stored in the > TriggerData structure. No new execution nodes are provided, so > only C triggers can use these relations, and must explicitly and > directly access the tuplestores from within the C code -- there is > no support for referencing these tuplestores from within queries. > > This is basic infrastructure needed for the more complete feature. > As far as I know there are no objections to what is implemented > here. I have pulled it out to make the review of the more > controversial portions easier. Since it had quite a bit of review > two years ago, I will do some testing to make sure that nothing has > broken and then push this part in a few days if there are no > objections. Hearing none, done. Hopefully that makes what remains easier to review. During final testing I was annoyed by the thin support for CREATE TRIGGER in the tab completion code, so I improved that a bit and pushed that, too. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Wed, Nov 2, 2016 at 4:09 PM, Adam Brusselback wrote: >> There may be some situations where crawling the indexes a row at a >> time will perform better than this by enough to want to retain that >> option. > > If an index existed, wouldn't it still be able to use that in the set-based > implementation? Yes. The optimizer would compare plans and pick the lowest cost. > Is there something which would make doing the check > set-based ever worse than row based inherently? I'm not sure. I doubt that it would ever lose by very much, but only benchmarking can really answer that question. Anyway, it's probably premature to get too far into it now. It just occurred to me that it might be a worthwhile project once the transition tables are available, so I did a quick set of triggers to see what the potential was in a "best case" scenario. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
> > There may be some situations where crawling the indexes a row at a > time will perform better than this by enough to want to retain that > option. If an index existed, wouldn't it still be able to use that in the set-based implementation? Is there something which would make doing the check set-based ever worse than row based inherently?
Re: [HACKERS] delta relations in AFTER triggers
> 2016-11-02 15:57 GMT+01:00 Kevin Grittner : >> On Sun, Oct 30, 2016 at 10:35 AM, Kevin Grittner wrote: >> >>> SPI support would also >>> allow us to consider using set logic for validating foreign keys, >>> instead of the one-row-at-a-time approach currently used. >> >> Just as a proof of concept for this I used the attached test case >> to create foreign keys using current techniques versus set-oriented >> queries with the transition-tsr code. These probably can be >> improved, since this is a "first cut" off the top of my head. >> >> The delete of about one million rows from a "parent" table with no >> matching rows in the "child" table, and no index on referencing >> column in the child table, took 24:17.969 using current triggers >> and 00:03.262 using the set-based triggers. Yes, that reduces >> current run time for that case by 99.78% On Wed, Nov 2, 2016 at 11:07 AM, Pavel Stehule wrote: > > this is great number On Wed, Nov 2, 2016 at 11:41 AM, Adam Brusselback wrote: > > That is really incredible. Gets rid of the need for an index on referencing > columns for a ton of use cases. Keep in mind that this is just a quick proof of concept. Unless all participating transactions are at serializable isolation level something would need to be done to handle race conditions, and that might affect performance. I do think that this approach is likely to be better in enough circumstances, even after that is covered, that it will be worth pursuing -- either as an option when declaring a foreign key, or as the only implementation. Until we have a version that covers the race conditions and benchmark it in a variety of workloads, it is hard to feel sure about the latter. There may be some situations where crawling the indexes a row at a time will perform better than this by enough to want to retain that option. A big plus of a single set-oriented statement is that it doesn't suck unlimited RAM -- it will use work_mem to limit each tuplestore and each query node, spilling to disk if needed. The current FK implementation sometimes runs for a very long time and can run people out of memory. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
> > The delete of about one million rows from a "parent" table with no > matching rows in the "child" table, and no index on referencing > column in the child table, took 24:17.969 using current triggers > and 00:03.262 using the set-based triggers. Yes, that reduces > current run time for that case by 99.78% That is really incredible. Gets rid of the need for an index on referencing columns for a ton of use cases.
Re: [HACKERS] delta relations in AFTER triggers
2016-11-02 15:57 GMT+01:00 Kevin Grittner : > On Sun, Oct 30, 2016 at 10:35 AM, Kevin Grittner > wrote: > > > SPI support would also > > allow us to consider using set logic for validating foreign keys, > > instead of the one-row-at-a-time approach currently used. > > Just as a proof of concept for this I used the attached test case > to create foreign keys using current techniques versus set-oriented > queries with the transition-tsr code. These probably can be > improved, since this is a "first cut" off the top of my head. > > The delete of about one million rows from a "parent" table with no > matching rows in the "child" table, and no index on referencing > column in the child table, took 24:17.969 using current triggers > and 00:03.262 using the set-based triggers. Yes, that reduces > current run time for that case by 99.78% > this is great number Pavel > > -- > Kevin Grittner > EDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > >
Re: [HACKERS] delta relations in AFTER triggers
Attached is a minor fix to go on top of transition-tsr for issues
found yesterday in testing.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimizer/path/costsize.c
index 53bfd4b..2da841e 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4512,7 +4512,7 @@ set_cte_size_estimates(PlannerInfo *root, RelOptInfo
*rel, double cte_rows)
void
set_tuplestore_size_estimates(PlannerInfo *root, RelOptInfo *rel)
{
- RangeTblEntry *rte;
+ RangeTblEntry *rte PG_USED_FOR_ASSERTS_ONLY;
/* Should only be applied to base relations that are tuplestore
references */
Assert(rel->relid > 0);
diff --git a/src/backend/parser/parse_relation.c
b/src/backend/parser/parse_relation.c
index 7c943c3..0b45139 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -1160,12 +1160,17 @@ parserOpenTable(ParseState *pstate, const RangeVar
*relation, int lockmode)
else
{
/*
+* An unqualified name might be a tuplestore relation
name.
+*/
+ if (get_visible_tuplestore_metadata(pstate->p_tsrcache,
relation->relname))
+ rel = NULL;
+ /*
* An unqualified name might have been meant as a
reference to
* some not-yet-in-scope CTE. The bare "does not
exist" message
* has proven remarkably unhelpful for figuring out
such problems,
* so we take pains to offer a specific hint.
*/
- if (isFutureCTE(pstate, relation->relname))
+ else if (isFutureCTE(pstate, relation->relname))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_TABLE),
errmsg("relation \"%s\" does
not exist",
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index a9f5d6e..57906c0 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -158,7 +158,7 @@ struct ParseState
boolp_locked_from_parent;
Relationp_target_relation;
RangeTblEntry *p_target_rangetblentry;
- Tsrcache*p_tsrcache;/* visible named tuplestore relations */
+ Tsrcache *p_tsrcache; /* visible named tuplestore relations */
/*
* Optional hook functions for parser callbacks. These are null unless
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Oct 30, 2016 at 10:35 AM, Kevin Grittner wrote: > SPI support would also > allow us to consider using set logic for validating foreign keys, > instead of the one-row-at-a-time approach currently used. Just as a proof of concept for this I used the attached test case to create foreign keys using current techniques versus set-oriented queries with the transition-tsr code. These probably can be improved, since this is a "first cut" off the top of my head. The delete of about one million rows from a "parent" table with no matching rows in the "child" table, and no index on referencing column in the child table, took 24:17.969 using current triggers and 00:03.262 using the set-based triggers. Yes, that reduces current run time for that case by 99.78% -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ri-set-logic.sql Description: application/sql -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Sun, Oct 2, 2016 at 11:20 PM, Michael Paquier wrote: > On Sat, Sep 10, 2016 at 7:28 AM, Kevin Grittner wrote: >> v6 fixes recently-introduced bit-rot. > > Not as big as I thought, only 2k when both patches are combined... The > patch without noapi in its name needs to be applied first, and after > the patch with noapi can be applied. > 60 files changed, 2073 insertions(+), 63 deletions(-) > Moved to next CF. In an attempt to make this patch more digestible for reviewers, I have split it up as follows: transition-c-triggers-only-v7.diff contrib/pgstattuple/pgstattuple.c| 2 + doc/src/sgml/catalogs.sgml | 16 ++ doc/src/sgml/ref/create_trigger.sgml | 94 +-- src/backend/commands/tablecmds.c | 5 +- src/backend/commands/trigger.c | 327 - src/backend/nodes/copyfuncs.c| 16 ++ src/backend/nodes/equalfuncs.c | 14 ++ src/backend/nodes/outfuncs.c | 13 + src/backend/parser/gram.y| 70 +- src/backend/utils/adt/ruleutils.c| 23 ++ src/bin/pg_basebackup/walmethods.c | 5 - src/include/catalog/pg_trigger.h | 13 +- src/include/commands/trigger.h | 2 + src/include/nodes/nodes.h| 1 + src/include/nodes/parsenodes.h | 17 ++ src/include/parser/kwlist.h | 3 + src/include/utils/reltrigger.h | 7 + 17 files changed, 581 insertions(+), 47 deletions(-) This part is virtually unchanged (just curing bit-rot) since August, 2014, when I believe I had addressed all issues raised by reviewers. It does provide a barely usable feature, since the syntax for transition tables is added and tuplestores are created when needed (and only when needed), with references stored in the TriggerData structure. No new execution nodes are provided, so only C triggers can use these relations, and must explicitly and directly access the tuplestores from within the C code -- there is no support for referencing these tuplestores from within queries. This is basic infrastructure needed for the more complete feature. As far as I know there are no objections to what is implemented here. I have pulled it out to make the review of the more controversial portions easier. Since it had quite a bit of review two years ago, I will do some testing to make sure that nothing has broken and then push this part in a few days if there are no objections. transition-noapi.diff contrib/pgstattuple/pgstattuple.c | 2 - doc/src/sgml/spi.sgml | 279 src/backend/commands/explain.c| 10 + src/backend/executor/Makefile | 3 +- src/backend/executor/execAmi.c| 6 + src/backend/executor/execProcnode.c | 14 + src/backend/executor/nodeTuplestorescan.c | 200 ++ src/backend/nodes/copyfuncs.c | 25 ++ src/backend/nodes/nodeFuncs.c | 2 + src/backend/nodes/outfuncs.c | 20 ++ src/backend/nodes/print.c | 4 + src/backend/nodes/readfuncs.c | 7 + src/backend/optimizer/path/allpaths.c | 44 +++ src/backend/optimizer/path/costsize.c | 66 + src/backend/optimizer/plan/createplan.c | 71 + src/backend/optimizer/plan/setrefs.c | 11 + src/backend/optimizer/plan/subselect.c| 5 + src/backend/optimizer/prep/prepjointree.c | 2 + src/backend/optimizer/util/pathnode.c | 25 ++ src/backend/optimizer/util/plancat.c | 4 +- src/backend/optimizer/util/relnode.c | 1 + src/backend/parser/Makefile | 3 +- src/backend/parser/analyze.c | 11 + src/backend/parser/parse_clause.c | 23 +- src/backend/parser/parse_node.c | 2 + src/backend/parser/parse_relation.c | 115 +++- src/backend/parser/parse_target.c | 2 + src/backend/parser/parse_tuplestore.c | 34 +++ src/backend/tcop/utility.c| 3 +- src/backend/utils/adt/ruleutils.c | 1 + src/backend/utils/cache/Makefile | 2 +- src/backend/utils/cache/tsrcache.c| 111 src/bin/pg_basebackup/walmethods.c| 5 + src/include/executor/nodeTuplestorescan.h | 24 ++ src/include/nodes/execnodes.h | 18 ++ src/include/nodes/nodes.h | 2 + src/include/nodes/parsenodes.h| 11 +- src/include/nodes/plannodes.h | 10 + src/include/optimizer/cost.h | 3 + src/include/optimizer/pathnode.h | 2 + src/include/parser/parse_node.h | 2 + src/include/parser/parse_relation.h | 4 + src/include/parser/parse_tuplestore.h | 24 ++ src/include/utils/tsrcache.h | 27 ++ src/include/utils/tsrmd.h | 29 ++ src/include/utils/tsrmdcache.h| 31 +++ src/include/utils/tuplestore.h| 14 + src/pl/plpgsql/src/pl_comp.c | 13 +- src/pl/plpgsql/src/pl_exec.c
Re: [HACKERS] delta relations in AFTER triggers
On Sat, Sep 10, 2016 at 7:28 AM, Kevin Grittner wrote: > v6 fixes recently-introduced bit-rot. Not as big as I thought, only 2k when both patches are combined... The patch without noapi in its name needs to be applied first, and after the patch with noapi can be applied. 60 files changed, 2073 insertions(+), 63 deletions(-) Moved to next CF. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
v6 fixes recently-introduced bit-rot. Kevin Grittner On Wed, Aug 31, 2016 at 3:24 PM, Kevin Grittner wrote: > I have merged in the changes since v4 (a year and a half ago) and > cured all bit-rot I found, to get the attached v5 which runs `make > check world` without problem -- including the tests added for this > feature. > > I did remove the contrib code that David Fetter wrote to > demonstrate the correctness and performance of the tuplestores as > created during the transaction, and how to use them directly from C > code, before any API code was written. If we want that to be > committed, it should be considered separately after the main > feature is in. > > Thanks to Thomas Munro who took a look at v4 and pointed out a bug > (which is fixed in v5) and suggested a way forward for using the > parameters. Initial attempts to get that working were not > successful,, but I think it is fundamentally the right course, > should we reach a consensus to go that way, > > On Thu, Jul 7, 2016 at 5:07 PM, Robert Haas wrote: >> On Fri, May 13, 2016 at 2:37 PM, Kevin Grittner wrote: >>> On Fri, May 13, 2016 at 1:02 PM, David Fetter wrote: On Thu, Jan 22, 2015 at 02:41:42PM +, Kevin Grittner wrote: >>> > [ideas on how to pass around references to ephemeral relations] [almost 17 months later] It seems like now is getting close to the time to get this into master. The patch might have suffered from some bit rot, but the design so far seems sound. What say? >>> >>> I had a talk with Tom in Brussels about this. As I understood it, >>> he wasn't too keen on the suggestion by Heikki (vaguely >>> sorta-endorsed by Robert) of passing ephemeral named relations such >>> as these tuplestores around in the structures currently used for >>> parameter values. He intuitively foresaw the types of problems I >>> had run into trying to use the same structure to pass a relation >>> (with structure and rows and columns of data) as is used to pass, >>> say, an integer. >> >> See, the thing that disappoints me about this is that I think we were >> pretty closed to having the ParamListInfo-based approach working. > > Maybe, but the thing I would like to do before proceeding down that > road is to confirm that we have a consensus that such a course is > better than what Is on the branch which is currently working. If > that's the consensus here, I'll work on that for the next CF. If > not, there may not be a lot left to do before commit. (Notably, we > may want to provide a way to free a tuplestore pointer when done > with it, but that's not too much work.) Let me describe the API I > have working. > > There are 11 function prototypes modified under src/include, in all > cases to add a Tsrcache parameter: > 1 createas.h > 3 explain.h > 1 prepare.h > 1 analyze.h > 2 tcopprot.h > 3 utility.h > > There are three new function prototypes in SPI. NOTE: This does > *not* mean that SPI is required to use named tuplestores in > queries, just that there are convenience functions for any queries > being run through SPI, so that any code using SPI (including any > PLs that do) will find assigning a name to a tuplestore and > referencing that within a query about as easy as falling off a log. > A tuplestore is registered to the current SPI context and not > visible outside that context. This results in a Tsrcache being > passed to the functions mentioned above when that context is > active, just as any non-SPI code could do. > >> The thing I liked about that approach is that we already know that any >> place where you can provide parameters for a query, there will also be >> an opportunity to provide a ParamListInfo. And I think that >> parameterizing a query by an ephemeral table is conceptually similar >> to parameterizing it by a scalar value. If we invent a new mechanism, >> it's got to reach all of those same places in the code. > > Do you see someplace that the patch missed? > >> One other comment that I would make here is that I think that it's >> important, however we pass the data, that the scope of the tuplestores >> is firmly lexical and not dynamic. We need to make sure that we don't >> set some sort of context via global variables that might get used for >> some query other than the one to which it's intended to apply. > > Is this based on anything actually in the patch? > > > For this CF, the main patch attached is a working version of the > feature that people can test, review documentation, etc. Any API > changes are not expected to change these visible behaviors, so any > feedback on usability or documentation can be directly useful > regardless of the API discussion. > > I have also attached a smaller patch which applies on top of the > main one which rips out the Tsrcache API to get to a "no API" > version that compiles cleanly and runs fine as long as you don't > try to use the feature, in which case it will not recognize the > tuplestore names and will give th
Re: [HACKERS] delta relations in AFTER triggers
I have merged in the changes since v4 (a year and a half ago) and cured all bit-rot I found, to get the attached v5 which runs `make check world` without problem -- including the tests added for this feature. I did remove the contrib code that David Fetter wrote to demonstrate the correctness and performance of the tuplestores as created during the transaction, and how to use them directly from C code, before any API code was written. If we want that to be committed, it should be considered separately after the main feature is in. Thanks to Thomas Munro who took a look at v4 and pointed out a bug (which is fixed in v5) and suggested a way forward for using the parameters. Initial attempts to get that working were not successful,, but I think it is fundamentally the right course, should we reach a consensus to go that way, On Thu, Jul 7, 2016 at 5:07 PM, Robert Haas wrote: > On Fri, May 13, 2016 at 2:37 PM, Kevin Grittner wrote: >> On Fri, May 13, 2016 at 1:02 PM, David Fetter wrote: >>> On Thu, Jan 22, 2015 at 02:41:42PM +, Kevin Grittner wrote: >> [ideas on how to pass around references to ephemeral relations] >>> >>> [almost 17 months later] >>> >>> It seems like now is getting close to the time to get this into >>> master. The patch might have suffered from some bit rot, but the >>> design so far seems sound. >>> >>> What say? >> >> I had a talk with Tom in Brussels about this. As I understood it, >> he wasn't too keen on the suggestion by Heikki (vaguely >> sorta-endorsed by Robert) of passing ephemeral named relations such >> as these tuplestores around in the structures currently used for >> parameter values. He intuitively foresaw the types of problems I >> had run into trying to use the same structure to pass a relation >> (with structure and rows and columns of data) as is used to pass, >> say, an integer. > > See, the thing that disappoints me about this is that I think we were > pretty closed to having the ParamListInfo-based approach working. Maybe, but the thing I would like to do before proceeding down that road is to confirm that we have a consensus that such a course is better than what Is on the branch which is currently working. If that's the consensus here, I'll work on that for the next CF. If not, there may not be a lot left to do before commit. (Notably, we may want to provide a way to free a tuplestore pointer when done with it, but that's not too much work.) Let me describe the API I have working. There are 11 function prototypes modified under src/include, in all cases to add a Tsrcache parameter: 1 createas.h 3 explain.h 1 prepare.h 1 analyze.h 2 tcopprot.h 3 utility.h There are three new function prototypes in SPI. NOTE: This does *not* mean that SPI is required to use named tuplestores in queries, just that there are convenience functions for any queries being run through SPI, so that any code using SPI (including any PLs that do) will find assigning a name to a tuplestore and referencing that within a query about as easy as falling off a log. A tuplestore is registered to the current SPI context and not visible outside that context. This results in a Tsrcache being passed to the functions mentioned above when that context is active, just as any non-SPI code could do. > The thing I liked about that approach is that we already know that any > place where you can provide parameters for a query, there will also be > an opportunity to provide a ParamListInfo. And I think that > parameterizing a query by an ephemeral table is conceptually similar > to parameterizing it by a scalar value. If we invent a new mechanism, > it's got to reach all of those same places in the code. Do you see someplace that the patch missed? > One other comment that I would make here is that I think that it's > important, however we pass the data, that the scope of the tuplestores > is firmly lexical and not dynamic. We need to make sure that we don't > set some sort of context via global variables that might get used for > some query other than the one to which it's intended to apply. Is this based on anything actually in the patch? For this CF, the main patch attached is a working version of the feature that people can test, review documentation, etc. Any API changes are not expected to change these visible behaviors, so any feedback on usability or documentation can be directly useful regardless of the API discussion. I have also attached a smaller patch which applies on top of the main one which rips out the Tsrcache API to get to a "no API" version that compiles cleanly and runs fine as long as you don't try to use the feature, in which case it will not recognize the tuplestore names and will give this message: "executor could not find named tuplestore \"%s\"". There may be a little bit left to rip out when adding a parameter-based API, but this is basically where we're moving from if we go that way. I include it both to help isolate the API we're discussing an
Re: [HACKERS] delta relations in AFTER triggers
On Fri, May 13, 2016 at 2:37 PM, Kevin Grittner wrote: > On Fri, May 13, 2016 at 1:02 PM, David Fetter wrote: >> On Thu, Jan 22, 2015 at 02:41:42PM +, Kevin Grittner wrote: > >>> [ideas on how to pass around references to ephemeral relations] >> >> [almost 17 months later] >> >> It seems like now is getting close to the time to get this into >> master. The patch might have suffered from some bit rot, but the >> design so far seems sound. >> >> What say? > > I had a talk with Tom in Brussels about this. As I understood it, > he wasn't too keen on the suggestion by Heikki (vaguely > sorta-endorsed by Robert) of passing ephemeral named relations such > as these tuplestores around in the structures currently used for > parameter values. He intuitively foresaw the types of problems I > had run into trying to use the same structure to pass a relation > (with structure and rows and columns of data) as is used to pass, > say, an integer. See, the thing that disappoints me about this is that I think we were pretty closed to having the ParamListInfo-based approach working. The thing I liked about that approach is that we already know that any place where you can provide parameters for a query, there will also be an opportunity to provide a ParamListInfo. And I think that parameterizing a query by an ephemeral table is conceptually similar to parameterizing it by a scalar value. If we invent a new mechanism, it's got to reach all of those same places in the code. One other comment that I would make here is that I think that it's important, however we pass the data, that the scope of the tuplestores is firmly lexical and not dynamic. We need to make sure that we don't set some sort of context via global variables that might get used for some query other than the one to which it's intended to apply. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Fri, May 13, 2016 at 1:02 PM, David Fetter wrote: > On Thu, Jan 22, 2015 at 02:41:42PM +, Kevin Grittner wrote: >> [ideas on how to pass around references to ephemeral relations] > > [almost 17 months later] > > It seems like now is getting close to the time to get this into > master. The patch might have suffered from some bit rot, but the > design so far seems sound. > > What say? I had a talk with Tom in Brussels about this. As I understood it, he wasn't too keen on the suggestion by Heikki (vaguely sorta-endorsed by Robert) of passing ephemeral named relations such as these tuplestores around in the structures currently used for parameter values. He intuitively foresaw the types of problems I had run into trying to use the same structure to pass a relation (with structure and rows and columns of data) as is used to pass, say, an integer. After discussing a while, he suggested that this patch should be looked at as an opportunity to refactor the existing handling of the data used by AFTER triggers. He pointed out that the existing technique is unbounded in RAM use, with no ability to spill to temporary files, and regularly generates complaints. This patch is putting all that same data into a pair of tuplestores (for old and new row versions) that would spill to disk as needed and would likely perform better. I think that still leaves the question open of how best to pass around information about ephemeral relations. It seems to me that threads about other features have brushed up against similar needs, and we should consider those in conjunction with this to make sure we solve the problem once in a sufficiently general way to cover all the needs. (For example, the "asynchronous and vectorized execution" brushes against related concepts, and I seem to recall others.) Unfortunately for those eager to have incremental maintenance of materialized views (for which this patch was a milestone on the way), Tom's suggestions seem to put additional work on that path. When I pointed that out, he pointed out that doing features well can take a lot of time. I think that was about the time the next round of drinks arrived and we decided we'd better not try to take the discussion much further that night. ;-) I welcome any other thoughts on this, particularly from Tom (and *especially* if I've misrepresented his position in any way!). Perhaps we can get a design we all like and split the work so that the refactoring Tom wants and the feature this patch is intended to implement can get done in the next CF or two, and some initial work on IMMV can still make it into the next development cycle, building on this. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Jan 22, 2015 at 02:41:42PM +, Kevin Grittner wrote:
> Robert Haas wrote:
>
> > Another idea is to change what actually gets passed to the parser
> > callback. Right now we just pass the PLpgSQL_expr. If we created a
> > new structure that contained that plus the PLpgSQL_execstate, we'd be
> > in fine shape. But this sort of gets at the root of the problem here:
> > with variables, the parser callback doesn't return the actual *value*,
> > it returns a Param node that will later, at execution time, be looked
> > up to find the actual value. With relations, we're sort of doing the
> > same thing - the tuplestore RTE doesn't need to contain the actual
> > data, just the tuple descriptor. But the data structures from which
> > we can get that information also contain the actual execution-time
> > information, so passing them into parse-time callbacks seems like it
> > might be, if nothing else, a recipe for future bugs.
>
> That was, of course, why this patch evolved to using this structure
> during parsing:
>
> typedef struct TsrmdData
> {
> char *name; /* name used to identify the
> tuplestore */
> TupleDesc tupdesc;/* description of result rows */
> } TsrmdData;
>
> typedef TsrmdData *Tsrmd;
>
> ... and this during execution:
>
> typedef struct TsrData
> {
> TsrmdData md;
> Tuplestorestate*tstate; /* data (or tids) */
> } TsrData;
>
> typedef TsrData *Tsr;
>
> The big problem, as I see it, is how to deliver these to where they
> are needed. I didn't think it was that hard to do with the parser
> hook; it's what to do to get the execution time structure to where
> it's needed that I can't figure out. Passing it with the
> parameters is tricky because we're often passing a NULL for the
> reference to the parameter list when we need these. Trying to coax
> the executor to pass in a parameter list when there are no
> parameters, just these ephemeral relations, seems very tricky and
> all solutions I have tried (other than the one Heikki and others
> have objected to) very fragile.
>
> In short, the only solution which I've been able to come up with
> that works (and seems to me solid enough to commit) is the one that
> Hekki, Tom, and Robert seem to think should be made more like
> parameter handling; and every attempt at handling these relations
> like parameters seems to me too fragile for me to feel it is worthy
> of commit.
>
> We're really down to the wire on getting this feature into 9.5; and
> we're way past what was initially my goal, which was to build on
> this to get some incremental maintenance of (some types of simple)
> materialized views into 9.5. IMO we need to be able to build up
> tuplestores and easily reference them from within complex queries
> to create any sane and efficient implementation of incremental
> maintenance of materialized views. This patch was mainly intended
> to make progress on the MV front, with an incidental benefit of
> providing a standard feature that I have seen requested a few times.
[almost 17 months later]
It seems like now is getting close to the time to get this into
master. The patch might have suffered from some bit rot, but the
design so far seems sound.
What say?
Cheers,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fet...@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Robert Haas wrote:
> Another idea is to change what actually gets passed to the parser
> callback. Right now we just pass the PLpgSQL_expr. If we created a
> new structure that contained that plus the PLpgSQL_execstate, we'd be
> in fine shape. But this sort of gets at the root of the problem here:
> with variables, the parser callback doesn't return the actual *value*,
> it returns a Param node that will later, at execution time, be looked
> up to find the actual value. With relations, we're sort of doing the
> same thing - the tuplestore RTE doesn't need to contain the actual
> data, just the tuple descriptor. But the data structures from which
> we can get that information also contain the actual execution-time
> information, so passing them into parse-time callbacks seems like it
> might be, if nothing else, a recipe for future bugs.
That was, of course, why this patch evolved to using this structure
during parsing:
typedef struct TsrmdData
{
char *name; /* name used to identify the
tuplestore */
TupleDesc tupdesc;/* description of result rows */
} TsrmdData;
typedef TsrmdData *Tsrmd;
... and this during execution:
typedef struct TsrData
{
TsrmdData md;
Tuplestorestate*tstate; /* data (or tids) */
} TsrData;
typedef TsrData *Tsr;
The big problem, as I see it, is how to deliver these to where they
are needed. I didn't think it was that hard to do with the parser
hook; it's what to do to get the execution time structure to where
it's needed that I can't figure out. Passing it with the
parameters is tricky because we're often passing a NULL for the
reference to the parameter list when we need these. Trying to coax
the executor to pass in a parameter list when there are no
parameters, just these ephemeral relations, seems very tricky and
all solutions I have tried (other than the one Heikki and others
have objected to) very fragile.
In short, the only solution which I've been able to come up with
that works (and seems to me solid enough to commit) is the one that
Hekki, Tom, and Robert seem to think should be made more like
parameter handling; and every attempt at handling these relations
like parameters seems to me too fragile for me to feel it is worthy
of commit.
We're really down to the wire on getting this feature into 9.5; and
we're way past what was initially my goal, which was to build on
this to get some incremental maintenance of (some types of simple)
materialized views into 9.5. IMO we need to be able to build up
tuplestores and easily reference them from within complex queries
to create any sane and efficient implementation of incremental
maintenance of materialized views. This patch was mainly intended
to make progress on the MV front, with an incidental benefit of
providing a standard feature that I have seen requested a few times.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Oct 23, 2014 at 11:19 AM, Robert Haas wrote: > So what I'm imagining now is: > > 1. During parse analysis, p_tableref_hook gets control and calls > addRangeTableEntryForTuplestore(), creating an RTE of type > RTE_TUPLESTORE. The RTE stores an integer parameter-index. > > 2. Path generation doesn't need to do anything very exciting; it just > generates a Path node of type T_TuplestoreScan. The RTE is still > available, so the path itself doesn't need to know which tuplestore > we're referencing, because that information is present in the RTE. > > 3. At plan generation time, we look up the RTE for the path and > extract the parameter index, which is what gets stored in the > TuplestoreScan node. > > 4. At executor initialization time, we use the parameter index in the > TuplestoreScan to index into the EState's es_param_list_info and > retrieve the tuplestore. I spent some time poking at this yesterday, based on commit 5060b9352b0d0301ffb002355f0572e93f8b05fe from https://github.com/kgrittn/postgres.git Here's where I got stuck: The plpgsql_parser_setup() callback sets pstate->p_ref_hook_state = (void *) expr, so if we add p_tableref_hook as an additional callback, that's what it has to work with to find the information needed to generate a RangeTblEntry. That is a PLpgSQL_expr, and it contains a pointer to the PLpgSQL_function, which is created when the function is compiled, which seems good, but the information we need is not there. Specifically, we need to know the names the user picked for the old and new tuplestores (tgoldtable and tgnewtable) and we need to know what the tuple descriptor should be, and the PLpgSQL_function hasn't got it. It does not seem impossible to fix that, but I'm not sure it's safe. do_compile() has the FunctionCallInfo, so from there it can get at the TriggerData and the Trigger. The trigger has got tgoldtable and tgnewtable, and the TriggerData has got tg_relation, so everything we need is there. We could add pointers to the relevant stuff to the PLpgSQL_function, and then the parser callbacks could get at it. However, I'm not sure that's really OK -- presumably, tg_relation is going to be valid only during the initial compile. If somebody came back and looked at that PLpgSQL_function again later, and tried to follow that pointer, bad things would happen. In practice maybe it would be OK because the only likely reason to come back and look at the PLpgSQL_function again is because we're recompiling, and at that point we'd have a new relation pointer to copy in there, and so it would probably be OK. But that feels mighty ugly. Another idea is to change what actually gets passed to the parser callback. Right now we just pass the PLpgSQL_expr. If we created a new structure that contained that plus the PLpgSQL_execstate, we'd be in fine shape. But this sort of gets at the root of the problem here: with variables, the parser callback doesn't return the actual *value*, it returns a Param node that will later, at execution time, be looked up to find the actual value. With relations, we're sort of doing the same thing - the tuplestore RTE doesn't need to contain the actual data, just the tuple descriptor. But the data structures from which we can get that information also contain the actual execution-time information, so passing them into parse-time callbacks seems like it might be, if nothing else, a recipe for future bugs. Any suggestions? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Wed, Oct 22, 2014 at 5:29 PM, Tom Lane wrote: >> I was thinking that the hook would return a RelationParam. When parse >> analysis sees the returned RelationParam, it adds an entry for that to >> the range table, and creates the RangeTblRef for it. The way you >> describe it works too, but manipulating the range table directly in the >> hook seems a bit too low-level. > > The problem with that idea is that then the API for the hook has to cover > every possible sort of RTE that hooks might wish to create; I see no > reason to restrict them to creating just one kind. I agree that the hook > should avoid *physically* manipulating the rangetable, but it seems > reasonable to expect that it can call one of the addRangeTableEntryXXX > functions provided by parse_relation.c, and then return a RangeTblEntry* > gotten that way. So hooks would have an API more or less equivalent > to, eg, transformRangeFunction(). Right, that reasoning makes sense to me. Unlike regular parameters, where the existence of the parameter is known at parse time but the value isn't available until bind time, we would be creating a RelationParam node and then, literally immediately, turning it into a range-table entry. That seems like unnecessary complexity, and it's also something we can invent later if a more compelling use case emerges. So what I'm imagining now is: 1. During parse analysis, p_tableref_hook gets control and calls addRangeTableEntryForTuplestore(), creating an RTE of type RTE_TUPLESTORE. The RTE stores an integer parameter-index. 2. Path generation doesn't need to do anything very exciting; it just generates a Path node of type T_TuplestoreScan. The RTE is still available, so the path itself doesn't need to know which tuplestore we're referencing, because that information is present in the RTE. 3. At plan generation time, we look up the RTE for the path and extract the parameter index, which is what gets stored in the TuplestoreScan node. 4. At executor initialization time, we use the parameter index in the TuplestoreScan to index into the EState's es_param_list_info and retrieve the tuplestore. This means that Kevin's notion of a Tsrcache goes away completely, which means a lot of the function-signature changes in his last version of the patch can be reverted. The EState doesn't need a es_tsrcache either. The mapping from name (OLD/NEW) to parameter index happens inside the p_paramref_hook and after that we use integer indices throughout. All that sees good. One thing that's not too clear to me is how we're imagining that the TuplestoreScan will get it's tupledesc. Right now the Tsrcache stores essentially (tupledesc, tuplestore), but I understood the suggestions above to imply that the ParamListInfo should point only to the tuplestore, not to the tupledesc. I *think* the information we need to reconstruct the TupleDesc is mostly present in the RTE; Kevin reused the ctecoltypes, ctecoltypmods, and ctecolcollations fields to store that information, which (a) probably requires some thought about renaming those fields but (b) seems like it ought to be enough to construct a viable TupleDesc. It seems that for CTEs, we somehow engineer things so that the RecursiveUnion's target-list is such that we can apply ExecAssignResultTypeFromTL() to it and get the tupledesc that matches its Tuplestorestate, but I'm kinda unclear about what makes that work and whether we can use a similar trick here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Heikki Linnakangas writes: > On 10/22/2014 11:10 PM, Robert Haas wrote: >> Studying this proposed design a bit further, I am a little fuzzy on >> what is supposed to happen in this design during parse analysis. In >> Kevin's current code, after checking whether a RangeVar might be a CTE >> reference and before deciding that it must be a table reference, >> scanNameSpaceForTsr() is used to check whether there's a tuplestore by >> that name and, if so, then we insert a RTE with type RTE_TUPLESTORE >> which references the tuplestore by name. To me, the obvious thing to >> do here seems to be to instead call p_tableref_hook and let it return >> a suitable RangeTblRef (or NULL if it wishes to take no action). In >> the case where the hook wants to redirect the use of that name to a >> tuplestore, it can add a range-table entry of type RTE_TUPLESTORE, and >> that entry can store a parameter-index rather than, as in the current >> design, a name. But then where does Heikki's notion of a >> RelationParam get used? > I was thinking that the hook would return a RelationParam. When parse > analysis sees the returned RelationParam, it adds an entry for that to > the range table, and creates the RangeTblRef for it. The way you > describe it works too, but manipulating the range table directly in the > hook seems a bit too low-level. The problem with that idea is that then the API for the hook has to cover every possible sort of RTE that hooks might wish to create; I see no reason to restrict them to creating just one kind. I agree that the hook should avoid *physically* manipulating the rangetable, but it seems reasonable to expect that it can call one of the addRangeTableEntryXXX functions provided by parse_relation.c, and then return a RangeTblEntry* gotten that way. So hooks would have an API more or less equivalent to, eg, transformRangeFunction(). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 10/22/2014 11:10 PM, Robert Haas wrote: On Tue, Sep 23, 2014 at 1:51 PM, Tom Lane wrote: Add a new p_tableref_hook function pointer, similar to p_paramref_hook. Whenever the parser sees a RangeVar that it doesn't recognize (or actually, I think it should call it *before* resolving regular tables, but let's ignore that for now), it calls the p_tableref_hook. It can return a new RelationParam node (similar to regular Param), which contains a numeric ID for the table/tuplestore, as well as its tuple descriptor. For the execution phase, add a new array of Tuplestorestates to ParamListInfo. Similar to the existing array of ParamExternalDatas. I haven't been following this issue closely, but this sounds like a really nice design. I'm on board with the parser hooks part of that. I don't especially agree with the idea of a new sub-structure for ParamListInfo: if you do that you will need a whole bunch of new boilerplate infrastructure to allocate, copy, and generally manage that structure, for darn little gain. What I'd suggest is just saying that some Params might have type INTERNAL with Datum values that are pointers to tuplestores; then all you need to do is remember which Param number has been assigned to the particular tuplestore you want. There is already precedent for that in the recursive CTE code, IIRC. Studying this proposed design a bit further, I am a little fuzzy on what is supposed to happen in this design during parse analysis. In Kevin's current code, after checking whether a RangeVar might be a CTE reference and before deciding that it must be a table reference, scanNameSpaceForTsr() is used to check whether there's a tuplestore by that name and, if so, then we insert a RTE with type RTE_TUPLESTORE which references the tuplestore by name. To me, the obvious thing to do here seems to be to instead call p_tableref_hook and let it return a suitable RangeTblRef (or NULL if it wishes to take no action). In the case where the hook wants to redirect the use of that name to a tuplestore, it can add a range-table entry of type RTE_TUPLESTORE, and that entry can store a parameter-index rather than, as in the current design, a name. But then where does Heikki's notion of a RelationParam get used? I was thinking that the hook would return a RelationParam. When parse analysis sees the returned RelationParam, it adds an entry for that to the range table, and creates the RangeTblRef for it. The way you describe it works too, but manipulating the range table directly in the hook seems a bit too low-level. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Sep 23, 2014 at 1:51 PM, Tom Lane wrote: >>> Add a new p_tableref_hook function pointer, similar to p_paramref_hook. >>> Whenever the parser sees a RangeVar that it doesn't recognize (or actually, >>> I think it should call it *before* resolving regular tables, but let's >>> ignore that for now), it calls the p_tableref_hook. It can return a new >>> RelationParam node (similar to regular Param), which contains a numeric ID >>> for the table/tuplestore, as well as its tuple descriptor. >>> >>> For the execution phase, add a new array of Tuplestorestates to >>> ParamListInfo. Similar to the existing array of ParamExternalDatas. > >> I haven't been following this issue closely, but this sounds like a >> really nice design. > > I'm on board with the parser hooks part of that. I don't especially agree > with the idea of a new sub-structure for ParamListInfo: if you do that you > will need a whole bunch of new boilerplate infrastructure to allocate, > copy, and generally manage that structure, for darn little gain. What I'd > suggest is just saying that some Params might have type INTERNAL with > Datum values that are pointers to tuplestores; then all you need to do is > remember which Param number has been assigned to the particular tuplestore > you want. There is already precedent for that in the recursive CTE code, > IIRC. Studying this proposed design a bit further, I am a little fuzzy on what is supposed to happen in this design during parse analysis. In Kevin's current code, after checking whether a RangeVar might be a CTE reference and before deciding that it must be a table reference, scanNameSpaceForTsr() is used to check whether there's a tuplestore by that name and, if so, then we insert a RTE with type RTE_TUPLESTORE which references the tuplestore by name. To me, the obvious thing to do here seems to be to instead call p_tableref_hook and let it return a suitable RangeTblRef (or NULL if it wishes to take no action). In the case where the hook wants to redirect the use of that name to a tuplestore, it can add a range-table entry of type RTE_TUPLESTORE, and that entry can store a parameter-index rather than, as in the current design, a name. But then where does Heikki's notion of a RelationParam get used? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Thu, Sep 4, 2014 at 12:14 AM, Kevin Grittner wrote: >> Did I miss something? > > Apparently. I did a search on the document and counted and got 101 > occurrences of "transition table". > | A transient table is a named table that may come into existence > | implicitly during the evaluation of a or the > | execution of a trigger. D'oh, I was reading only the sections about triggers. You are correct. Anyway, I tried out the latest from your GitHub branch and it seems most of my concerns no longer apply to the current version, as transition tables are now local to the trigger function. Thanks. Not sure if you're looking for feedback on this level yet, but I tried breaking it and found that transition tuplestores don't work with cursors. Probably not a valid use case once we have some other way to pass tuplestores between functions. I don't know if it could work anyway, as cursors may outlive the trigger call. But in that case, a better error message is in order. create table test1(i int); create or replace function test1trg() returns trigger language plpgsql as $$ declare curs cursor for select * from newtab; r record; begin for r in curs loop end loop; return new; end;$$; create trigger test1trg after insert on test1 referencing new table as newtab execute procedure test1trg(); insert into test1 values(1); ERROR: executor could not find named tuplestore "newtab" CONTEXT: PL/pgSQL function test1trg() line 6 at FOR over cursor I still see a chance of introducing security problems with SECURITY DEFINER trigger functions. An attacker can overshadow table names queried by such a function and inject arbitrary data into it. Similar to search_path vulnerabilities, but there are ways to avoid that. Perhaps there should be a restriction when using REFERENCING and the function is SECURITY DEFINER: require that the trigger definer (if not superuser) matches the function owner? Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 09/25/2014 06:54 PM, Kevin Grittner wrote: Heikki Linnakangas wrote: You cast the TuplestoreRelation to Plan, and pass it to CopyPlanFields. That will crash, because TuplestoreRelation is nothing like a Plan: Oops. That's a copy/paste error I should have noticed. Fixed, even though the node type might be going away. Since all of this seems to be working very well from a user point of view, I'm going to try to generate a lot more regression tests against the existing code before taking another run at the API, to make sure that things don't break in the refactoring. I didn't hit the copy/out bugs in testing so far -- any suggestions on a test that would exercise this code? (I'm probably missing something obvious.) There's some debugging code in tcop/postgres.c, search for COPY_PARSE_PLAN_TREES. It won't catch everything, but probably would've caught this one. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 09/25/2014 11:54 PM, Kevin Grittner wrote: > I have fixed the bug reported by Heikki; be sure to grab that. Will do. > I have been merging in changes to master as I go, so that bit rot > doesn't accumulate, but I don't squash or rebase; hopefully that > style works for you. IMO it only really matters before the final push to master; before then it's all just a matter of how you prefer to work. I'm a big fan of rebasing my feature branches as I go: git tag before-rebase git pull --rebase ... do any merges during rebase ... git tag -d before-rebase For bug fixes I tend to commit them separately, then when I rebase I squash them into the relevant patch. Git's "fixup! " commits are really handy for this; if I have a commit: Add widget support Everyone wants more widgets. and want to fix an issue in that commit I can just commit fixup! Add widget support It's spelled widget not wodget and when I "git rebase --autosquash master" they get automatically squashed into the relevant changeset. (I usually run with the config rebase.autosquash enabled so this happens during my rebase pulls on top of master). I got in the habit while working on RLS, to keep me sane with that patchset, and find it works well for me. However, everyone has a different work style. Colour me happy if it's in git at all. -- 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
Re: [HACKERS] delta relations in AFTER triggers
Heikki Linnakangas wrote: > You cast the TuplestoreRelation to Plan, and pass it to CopyPlanFields. > That will crash, because TuplestoreRelation is nothing like a Plan: Oops. That's a copy/paste error I should have noticed. Fixed, even though the node type might be going away. Since all of this seems to be working very well from a user point of view, I'm going to try to generate a lot more regression tests against the existing code before taking another run at the API, to make sure that things don't break in the refactoring. I didn't hit the copy/out bugs in testing so far -- any suggestions on a test that would exercise this code? (I'm probably missing something obvious.) Craig Ringer wrote: > On 09/15/2014 10:25 PM, Kevin Grittner wrote: > >> I broke out the changes from the previous patch in multiple commits >> in my repository on github: > > *Thankyou* > A nice patch series published in a git repo is so much easier to work > with than a giant squashed patch as an attachment. I have fixed the bug reported by Heikki; be sure to grab that. I have been merging in changes to master as I go, so that bit rot doesn't accumulate, but I don't squash or rebase; hopefully that style works for you. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 09/15/2014 10:25 PM, Kevin Grittner wrote: > I broke out the changes from the previous patch in multiple commits > in my repository on github: *Thankyou* That gives me the incentive to pull it and test it. A nice patch series published in a git repo is so much easier to work with than a giant squashed patch as an attachment. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On 08/28/2014 05:03 AM, Kevin Grittner wrote: > I don't have to squint that hard -- I've always been comfortable > with the definition of a table as a relation variable, and it's not > too big a stretch to expand that to a tuplestore. ;-) In fact, I > will be surprised if someone doesn't latch onto this to create a > new "declared temporary table" that only exists within the scope of > a compound statement (i.e., a BEGIN/END block). You would DECLARE > them just like you would a scalar variable in a PL, and they would > have the same scope. > > I'll take a look at doing this in the next couple days, and see > whether doing it that way is as easy as it seems on the face of it. Oracle's TABLE variables in PL/SQL are quite similar; it might be worth observing how they work for comparison. -- 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
Re: [HACKERS] delta relations in AFTER triggers
On 09/24/2014 12:22 AM, Kevin Grittner wrote:
Heikki Linnakangas wrote:
instead of passing parameters to the SPI calls individually, you
invented SPI_register_tuplestore which affects all subsequent SPI
calls.
All subsequent SPI calls on that particular SPI connection until it
is closed, except for any tuplestores are later unregistered.
Nested SPI connections do not automatically inherit the named
tuplestores; whatever code opens an SPI connection would need to
register them for the new context, if desired. This seemed to me
to provide minimal disruption to the existing SPI callers who might
want to use this.
Yeah, I got that. And note that I'm not saying that's necessarily a bad
design per se - it's just that it's different from the way parameters
work, and I don't like it for that reason.
You could imagine doing the same for parameters; have a
SPI_register_param() function that you could use to register parameter
types, and the parameters could then be referenced in any SPI calls that
follow (within the same connection). But as the code stands, SPI is
stateless wrt. to parameters, and tuplestores or relation parameters
should follow the lead.
The next question is how to pass the new hooks and tuplestores
However, there doesn't seem to be any way to do one-shot queries
with a ParserSetupHook and ParamListInfo. That seems like an
oversight, and would be nice to address that anyway.
There are dozens of SPI_prepare* and SPI_exec* calls scattered all
over the backend, pl, and contrib code which appear to get along
fine without that.
Yeah. None of the current callers have apparently needed that
functionality. But it's not hard to imagine cases where it would be
needed. For example, imagine a variant of EXECUTE '...' where all the
PL/pgSQL variables can be used in the query, like they can in static
queries:
declare
myvar int4;
tablename text;
begin
...
EXECUTE 'insert into ' || tablename ||' values (myvar)';
end;
Currently you have to use $1 in place of the variable name, and pass the
variable's current value with USING. If you wanted to make the above
work, you would need a variant of SPI_execute that can run a one-shot
query, with a parser-hook.
Whether you want to use a parser-hook or is orthogonal to whether or not
you want to run a one-shot query or prepare it and keep the plan.
Partly it may be because it involves something
of a modularity violation; the comment for the function used for
this call (where it *is* used) says:
/*
* plpgsql_parser_setup set up parser hooks for dynamic parameters
*
* Note: this routine, and the hook functions it prepares for, are logically
* part of plpgsql parsing. But they actually run during function execution,
* when we are ready to evaluate a SQL query or expression that has not
* previously been parsed and planned.
*/
No, that's something completely different. The comment points out that
even though plpgsql_parser_setup is in pl_comp.c, which contains code
related to compiling a PL/pgSQL function, it's actually called at
execution time, not compilation time.
Can you clarify what benefit you see to modifying the SPI API the
way you suggest, and what impact it might have on existing calling
code?
Well, we'll have to keep the existing functions anyway, to avoid
breaking 3rd party code that use them, so there would be no impact on
existing code. The benefit would be that you could use the parser hooks
and the ParamListInfo struct even when doing a one-shot query.
Or perhaps you could just use SPI_prepare_params +
SPI_execute_plan_with_paramlist even for one-shot queries. There is some
overhead when a SPIPlan has to be allocated, but maybe it's not big
enough to worry about. That would be worth measuring before adding new
functions to the SPI.
PS. the copy/read/write functions for TuplestoreRelation in the
patch are broken; TuplestoreRelation is not a subclass of Plan.
I'm not sure what you mean by "broken" -- could you elaborate?
Sure:
+ /*
+ * _copyTuplestoreRelation
+ */
+ static TuplestoreRelation *
+ _copyTuplestoreRelation(const TuplestoreRelation *from)
+ {
+ TuplestoreRelation *newnode = makeNode(TuplestoreRelation);
+
+ /*
+* copy node superclass fields
+*/
+ CopyPlanFields((const Plan *) from, (Plan *) newnode);
+
+ /*
+* copy remainder of node
+*/
+ COPY_STRING_FIELD(refname);
+
+ return newnode;
+ }
You cast the TuplestoreRelation to Plan, and pass it to CopyPlanFields.
That will crash, because TuplestoreRelation is nothing like a Plan:
+ /*
+ * TuplestoreRelation -
+ * synthetic node for tuplestore passed in to the query by name
+ *
+ * This is initially added to support trigger transition tables, but may find
+ * other uses, so we try to keep it generic.
+ */
+ typedef struct TuplestoreRelation
+ {
+ NodeTag type;
+ char *refname;
+ } TuplestoreRelat
Re: [HACKERS] delta relations in AFTER triggers
Thanks for reviewing this. I will spend some time looking at your recommendations in detail and seeing what it would take to implement them, and whether I agree that is better; but I wanted to point out a couple things regarding the SPI interface where I'm not sure you realize what's currently being done. I may want to argue some of the rest if I don't agree after more detailed review; this is just what jumps out on a first pass. Heikki Linnakangas wrote: > instead of passing parameters to the SPI calls individually, you > invented SPI_register_tuplestore which affects all subsequent SPI > calls. All subsequent SPI calls on that particular SPI connection until it is closed, except for any tuplestores are later unregistered. Nested SPI connections do not automatically inherit the named tuplestores; whatever code opens an SPI connection would need to register them for the new context, if desired. This seemed to me to provide minimal disruption to the existing SPI callers who might want to use this. > The next question is how to pass the new hooks and tuplestores > through the SPI interface. For prepared plans, the current > SPI_prepare_params + SPI_execute_plan_with_paramlist functions > work fine. They work fine, I guess, in the *one* place they are used. SPI_prepare_params() is called exactly *once* from plpgsql's pl_exec.c, and SPI_execute_plan_with_paramlist() is called twice from the same file. There are no other calls to either from anywhere else in the code base. > However, there doesn't seem to be any way to do one-shot queries > with a ParserSetupHook and ParamListInfo. That seems like an > oversight, and would be nice to address that anyway. There are dozens of SPI_prepare* and SPI_exec* calls scattered all over the backend, pl, and contrib code which appear to get along fine without that. Partly it may be because it involves something of a modularity violation; the comment for the function used for this call (where it *is* used) says: /* * plpgsql_parser_setup set up parser hooks for dynamic parameters * * Note: this routine, and the hook functions it prepares for, are logically * part of plpgsql parsing. But they actually run during function execution, * when we are ready to evaluate a SQL query or expression that has not * previously been parsed and planned. */ Can you clarify what benefit you see to modifying the SPI API the way you suggest, and what impact it might have on existing calling code? > PS. the copy/read/write functions for TuplestoreRelation in the > patch are broken; TuplestoreRelation is not a subclass of Plan. I'm not sure what you mean by "broken" -- could you elaborate? It is, in a lot of ways, parallel to the CommonTableExpr defined a little above it in the parsenodes.h file -- a relation which can only be referenced by unqualified name. It is after CTEs in search order, and before anything else. Having such a node in the tree after parse analysis allows a number of special cases to be handled pretty much the same as they are for CTEs, which seems like a good thing to me. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 09/23/2014 08:51 PM, Tom Lane wrote: Robert Haas writes: On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas wrote: Now, how do we make the tuplestores work similarly? Here's what I think we should do: Add a new p_tableref_hook function pointer, similar to p_paramref_hook. Whenever the parser sees a RangeVar that it doesn't recognize (or actually, I think it should call it *before* resolving regular tables, but let's ignore that for now), it calls the p_tableref_hook. It can return a new RelationParam node (similar to regular Param), which contains a numeric ID for the table/tuplestore, as well as its tuple descriptor. For the execution phase, add a new array of Tuplestorestates to ParamListInfo. Similar to the existing array of ParamExternalDatas. I haven't been following this issue closely, but this sounds like a really nice design. I'm on board with the parser hooks part of that. I don't especially agree with the idea of a new sub-structure for ParamListInfo: if you do that you will need a whole bunch of new boilerplate infrastructure to allocate, copy, and generally manage that structure, for darn little gain. What I'd suggest is just saying that some Params might have type INTERNAL with Datum values that are pointers to tuplestores; then all you need to do is remember which Param number has been assigned to the particular tuplestore you want. There is already precedent for that in the recursive CTE code, IIRC. Works for me. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Robert Haas writes: > On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas > wrote: >> Now, how do we make the tuplestores work similarly? Here's what I think we >> should do: >> >> Add a new p_tableref_hook function pointer, similar to p_paramref_hook. >> Whenever the parser sees a RangeVar that it doesn't recognize (or actually, >> I think it should call it *before* resolving regular tables, but let's >> ignore that for now), it calls the p_tableref_hook. It can return a new >> RelationParam node (similar to regular Param), which contains a numeric ID >> for the table/tuplestore, as well as its tuple descriptor. >> >> For the execution phase, add a new array of Tuplestorestates to >> ParamListInfo. Similar to the existing array of ParamExternalDatas. > I haven't been following this issue closely, but this sounds like a > really nice design. I'm on board with the parser hooks part of that. I don't especially agree with the idea of a new sub-structure for ParamListInfo: if you do that you will need a whole bunch of new boilerplate infrastructure to allocate, copy, and generally manage that structure, for darn little gain. What I'd suggest is just saying that some Params might have type INTERNAL with Datum values that are pointers to tuplestores; then all you need to do is remember which Param number has been assigned to the particular tuplestore you want. There is already precedent for that in the recursive CTE code, IIRC. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas wrote: > On 09/15/2014 05:25 PM, Kevin Grittner wrote: > Now, how do we make the tuplestores work similarly? Here's what I think we > should do: > > Add a new p_tableref_hook function pointer, similar to p_paramref_hook. > Whenever the parser sees a RangeVar that it doesn't recognize (or actually, > I think it should call it *before* resolving regular tables, but let's > ignore that for now), it calls the p_tableref_hook. It can return a new > RelationParam node (similar to regular Param), which contains a numeric ID > for the table/tuplestore, as well as its tuple descriptor. > > For the execution phase, add a new array of Tuplestorestates to > ParamListInfo. Similar to the existing array of ParamExternalDatas. I haven't been following this issue closely, but this sounds like a really nice design. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On 09/15/2014 05:25 PM, Kevin Grittner wrote: Tom Lane wrote: Heikki Linnakangas writes: On 08/30/2014 12:15 AM, Kevin Grittner wrote: If we were to go with the hooks as you propose, we would still need to take the information from TriggerData and put it somewhere else for the hook to reference. Sure. FWIW, I agree with Heikki on this point. It makes a lot more sense for the parser to provide hooks comparable to the existing hooks for resolving column refs, and it's not apparent that loading such functionality into SPI is sane at all. OTOH, I agree with Kevin that the things we're talking about are lightweight relations not variables. Try as I might, I was unable to find any sensible way to use hooks. If the issue was only the parsing, the route was fairly obvious, but the execution side needs to access the correct tuplestore(s) for each run, too -- so the sort of information provided by relcache needed to be passed in to based on the context within the process (i.e., if you have nested triggers firing, each needs to use a different tuplestore with the same name in the same function, even though it's using the same plan). On both sides it seemed easier to pass things through the same sort of techniques as "normal" parameters; I couldn't find a way to use hooks that didn't just make things uglier. Hmph. You didn't actually use the same sort of techniques we use for normal parameters. You invented a new TsrCache registry, which marries the metadata for planning with the tuplestore itself. That's quite different from the way we deal with parameters (TsrCache is a misnomer, BTW; it's not a cache, but the primary source of information for the planner). And instead of passing parameters to the SPI calls individually, you invented SPI_register_tuplestore which affects all subsequent SPI calls. To recap, this is how normal parameters work: In the parse stage, you pass a ParserSetupHook function pointer to the parser. The parser calls the callback, which sets up more hooks in the ParseState struct: a column-ref hook and/or a param ref hook. The parser then proceeds to parse the query, and whenever it sees a reference to a column that it doesn't recognize, or a $n style parameter marker, it calls the column-ref or param-ref hook. The column- or param-ref hook can return a Param node, indicating that the parameter's value will be supplied later, at execution time. The Param node contains a numeric ID for the parameter, and the type OID and other information needed to complete the parsing. At execution time, you pass a ParamListInfo struct to the executor. It contains values for all the parameters. Alternatively, the values can be supplied lazily, by providing a param-fetch hook in the ParamListInfo struct. Whenever the executor needs the value of a parameter, and the ParamListInfo struct doesn't contain it, it calls the paramFetch hook which should fill it in ParamListInfo. Now, how do we make the tuplestores work similarly? Here's what I think we should do: Add a new p_tableref_hook function pointer, similar to p_paramref_hook. Whenever the parser sees a RangeVar that it doesn't recognize (or actually, I think it should call it *before* resolving regular tables, but let's ignore that for now), it calls the p_tableref_hook. It can return a new RelationParam node (similar to regular Param), which contains a numeric ID for the table/tuplestore, as well as its tuple descriptor. For the execution phase, add a new array of Tuplestorestates to ParamListInfo. Similar to the existing array of ParamExternalDatas. The next question is how to pass the new hooks and tuplestores through the SPI interface. For prepared plans, the current SPI_prepare_params + SPI_execute_plan_with_paramlist functions work fine. However, there doesn't seem to be any way to do one-shot queries with a ParserSetupHook and ParamListInfo. That seems like an oversight, and would be nice to address that anyway. PS. the copy/read/write functions for TuplestoreRelation in the patch are broken; TuplestoreRelation is not a subclass of Plan. (But if you go with the approach I'm advocating for, TuplestoreRelation in its current form will be gone) - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Tom Lane wrote: > Heikki Linnakangas writes: > >> On 08/30/2014 12:15 AM, Kevin Grittner wrote: >>> If we were to go with the hooks as you propose, we would still need >>> to take the information from TriggerData and put it somewhere else >>> for the hook to reference. > >> Sure. > > FWIW, I agree with Heikki on this point. It makes a lot more sense for > the parser to provide hooks comparable to the existing hooks for resolving > column refs, and it's not apparent that loading such functionality into > SPI is sane at all. > > OTOH, I agree with Kevin that the things we're talking about are > lightweight relations not variables. Try as I might, I was unable to find any sensible way to use hooks. If the issue was only the parsing, the route was fairly obvious, but the execution side needs to access the correct tuplestore(s) for each run, too -- so the sort of information provided by relcache needed to be passed in to based on the context within the process (i.e., if you have nested triggers firing, each needs to use a different tuplestore with the same name in the same function, even though it's using the same plan). On both sides it seemed easier to pass things through the same sort of techniques as "normal" parameters; I couldn't find a way to use hooks that didn't just make things uglier. I see the down side of making this a feature which can only be used from SPI, so I've updated the patch to allow it from other contexts. On the other hand, I see many uses for it where SPI *is* used, and the SPI interface needs to change to support that. The functions I had added are one way to do that on the parsing/planning side without breaking any existing code. The same information (i.e., the metadata) needs to be passed to the executor along with references to the actual tuplestores, and that needs to go through a different path -- at least for the plpgsql usage. I broke out the changes from the previous patch in multiple commits in my repository on github: commit 2520db8fbb41c68a82c2c750c8543154c6d85eb9 Author: Kevin Grittner Date: Mon Sep 15 01:17:14 2014 -0500 Use executor state rather than SPI to get named tuplestores. spi.c and trigger.c will need a little more clean-up to match this, and it's likely that not all places that need to pass the baton are doing so, but at least this passes regression tests. commit de9067258125226d1625f160c3eee9aff90ca598 Author: Kevin Grittner Date: Sun Sep 14 22:01:04 2014 -0500 Pass the Tsrcache through ParserState to parse analysis. commit e94779c1e22ec587446a7aa2593ba5f102b6a28b Author: Kevin Grittner Date: Sun Sep 14 19:23:45 2014 -0500 Modify SPI to use Tsrcache instead of List. commit 7af841881d9113eb4c8dca8e82dc1867883bf75d Author: Kevin Grittner Date: Sun Sep 14 18:45:54 2014 -0500 Create context-specific Tsrcache. Not used yet. commit 35790a4b6c236d09e0be261be9b0017d34eaf9c9 Author: Kevin Grittner Date: Sun Sep 14 16:49:37 2014 -0500 Create Tsrmd structure so tuplestore.h isn't needed in parser. commit 93d57c580da095b71d9214f69fede71d2f8ed840 Author: Kevin Grittner Date: Sun Sep 14 15:59:45 2014 -0500 Improve some confusing naming for TuplestoreRelation node. Anyone who has already reviewed the earlier patch may want to look at these individually: https://github.com/kgrittn/postgres/compare/transition Attached is a new patch including all of that. Hopefully I haven't misunderstood what Heikki and Tom wanted; if I have, just let me know where I went wrong. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company trigger-transition-tables-v4.patch.gz Description: application/tgz -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Marti Raudsepp wrote: > On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner wrote: >> Marti Raudsepp wrote: >>> The concept of "lightweight relations" that pop into existence when a >>> certain kind of trigger definition is used somewhere in the function >>> stack, without a CREATE TABLE, without being discoverable in >>> information_schema etc., I find needs some more justification than >>> I've seen in this thread. So far I've only heard that it's more >>> convenient to implement in the current PostgreSQL code base. >> >> It is required by the SQL standard. > > I had a cursory read of the SQL 20nn draft and I don't get this > impression. The only place I could find discussing the behavior of > "transition tables" is in Foundation "4.39.1 General description of > triggers", which says: > > "Special variables make the data in the transition table(s) available > to the triggered action. For a statement-level > trigger the variable is one whose value is a transition table." > > There is no information about the scoping of such variables, so I > assume it refers to a regular locally scoped variable. > > Did I miss something? Apparently. I did a search on the document and counted and got 101 occurrences of "transition table". I might be off by a few, but that should be pretty close. Perhaps this, from 4.14 most directly answers your point: | A transient table is a named table that may come into existence | implicitly during the evaluation of a or the | execution of a trigger. A transient table is identified by a | if it arises during the evaluation of a , or by a if it arises during | the execution of a trigger. Such tables exist only for the | duration of the executing SQL-statement containing the or for the duration of the executing trigger. > Are you reading a different version of the spec? I'm looking at a draft of 200x from 2006-02-01. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner wrote: > Marti Raudsepp wrote: >> The concept of "lightweight relations" that pop into existence when a >> certain kind of trigger definition is used somewhere in the function >> stack, without a CREATE TABLE, without being discoverable in >> information_schema etc., I find needs some more justification than >> I've seen in this thread. So far I've only heard that it's more >> convenient to implement in the current PostgreSQL code base. > > It is required by the SQL standard. I had a cursory read of the SQL 20nn draft and I don't get this impression. The only place I could find discussing the behavior of "transition tables" is in Foundation "4.39.1 General description of triggers", which says: "Special variables make the data in the transition table(s) available to the triggered action. For a statement-level trigger the variable is one whose value is a transition table." There is no information about the scoping of such variables, so I assume it refers to a regular locally scoped variable. Did I miss something? Are you reading a different version of the spec? Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Kevin Grittner wrote: > Marti Raudsepp wrote: >> What are the interactions with search_path? > > Pretty much the same as the interactions of RTEs with search_path. > If the apparent relation name is not schema-qualified, parse > analysis first tries to resolve the name as an RTE, and if that > fails it tries to resolve it as a named tuplestore, and if that > fails it goes to the catalogs using search_path. Argh. s/RTE/CTE/ -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Marti Raudsepp wrote: > On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane wrote: > The concept of "lightweight relations" that pop into existence when a > certain kind of trigger definition is used somewhere in the function > stack, without a CREATE TABLE, without being discoverable in > information_schema etc., I find needs some more justification than > I've seen in this thread. So far I've only heard that it's more > convenient to implement in the current PostgreSQL code base. It is required by the SQL standard. > I'm sure more questions would pop up in practice, but as Heikki > mentioned: Are such relations also visible to other functions called > by the trigger function? > * If yes, this introduces non-obvious dependencies between functions. > What happens when one trigger with delta relations invokes another > trigger, does the previous one get shadowed or overwritten? This is indeed a killer objection. As things stand in the patch, a function called from a trigger function might have the table of the same name (if it's not a not schema-qualified reference) shadowed, or it might not -- depending on whether it was already planned. That's obviously not acceptable. Passing the metadata from the TriggerData structure to the PLpgSQL_execstate structure to the PLpgSQL_expr structure and on to the ParseState structure, and passing it down to child ParseState structures as needed, along with similar passing of the Tuplestorestate pointer (and associated name) to the execution state structures should fix that. > What are the interactions with search_path? Pretty much the same as the interactions of RTEs with search_path. If the apparent relation name is not schema-qualified, parse analysis first tries to resolve the name as an RTE, and if that fails it tries to resolve it as a named tuplestore, and if that fails it goes to the catalogs using search_path. > Can an unprivileged function override relation names when calling > a SECURITY DEFINER function? By changing things to the way Heikki and Tom suggest, any called functions are not aware of or affected by a named tuplestore in the caller's context. (Changing *back*, actually -- I had this largely done that way before; but it seemed like a rather fragile relay race, passing the baton from one structure to another at odd places. I guess there's no helping that. Or maybe once I post a version changed back to that someone can show me something I missed that makes it better.) > You could argue that CREATE TEMP TABLE already has some of these > problems, but it's very rare that people actually need to use that. If > delta relations get built on this new mechanism, avoiding won't be an > option any more. Not true -- you don't have them unless you request them in CREATE TRIGGER. Nobody can be using this now, so a table owner must *choose* to add the REFERENCING clause to the CREATE TRIGGER statement for it to matter in the trigger function that is then referenced. Perhaps if we implement the ability to specify the trigger code in the CREATE TRIGGER statement itself (rather than requiring that a trigger function be created first) it will be easier to look at and cope with. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane wrote: > OTOH, I agree with Kevin that the things we're talking about are > lightweight relations not variables. My worry is that PL/pgSQL and Postgres's SQL dialect is turning into a Frankenstein monster with many ways to do the same thing, each having different semantics that require effort to reason about. Variables and function arguments are non-contriversial, every experienced coder understands their semantics without thinking twice -- even if they're not familiar with Postgres. The concept of "lightweight relations" that pop into existence when a certain kind of trigger definition is used somewhere in the function stack, without a CREATE TABLE, without being discoverable in information_schema etc., I find needs some more justification than I've seen in this thread. So far I've only heard that it's more convenient to implement in the current PostgreSQL code base. I'm sure more questions would pop up in practice, but as Heikki mentioned: Are such relations also visible to other functions called by the trigger function? * If yes, this introduces non-obvious dependencies between functions. What happens when one trigger with delta relations invokes another trigger, does the previous one get shadowed or overwritten? What are the interactions with search_path? Can an unprivileged function override relation names when calling a SECURITY DEFINER function? * If not, this further inhibits developers from properly modularizing their trigger code (this is already a problem due to the current magic trigger variables). Even if these questions have reasonable answers, it takes mental effort to remember the details. Procedure code debugging, especially triggers, is hard enough due to poor tooling; increasing the cognitive load should not be done lightly. You could argue that CREATE TEMP TABLE already has some of these problems, but it's very rare that people actually need to use that. If delta relations get built on this new mechanism, avoiding won't be an option any more. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] delta relations in AFTER triggers
Heikki Linnakangas writes: > On 08/30/2014 12:15 AM, Kevin Grittner wrote: >> If we were to go with the hooks as you propose, we would still need >> to take the information from TriggerData and put it somewhere else >> for the hook to reference. > Sure. FWIW, I agree with Heikki on this point. It makes a lot more sense for the parser to provide hooks comparable to the existing hooks for resolving column refs, and it's not apparent that loading such functionality into SPI is sane at all. OTOH, I agree with Kevin that the things we're talking about are lightweight relations not variables. 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
