Thread: pgsql: Use slots in trigger infrastructure,except for the actual invoc
Use slots in trigger infrastructure, except for the actual invocation. In preparation for abstracting table storage, convert trigger.c to track tuples in slots. Which also happens to make code calling triggers simpler. As the calling interface for triggers themselves is not changed in this patch, HeapTuples still are extracted from the slot at that time. But that's handled solely inside trigger.c, not visible to callers. It's quite likely that we'll want to revise the external trigger interface, but that's a separate large project. As part of this work the slots used for old/new/return tuples are moved from EState into ResultRelInfo, as different updated tables might need different slots. The slots are now also now created on-demand, which is good both from an efficiency POV, but also makes the modifying code simpler. Author: Andres Freund, Amit Khandekar and Ashutosh Bapat Discussion: https://postgr.es/m/20180703070645.wchpu5muyto5n647@alap3.anarazel.de Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ff11e7f4b9ae017585c3ba146db7ba39c31f209a Modified Files -------------- contrib/postgres_fdw/postgres_fdw.c | 16 +- src/backend/commands/copy.c | 24 +- src/backend/commands/tablecmds.c | 2 - src/backend/commands/trigger.c | 673 ++++++++++++++++--------------- src/backend/executor/execMain.c | 6 +- src/backend/executor/execReplication.c | 25 +- src/backend/executor/execTuples.c | 4 + src/backend/executor/execUtils.c | 69 +++- src/backend/executor/nodeModifyTable.c | 166 +++----- src/backend/replication/logical/worker.c | 5 - src/backend/utils/adt/ri_triggers.c | 187 +++++---- src/include/commands/trigger.h | 24 +- src/include/executor/executor.h | 4 + src/include/nodes/execnodes.h | 8 +- 14 files changed, 642 insertions(+), 571 deletions(-)
Hi, On 2019-02-27 04:41:28 +0000, Andres Freund wrote: > Use slots in trigger infrastructure, except for the actual invocation. Andrew, I see that this broke crake's redis_fdw check. I see it actually fails with an error, rather than fail to build. Could you perhaps enable generating backtraces? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2019-02-27%2005%3A32%3A24 Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Use slots in trigger infrastructure, except for the actual invocation.
I believe it's this commit that is resulting in my compiler bleating
about
trigger.c: In function 'afterTriggerInvokeEvents':
trigger.c:4493: warning: 'rInfo' may be used uninitialized in this function
Please fix.
regards, tom lane
Hi, On 2019-02-27 11:35:18 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Use slots in trigger infrastructure, except for the actual invocation. > > I believe it's this commit that is resulting in my compiler bleating > about > > trigger.c: In function 'afterTriggerInvokeEvents': > trigger.c:4493: warning: 'rInfo' may be used uninitialized in this function Hm, yea, I can see why a compiler, especially without doing more expensive control flow analysis, would get this wrong. Easier to understand if we NULL initialize rInfo, not just rel, too. Pushed. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Hm, yea, I can see why a compiler, especially without doing more
> expensive control flow analysis, would get this wrong. Easier to
> understand if we NULL initialize rInfo, not just rel, too. Pushed.
All quiet now; thanks.
regards, tom lane
Re: pgsql: Use slots in trigger infrastructure, except for the actualinvoc
From
Andrew Dunstan
Date:
On 2/27/19 1:22 AM, Andres Freund wrote:
> Hi,
>
> On 2019-02-27 04:41:28 +0000, Andres Freund wrote:
>> Use slots in trigger infrastructure, except for the actual invocation.
> Andrew, I see that this broke crake's redis_fdw check. I see it actually
> fails with an error, rather than fail to build. Could you perhaps enable
> generating backtraces?
Not sure why it should fail to build.
Backtraces should be enabled in the animal - I will investigate why
we're not getting them here. Meanwhile, here's one for this problem:
Core was generated by `postgres: buildfarm contrib_regression_redis_fdw [l'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000000000086ab23 in GetMemoryChunkContext (pointer=0x0) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/include/utils/memutils.h:127
127 context = *(MemoryContext *) (((char *) pointer) -
sizeof(void *));
Missing separate debuginfos, use: dnf debuginfo-install
cyrus-sasl-lib-2.1.27-0.3rc7.fc29.x86_64 glibc-2.28-17.fc29.x86_64
hiredis-0.13.3-9.fc29.x86_64 keyutils-libs-1.5.10-8.fc29.x86_64
krb5-libs-1.16.1-21.fc29.x86_64 libcom_err-1.44.3-1.fc29.x86_64
libselinux-2.8-4.fc29.x86_64 libxcrypt-4.2.3-1.fc29.x86_64
libxml2-2.9.8-4.fc29.x86_64 openldap-2.4.46-9.fc29.x86_64
openssl-libs-1.1.1-3.fc29.x86_64 pcre2-10.32-3.fc29.x86_64
xz-libs-5.2.4-3.fc29.x86_64 zlib-1.2.11-14.fc29.x86_64
(gdb) bt
#0 0x000000000086ab23 in GetMemoryChunkContext (pointer=0x0) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/include/utils/memutils.h:127
#1 pfree (pointer=0x0) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/utils/mmgr/mcxt.c:1033
#2 0x0000000000486f85 in heap_freetuple (htup=<optimized out>) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/access/common/heaptuple.c:1340
#3 0x0000000000607821 in tts_buffer_heap_clear (slot=0x2731300) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/executor/execTuples.c:653
#4 0x0000000000607bbe in ExecClearTuple (slot=0x2731300) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/include/executor/tuptable.h:425
#5 ExecResetTupleTable (tupleTable=0x2730638, shouldFree=false) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/executor/execTuples.c:1147
#6 0x00000000005fe6c9 in ExecEndPlan (estate=0x272f890,
planstate=<optimized out>) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/executor/execMain.c:1558
#7 standard_ExecutorEnd (queryDesc=0x2683770) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/executor/execMain.c:494
#8 0x000000000073f925 in ProcessQuery (plan=<optimized out>,
sourceText=0x2664390 "delete from db15_w_1key_scalar;", params=0x0,
queryEnv=0x0, dest=0x274e470, completionTag=0x7fff9a230180 "DELETE 1")
at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/tcop/pquery.c:204
#9 0x000000000073fb00 in PortalRunMulti (portal=portal@entry=0x26c9510,
isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false, dest=dest@entry=0x274e470,
altdest=altdest@entry=0x274e470,
completionTag=completionTag@entry=0x7fff9a230180 "DELETE 1") at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/tcop/pquery.c:1283
#10 0x00000000007405c1 in PortalRun (portal=portal@entry=0x26c9510,
count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true,
run_once=run_once@entry=true, dest=dest@entry=0x274e470,
altdest=altdest@entry=0x274e470, completionTag=0x7fff9a230180 "DELETE 1")
at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/tcop/pquery.c:796
#11 0x000000000073c84f in exec_simple_query (query_string=0x2664390
"delete from db15_w_1key_scalar;") at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/tcop/postgres.c:1215
#12 0x000000000073df69 in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x268d760, dbname=<optimized out>, username=<optimized
out>) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/tcop/postgres.c:4256
#13 0x00000000006d13c6 in BackendRun (port=0x2686cd0, port=0x2686cd0) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4382
#14 BackendStartup (port=0x2686cd0) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:4073
#15 ServerLoop () at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:1703
#16 0x00000000006d21e5 in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x265ca50) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/postmaster/postmaster.c:1376
#17 0x000000000047dade in main (argc=3, argv=0x265ca50) at
/home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:228
Here's the relevant portion of the test script, leading up to the
failing statement:
create foreign table db15_w_1key_scalar(val text)
server localredis
options (singleton_key 'w_1key_scalar', database '15');
select * from db15_w_1key_scalar;
insert into db15_w_1key_scalar values ('only row');
select * from db15_w_1key_scalar;
insert into db15_w_1key_scalar values ('only row');
delete from db15_w_1key_scalar where val = 'not only row';
select * from db15_w_1key_scalar;
update db15_w_1key_scalar set val = 'new scalar val';
select * from db15_w_1key_scalar;
delete from db15_w_1key_scalar;
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-02-27 12:59:16 -0500, Andrew Dunstan wrote: > > On 2/27/19 1:22 AM, Andres Freund wrote: > > Hi, > > > > On 2019-02-27 04:41:28 +0000, Andres Freund wrote: > >> Use slots in trigger infrastructure, except for the actual invocation. > > Andrew, I see that this broke crake's redis_fdw check. I see it actually > > fails with an error, rather than fail to build. Could you perhaps enable > > generating backtraces? > > > Not sure why it should fail to build. Oh, because one of the options was that it used a slot from a variable name that doesn't exist anymore... > Backtraces should be enabled in the animal - I will investigate why > we're not getting them here. Meanwhile, here's one for this problem: Thanks, that helps! My first theories as to what's going on fell flat, unfortunately. I guess I'll have to figure out how to run that locally :(. Greetings, Andres Freund
Hi, On 2019-02-27 10:16:21 -0800, Andres Freund wrote: > On 2019-02-27 12:59:16 -0500, Andrew Dunstan wrote: > > Backtraces should be enabled in the animal - I will investigate why > > we're not getting them here. Meanwhile, here's one for this problem: > > Thanks, that helps! My first theories as to what's going on fell flat, > unfortunately. I guess I'll have to figure out how to run that locally > :(. The issue was just that we didn't allow materializing virtual tuples stored in a buffer tuple table slot. Locally allowing that fixed the problem for me, I assume that'll also fix the crake. It was already on a list of patches for pluggable storage... Btw, shouldn't redisExecForeignDelete return NULL when there's no row matched for the deletion? That's possible, I'd assume, if there's a concurrent deletion. Greetings, Andres Freund
Re: pgsql: Use slots in trigger infrastructure, except for the actualinvoc
From
Andrew Dunstan
Date:
On 2/28/19 3:41 PM, Andres Freund wrote: > Hi, > > On 2019-02-27 10:16:21 -0800, Andres Freund wrote: >> On 2019-02-27 12:59:16 -0500, Andrew Dunstan wrote: >>> Backtraces should be enabled in the animal - I will investigate why >>> we're not getting them here. Meanwhile, here's one for this problem: >> Thanks, that helps! My first theories as to what's going on fell flat, >> unfortunately. I guess I'll have to figure out how to run that locally >> :(. > The issue was just that we didn't allow materializing virtual tuples > stored in a buffer tuple table slot. Locally allowing that fixed the > problem for me, I assume that'll also fix the crake. It was already on a > list of patches for pluggable storage... > > Btw, shouldn't redisExecForeignDelete return NULL when there's no row > matched for the deletion? That's possible, I'd assume, if there's a > concurrent deletion. > Yes, probably. Thanks. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services