Re: [HACKERS] delta relations in AFTER triggers

[Robert Haas]

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

[Robert Haas]

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

[Noah Misch]

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:

Re: [HACKERS] delta relations in AFTER triggers

[Noah Misch]

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

[Noah Misch]

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

[Noah Misch]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Corey Huinker]

>
> 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

[Thomas Munro]

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=rep1=pdf
[2] 
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.31.3208=rep1=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

[Kevin Grittner]

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

[Thomas Munro]

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

[Kevin Grittner]

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

[Tom Lane]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Corey Huinker]

>
> > 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

[Craig Ringer]

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

[David Fetter]

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

[Kevin Grittner]

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

[Robert Haas]

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

[Tom Lane]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Alvaro Herrera]

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

[Kevin Grittner]

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

[Thomas Munro]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[David Steele]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Michael Paquier]

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)

[Jim Nasby]

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)

[Nico Williams]

[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 

Re: [HACKERS] delta relations in AFTER triggers

[Kevin Grittner]

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

[Nico Williams]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[Nico Williams]

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

[Kevin Grittner]

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

[Thomas Munro]

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

[Thomas Munro]

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

[Craig Ringer]

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

[Craig Ringer]

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

[Kevin Grittner]

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

[Haribabu Kommi]

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

[Jim Nasby]

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

[Thomas Munro]

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

[Kevin Grittner]

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

[Alvaro Herrera]

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

[Robert Haas]

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

[Kevin Grittner]

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 

Re: [HACKERS] delta relations in AFTER triggers

[Craig Ringer]

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

[Kevin Grittner]

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

[Kevin Grittner]

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

[Adam Brusselback]

>
> 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

[Kevin Grittner]

> 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

[Adam Brusselback]

>
> 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

[Pavel Stehule]

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

[Kevin Grittner]

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

[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%

--
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

[Kevin Grittner]

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  |  

Re: [HACKERS] delta relations in AFTER triggers

[Michael Paquier]

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

[Kevin Grittner]

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 

Re: [HACKERS] delta relations in AFTER triggers

[Kevin Grittner]

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.  

Re: [HACKERS] delta relations in AFTER triggers

[Robert Haas]

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

[Kevin Grittner]

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

[David Fetter]

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]

On Thu, Oct 23, 2014 at 11:19 AM, Robert Haas robertmh...@gmail.com 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

[Kevin Grittner]

Robert Haas robertmh...@gmail.com 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

[Robert Haas]

On Wed, Oct 22, 2014 at 5:29 PM, Tom Lane t...@sss.pgh.pa.us 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

[Robert Haas]

On Tue, Sep 23, 2014 at 1:51 PM, Tom Lane t...@sss.pgh.pa.us 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

[Heikki Linnakangas]

On 10/22/2014 11:10 PM, Robert Haas wrote:

On Tue, Sep 23, 2014 at 1:51 PM, Tom Lane t...@sss.pgh.pa.us 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

[Tom Lane]

Heikki Linnakangas hlinnakan...@vmware.com 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

[Heikki Linnakangas]

On 09/25/2014 06:54 PM, Kevin Grittner wrote:

Heikki Linnakangas hlinnakan...@vmware.com 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

[Kevin Grittner]

Heikki Linnakangas hlinnakan...@vmware.com 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 cr...@2ndquadrant.com 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

[Craig Ringer]

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]

On 09/24/2014 12:22 AM, Kevin Grittner wrote:

Heikki Linnakangas hlinnakan...@vmware.com 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   

Re: [HACKERS] delta relations in AFTER triggers

[Craig Ringer]

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

[Craig Ringer]

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

[Heikki Linnakangas]

On 09/15/2014 05:25 PM, Kevin Grittner wrote:

Tom Lane t...@sss.pgh.pa.us wrote:

Heikki Linnakangas hlinnakan...@vmware.com 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

[Robert Haas]

On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas
hlinnakan...@vmware.com 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

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas
 hlinnakan...@vmware.com 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

[Heikki Linnakangas]

On 09/23/2014 08:51 PM, Tom Lane wrote:

Robert Haas robertmh...@gmail.com writes:

On Tue, Sep 23, 2014 at 12:46 PM, Heikki Linnakangas
hlinnakan...@vmware.com 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

[Kevin Grittner]

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 hlinnakan...@vmware.com 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

[Kevin Grittner]

Tom Lane t...@sss.pgh.pa.us wrote:
 Heikki Linnakangas hlinnakan...@vmware.com 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 kgri...@postgresql.org
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 kgri...@postgresql.org
Date:   Sun Sep 14 22:01:04 2014 -0500

Pass the Tsrcache through ParserState to parse analysis.

commit e94779c1e22ec587446a7aa2593ba5f102b6a28b
Author: Kevin Grittner kgri...@postgresql.org
Date:   Sun Sep 14 19:23:45 2014 -0500

Modify SPI to use Tsrcache instead of List.

commit 7af841881d9113eb4c8dca8e82dc1867883bf75d
Author: Kevin Grittner kgri...@postgresql.org
Date:   Sun Sep 14 18:45:54 2014 -0500

Create context-specific Tsrcache.

Not used yet.

commit 35790a4b6c236d09e0be261be9b0017d34eaf9c9
Author: Kevin Grittner kgri...@postgresql.org
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 kgri...@postgresql.org
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

[Kevin Grittner]

Marti Raudsepp ma...@juffo.org wrote:
 On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane t...@sss.pgh.pa.us 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

[Kevin Grittner]

Kevin Grittner kgri...@ymail.com wrote:
 Marti Raudsepp ma...@juffo.org 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]

On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner kgri...@ymail.com wrote:
 Marti Raudsepp ma...@juffo.org 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]

Marti Raudsepp ma...@juffo.org wrote:
 On Wed, Sep 3, 2014 at 10:49 PM, Kevin Grittner kgri...@ymail.com wrote:
 Marti Raudsepp ma...@juffo.org 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 query expression or the
| execution of a trigger. A transient table is identified by a 
| query name if it arises during the evaluation of a query 
| expression, or by a transition table name if it arises during 
| the execution of a trigger. Such tables exist only for the 
| duration of the executing SQL-statement containing the query 
| expression 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

[Marti Raudsepp]

On Mon, Sep 1, 2014 at 9:09 PM, Tom Lane t...@sss.pgh.pa.us 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]

On 08/30/2014 12:15 AM, Kevin Grittner wrote:

Heikki Linnakangas hlinnakan...@vmware.com wrote:

On 08/28/2014 12:03 AM, Kevin Grittner wrote:

Heikki Linnakangas hlinnakan...@vmware.com wrote:

I suggest adding a new hook to the ParseState struct, (p_rangevar_hook
?). The planner calls it whenever it sees a reference to a table, and
the hook function returns back some sort of placeholder reference to the
tuplestore. With variables, the hook returns a Param node, and at
execution time, the executor calls the paramFetch hook to fetch the
value of the param. For relations/tuplestores, I guess we'll need to
invent something like a Param node, but for holding information about
the relation. Like your TsrData struct, but without the pointer to the
tuplestore. At execution time, in the SPI_execute call, you pass the
pointer to the tuplestore in the ParamListInfo struct, like you pass
parameter values.

Does this make sense?


I see your point, but SPI first has to be made aware of the
tuplestores and their corresponding names and TupleDesc structures.
Does it make sense to keep the SPI_register_tuplestore() and
SPI_unregister_tuplestore() functions for the client side of the
API, and pass things along to the parse analysis through execution
phases using the techniques you describe?


Sorry, I didn't understand that. What do you mean by first, and the
client side of the API? I don't see any need for the
SPI_register_tuplestore() and and SPI_unregister_tuplestore() functions
if you use the hooks.


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.


The hooks are generalized for plpgsql,
not just for triggers, and it doesn't seem appropriate for them to
be fishing around in the TriggerData structure.


PLpgSQL_execstate seems like the appropriate place.


And what if we add other sources for tuplestores?


What about it?


The lookup during parse analysis
each time an apparent relation name is encountered must be simple
and fast.


We already use hooks for ColumnRefs, which are called even more often, 
and we haven't had a problem making that fast enough.



I want named tuplestores to be easy to use from *all* PLs (for
trigger usage) as well as useful for other purposes people may want
to develop.


I'm not sure other PLs would even want to resolve the old/new relations 
like PL/pgSQL does. It might be more natural to access the new/old 
tuplestores as perl or python hashes or arrays, for example. But if they 
do, it's not that difficult to write the hooks.



I had to change the hashkey for plpgsql's plan
caching, but that needs to be done regardless of the API (to
prevent problems in the obscure case that someone attaches the same
trigger function to the same table for the same events more than
once with different trigger names and different transition table
names).  If you ignore that, the *entire* change to use this in
plpgsql is to add these lines to plpgsql_exec_trigger():

 /*
  * Capture the NEW and OLD transition TABLE tuplestores (if specified for
  * this trigger).
  */
 if (trigdata-tg_newtable)
 {
 Tsr tsr = palloc(sizeof(TsrData));

 tsr-name = trigdata-tg_trigger-tgnewtable;
 tsr-tstate = trigdata-tg_newtable;
 tsr-tupdesc = trigdata-tg_relation-rd_att;
 tsr-relid = trigdata-tg_relation-rd_id;
 SPI_register_tuplestore(tsr);
 }
 if (trigdata-tg_oldtable)
 {
 Tsr tsr = palloc(sizeof(TsrData));

 tsr-name = trigdata-tg_trigger-tgoldtable;
 tsr-tstate = trigdata-tg_oldtable;
 tsr-tupdesc = trigdata-tg_relation-rd_att;
 tsr-relid = trigdata-tg_relation-rd_id;
 SPI_register_tuplestore(tsr);
 }

With the new SPI functions, the code to implement this in each
other PL should be about the same (possibly identical), and areas
using SPI only need similar code to make tuplestores visible to the
planner and usable in the executor if someone has another use for
this.  You just do the above once you have run SPI_connect() and
before preparing or executing any query that references the named
tuplestore.


With hooks, the code to implement them in other PLs would be about the 
same too, if they want the same behavior.



 It remains available on that SPI connection until
SPI_finish() is called or you explicitly unregister it (by name).


Yeah, I don't like that. The SPI interface is currently stateless. Well, 
except for cursors and plans explicitly saved with SPI_keepplan. But the 
way queries are parsed is stateless - you pass all the necessary 
information as part of the SPI_execute call (or similar), using direct 
arguments and the ParamListInfo struct.



If you don't want to use hooks, I nevertheless feel that the old/new 
relations should be passed as part of the ParamListInfo struct, one way 
or another. With hooks, you would set the parserSetup hook, 

Re: [HACKERS] delta relations in AFTER triggers

[Tom Lane]

Heikki Linnakangas hlinnakan...@vmware.com 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