From: Mark Wong <markwkm@gmail.com>
To: Chapman Flack <chap@anastigmatix.net>
Cc: pgsql-hackers@lists.postgresql.org, Konstantina Skovola <konskov@gmail.com>
Subject: Re: trigger example for plsample
Date: Wed, 2 Mar 2022 12:12:01 -0800
Message-ID: <Yh/PkdpVPbeydjC/@workstation-mark-wong> (raw)
In-Reply-To: <164581437973.1035.16436524743292685141.pgcf@coridan.postgresql.org>
References: <Yd8Cz22eHi80XS30@workstation-mark-wong>
<Yd8Cz22eHi80XS30@workstation-mark-wong>
<164581437973.1035.16436524743292685141.pgcf@coridan.postgresql.org>
On Fri, Feb 25, 2022 at 06:39:39PM +0000, Chapman Flack wrote:
> This patch is straightforward, does what it says, and passes the tests.
>
> Regarding the duplication of code between plsample_func_handler and
> plsample_trigger_handler, perhaps that's for the best for now, as 3554 in
> the same commitfest also touches plsample, so merge conflicts may be
> minimized by not doing more invasive refactoring.
>
> That would leave low-hanging fruit for a later patch that could refactor
> plsample to reduce the duplication (maybe adding a validator at the same
> time? That would also duplicate some of the checks in the existing handlers.)
>
> I am not sure that structuring the trigger handler with separate compile and
> execute steps is worth the effort for a simple example like plsample. The main
> plsample_func_handler is not so structured.
>
> It's likely that many real PLs will have some notion of compilation separate from
> execution. But those will also have logic to do the compilation only once, and
> somewhere to cache the result of that for reuse across calls, and those kinds of
> details might make plsample's basic skeleton more complex than needed.
>
> I know that in just looking at expected/plsample.out, I was a little distracted by
> seeing multiple "compile" messages for the same trigger function in the same
> session and wondering why that was.
>
> So maybe it would be simpler and less distracting to assume that the PL targeted
> by plsample is one that just has a simple interpreter that works from the source text.
I've attached v2, which reduces the output:
* Removing the notices for the text body, and the "compile" message.
* Replaced the notice for "compile" message with a comment as a
placeholder for where a compiling code or checking a cache may go.
* Reducing the number of rows inserted into the table, thus reducing
the number of notice messages about which code path is taken.
I think that reduces the repetitiveness of the output...
Regards,
Mark
Attachments:plsample-trigger-v2.patch (text/plain, 10.0K) downloaddiff --git a/src/test/modules/plsample/expected/plsample.out b/src/test/modules/plsample/expected/plsample.outindex a0c318b6df..a7912c7897 100644--- a/src/test/modules/plsample/expected/plsample.out+++ b/src/test/modules/plsample/expected/plsample.out@@ -6,9 +6,6 @@ AS $$
Example of source with text result.
$$ LANGUAGE plsample;
SELECT plsample_result_text(1.23, 'abc', '{4, 5, 6}');
-NOTICE: source text of function "plsample_result_text": - Example of source with text result.-
NOTICE: argument: 0; name: a1; value: 1.23
NOTICE: argument: 1; name: a2; value: abc
NOTICE: argument: 2; name: a3; value: {4,5,6}
@@ -25,12 +22,57 @@ AS $$
Example of source with void result.
$$ LANGUAGE plsample;
SELECT plsample_result_void('{foo, bar, hoge}');
-NOTICE: source text of function "plsample_result_void": - Example of source with void result.-
NOTICE: argument: 0; name: a1; value: {foo,bar,hoge}
plsample_result_void
----------------------
(1 row)
+CREATE FUNCTION my_trigger_func() RETURNS trigger AS $$+if TD_event == "INSERT"+ return TD_NEW+elseif TD_event == "UPDATE"+ return TD_NEW+else+ return "OK"+end+$$ language plsample;+CREATE TABLE my_table (num integer, description text);+CREATE TRIGGER my_trigger_func BEFORE INSERT OR UPDATE ON my_table+ FOR EACH ROW EXECUTE FUNCTION my_trigger_func();+CREATE TRIGGER my_trigger_func2 AFTER INSERT OR UPDATE ON my_table+ FOR EACH ROW EXECUTE FUNCTION my_trigger_func(8);+INSERT INTO my_table (num, description)+VALUES (1, 'first');+NOTICE: trigger name: my_trigger_func+NOTICE: trigger relation: my_table+NOTICE: trigger relation schema: public+NOTICE: triggered by INSERT+NOTICE: triggered BEFORE+NOTICE: triggered per row+NOTICE: trigger name: my_trigger_func2+NOTICE: trigger relation: my_table+NOTICE: trigger relation schema: public+NOTICE: triggered by INSERT+NOTICE: triggered AFTER+NOTICE: triggered per row+NOTICE: trigger arg[0]: 8+UPDATE my_table+SET description = 'first, modified once'+WHERE num = 1;+NOTICE: trigger name: my_trigger_func+NOTICE: trigger relation: my_table+NOTICE: trigger relation schema: public+NOTICE: triggered by UPDATE+NOTICE: triggered BEFORE+NOTICE: triggered per row+NOTICE: trigger name: my_trigger_func2+NOTICE: trigger relation: my_table+NOTICE: trigger relation schema: public+NOTICE: triggered by UPDATE+NOTICE: triggered AFTER+NOTICE: triggered per row+NOTICE: trigger arg[0]: 8+DROP TRIGGER my_trigger_func ON my_table;+DROP TRIGGER my_trigger_func2 ON my_table;+DROP FUNCTION my_trigger_func;diff --git a/src/test/modules/plsample/plsample.c b/src/test/modules/plsample/plsample.cindex 6fc33c728c..fea266f522 100644--- a/src/test/modules/plsample/plsample.c+++ b/src/test/modules/plsample/plsample.c@@ -19,6 +19,7 @@
#include "catalog/pg_type.h"
#include "commands/event_trigger.h"
#include "commands/trigger.h"
+#include "executor/spi.h"
#include "funcapi.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
@@ -29,6 +30,7 @@ PG_MODULE_MAGIC;
PG_FUNCTION_INFO_V1(plsample_call_handler);
static Datum plsample_func_handler(PG_FUNCTION_ARGS);
+static HeapTuple plsample_trigger_handler(PG_FUNCTION_ARGS);
/*
* Handle function, procedure, and trigger calls.
@@ -51,6 +53,7 @@ plsample_call_handler(PG_FUNCTION_ARGS)
* (TriggerData *) fcinfo->context includes the information of the
* context.
*/
+ retval = PointerGetDatum(plsample_trigger_handler(fcinfo));
}
else if (CALLED_AS_EVENT_TRIGGER(fcinfo))
{
@@ -119,9 +122,6 @@ plsample_func_handler(PG_FUNCTION_ARGS)
elog(ERROR, "could not find source text of function \"%s\"",
proname);
source = DatumGetCString(DirectFunctionCall1(textout, ret));
- ereport(NOTICE,- (errmsg("source text of function \"%s\": %s",- proname, source)));
/*
* Allocate a context that will hold all the Postgres data for the
@@ -185,3 +185,176 @@ plsample_func_handler(PG_FUNCTION_ARGS)
ret = InputFunctionCall(&result_in_func, source, result_typioparam, -1);
PG_RETURN_DATUM(ret);
}
++/*+ * plsample_trigger_handler+ *+ * Function called by the call handler for trigger execution.+ */+static HeapTuple+plsample_trigger_handler(PG_FUNCTION_ARGS)+{+ TriggerData *trigdata = (TriggerData *) fcinfo->context;+ char *string;+ volatile HeapTuple rettup;+ HeapTuple pl_tuple;+ Datum ret;+ char *source;+ bool isnull;+ Form_pg_proc pl_struct;+ char *proname;+ int rc PG_USED_FOR_ASSERTS_ONLY;+ MemoryContext proc_cxt = NULL;+ MemoryContext old_cxt = NULL;+ char *trigger_code = NULL;++ /* Make sure this is being called from a trigger. */+ if (!CALLED_AS_TRIGGER(fcinfo))+ elog(ERROR, "not called by trigger manager");++ /* Connect to the SPI manager */+ if (SPI_connect() != SPI_OK_CONNECT)+ elog(ERROR, "could not connect to SPI manager");++ rc = SPI_register_trigger_data(trigdata);+ Assert(rc >= 0);++ /* Fetch the source text of the function. */+ pl_tuple = SearchSysCache(PROCOID,+ ObjectIdGetDatum(fcinfo->flinfo->fn_oid), 0, 0, 0);+ if (!HeapTupleIsValid(pl_tuple))+ elog(ERROR, "cache lookup failed for function %u",+ fcinfo->flinfo->fn_oid);++ /*+ * Extract and print the source text of the function. This can be used as+ * a base for the function validation and execution.+ */+ pl_struct = (Form_pg_proc) GETSTRUCT(pl_tuple);+ proname = pstrdup(NameStr(pl_struct->proname));+ ret = SysCacheGetAttr(PROCOID, pl_tuple, Anum_pg_proc_prosrc, &isnull);+ if (isnull)+ elog(ERROR, "could not find source text of function \"%s\"",+ proname);+ source = DatumGetCString(DirectFunctionCall1(textout, ret));++ PG_TRY();+ {+ char *internal_args = "TD_name, TD_relid, TD_table_name, TD_table_schema, TD_event, TD_when, TD_level, TD_NEW, TD_OLD, args";++ /*+ * The code generated for this trigger is: proname(internal_args)+ * source+ */+ int length =+ strlen(proname) + strlen(source) + strlen(internal_args) + 4;++ /*+ * Allocate a context that will hold all the Postgres data for the+ * procedure.+ */+ proc_cxt = AllocSetContextCreate(TopMemoryContext,+ "PL/Sample function", ALLOCSET_SMALL_SIZES);++ trigger_code = (char *) palloc0(length * sizeof(char));+ trigger_code[0] = '\0';+ strcpy(trigger_code, proname);+ strcat(trigger_code, "(");+ strcat(trigger_code, internal_args);+ strcat(trigger_code, ") ");+ strcat(trigger_code, source);++ /*+ * Compile trigger code here, or lookup cache of compiled code if+ * necessary.+ */++ old_cxt = MemoryContextSwitchTo(proc_cxt);++ MemoryContextSwitchTo(old_cxt);+ }+ PG_CATCH();+ {+ PG_RE_THROW();+ }+ PG_END_TRY();++ pfree(source);++ ReleaseSysCache(pl_tuple);++ PG_TRY();+ {+ int i;++ ereport(NOTICE,+ (errmsg("trigger name: %s", trigdata->tg_trigger->tgname)));+ string = SPI_getrelname(trigdata->tg_relation);+ ereport(NOTICE, (errmsg("trigger relation: %s", string)));+ pfree(string);++ string = SPI_getnspname(trigdata->tg_relation);+ ereport(NOTICE, (errmsg("trigger relation schema: %s", string)));+ pfree(string);++ /* Example handling of different trigger aspects. */++ if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event))+ {+ ereport(NOTICE, (errmsg("triggered by INSERT")));+ rettup = trigdata->tg_trigtuple;+ }+ else if (TRIGGER_FIRED_BY_DELETE(trigdata->tg_event))+ {+ ereport(NOTICE, (errmsg("triggered by DELETE")));+ rettup = trigdata->tg_trigtuple;+ }+ else if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))+ {+ ereport(NOTICE, (errmsg("triggered by UPDATE")));+ rettup = trigdata->tg_trigtuple;+ }+ else if (TRIGGER_FIRED_BY_TRUNCATE(trigdata->tg_event))+ {+ ereport(NOTICE, (errmsg("triggered by TRUNCATE")));+ rettup = trigdata->tg_trigtuple;+ }+ else+ elog(ERROR, "unrecognized event: %u", trigdata->tg_event);++ if (TRIGGER_FIRED_BEFORE(trigdata->tg_event))+ ereport(NOTICE, (errmsg("triggered BEFORE")));+ else if (TRIGGER_FIRED_AFTER(trigdata->tg_event))+ ereport(NOTICE, (errmsg("triggered AFTER")));+ else if (TRIGGER_FIRED_INSTEAD(trigdata->tg_event))+ ereport(NOTICE, (errmsg("triggered INSTEAD OF")));+ else+ elog(ERROR, "unrecognized when: %u", trigdata->tg_event);++ if (TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))+ ereport(NOTICE, (errmsg("triggered per row")));+ else if (TRIGGER_FIRED_FOR_STATEMENT(trigdata->tg_event))+ ereport(NOTICE, (errmsg("triggered per statement")));+ else+ elog(ERROR, "unrecognized level: %u", trigdata->tg_event);++ /*+ * Iterate through all of the trigger arguments, printing each input+ * value.+ */+ for (i = 0; i < trigdata->tg_trigger->tgnargs; i++)+ ereport(NOTICE,+ (errmsg("trigger arg[%i]: %s", i,+ trigdata->tg_trigger->tgargs[i])));+ }+ PG_CATCH();+ {+ PG_RE_THROW();+ }+ PG_END_TRY();++ if (SPI_finish() != SPI_OK_FINISH)+ elog(ERROR, "SPI_finish() failed");++ return rettup;+}diff --git a/src/test/modules/plsample/sql/plsample.sql b/src/test/modules/plsample/sql/plsample.sqlindex bf0fddac7f..ac06e99643 100644--- a/src/test/modules/plsample/sql/plsample.sql+++ b/src/test/modules/plsample/sql/plsample.sql@@ -13,3 +13,31 @@ AS $$
Example of source with void result.
$$ LANGUAGE plsample;
SELECT plsample_result_void('{foo, bar, hoge}');
++CREATE FUNCTION my_trigger_func() RETURNS trigger AS $$+if TD_event == "INSERT"+ return TD_NEW+elseif TD_event == "UPDATE"+ return TD_NEW+else+ return "OK"+end+$$ language plsample;++CREATE TABLE my_table (num integer, description text);+CREATE TRIGGER my_trigger_func BEFORE INSERT OR UPDATE ON my_table+ FOR EACH ROW EXECUTE FUNCTION my_trigger_func();+CREATE TRIGGER my_trigger_func2 AFTER INSERT OR UPDATE ON my_table+ FOR EACH ROW EXECUTE FUNCTION my_trigger_func(8);++INSERT INTO my_table (num, description)+VALUES (1, 'first');++UPDATE my_table+SET description = 'first, modified once'+WHERE num = 1;++DROP TRIGGER my_trigger_func ON my_table;+DROP TRIGGER my_trigger_func2 ON my_table;+DROP FUNCTION my_trigger_func;+
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: markwkm@gmail.com, chap@anastigmatix.net, konskov@gmail.com
Subject: Re: trigger example for plsample
In-Reply-To: <Yh/PkdpVPbeydjC/@workstation-mark-wong>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox