trigger example for plsample

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

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.

Regards,
-Chap

The new status of this patch is: Waiting on Author

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

On 03/02/22 15:12, Mark Wong wrote:

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 the simplifying assumption of a simple interpreted language allows
a lot more of this code to go away. I mean more or less that whole first
PG_TRY...PG_END_TRY block, which could be replaced with a comment saying
something like

The source text may be augmented here, such as by wrapping it as the
body of a function in the target language, prefixing a parameter list
with names like TD_name, TD_relid, TD_table_name, TD_table_schema,
TD_event, TD_when, TD_level, TD_NEW, TD_OLD, and args, using whatever
types in the target language are convenient. The augmented text can be
cached in a longer-lived memory context, or, if the target language uses
a compilation step, that can be done here, caching the result of the
compilation.

That would leave only the later PG_TRY block where the function gets
"executed". That could stay largely as is, but should probably also have
a comment within it, something like

Here the function (the possibly-augmented source text, or the result
of compilation if the target language uses such a step) should be
executed, after binding these values from the TriggerData struct to
the expected parameters.

That should make the example shorter and clearer, and preserve the output
tested by the regression test. Trying to show much more than that involves
assumptions about what the PL's target language syntax looks like, how its
execution engine works and parameters are bound, and so on, and that is
likely to just be distracting, IMV.

Regards,
-Chap

On Thu, Mar 10, 2022 at 06:36:44PM -0500, Chapman Flack wrote:

On 03/02/22 15:12, Mark Wong wrote:

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 the simplifying assumption of a simple interpreted language allows
a lot more of this code to go away. I mean more or less that whole first
PG_TRY...PG_END_TRY block, which could be replaced with a comment saying
something like

The source text may be augmented here, such as by wrapping it as the
body of a function in the target language, prefixing a parameter list
with names like TD_name, TD_relid, TD_table_name, TD_table_schema,
TD_event, TD_when, TD_level, TD_NEW, TD_OLD, and args, using whatever
types in the target language are convenient. The augmented text can be
cached in a longer-lived memory context, or, if the target language uses
a compilation step, that can be done here, caching the result of the
compilation.

That would leave only the later PG_TRY block where the function gets
"executed". That could stay largely as is, but should probably also have
a comment within it, something like

Here the function (the possibly-augmented source text, or the result
of compilation if the target language uses such a step) should be
executed, after binding these values from the TriggerData struct to
the expected parameters.

That should make the example shorter and clearer, and preserve the output
tested by the regression test. Trying to show much more than that involves
assumptions about what the PL's target language syntax looks like, how its
execution engine works and parameters are bound, and so on, and that is
likely to just be distracting, IMV.

I think I've applied all of these suggestions and attached a new patch.

Regards,
Mark

On Wed, Apr 6, 2022 at 5:44 PM Mark Wong <markwkm@gmail.com> wrote:

On Thu, Mar 10, 2022 at 06:36:44PM -0500, Chapman Flack wrote:

On 03/02/22 15:12, Mark Wong wrote:

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 the simplifying assumption of a simple interpreted language

allows

a lot more of this code to go away. I mean more or less that whole first
PG_TRY...PG_END_TRY block, which could be replaced with a comment saying
something like

The source text may be augmented here, such as by wrapping it as the
body of a function in the target language, prefixing a parameter list
with names like TD_name, TD_relid, TD_table_name, TD_table_schema,
TD_event, TD_when, TD_level, TD_NEW, TD_OLD, and args, using whatever
types in the target language are convenient. The augmented text can be
cached in a longer-lived memory context, or, if the target language

uses

a compilation step, that can be done here, caching the result of the
compilation.

That would leave only the later PG_TRY block where the function gets
"executed". That could stay largely as is, but should probably also have
a comment within it, something like

Here the function (the possibly-augmented source text, or the result
of compilation if the target language uses such a step) should be
executed, after binding these values from the TriggerData struct to
the expected parameters.

That should make the example shorter and clearer, and preserve the

output

tested by the regression test. Trying to show much more than that

involves

assumptions about what the PL's target language syntax looks like, how

its

execution engine works and parameters are bound, and so on, and that is
likely to just be distracting, IMV.

I think I've applied all of these suggestions and attached a new patch.

Cool... I also have a look into the code. To me everything is also ok!!!

Regards,

--
Fabrízio de Royes Mello

On 2022-04-06 16:44, Mark Wong wrote:

I think I've applied all of these suggestions and attached a new patch.

That looks good to me, though I wonder about the pfree(source).
In the simplest case of a PL that uses no advance compilation or
augmentation step, the Code Execution block might naturally refer
to source, so perhaps the example boilerplate shouldn't include
a pfree that needs to be removed in that case.

In fact, I wonder about the need for any retail pfree()s here. Those
added in this patch are the only ones in plsample.c. They are small
allocations, and maybe it would both streamline the example to leave
out the pfree calls, and be an illustration of best practice in letting
the memory context machinery handle all the deallocation at once, where
there isn't a special need to free something, like an especially large
allocation, at retail.

Regards,
-Chap

On Thu, Apr 07, 2022 at 10:30:13AM -0400, chap@anastigmatix.net wrote:

On 2022-04-06 16:44, Mark Wong wrote:

I think I've applied all of these suggestions and attached a new patch.

That looks good to me, though I wonder about the pfree(source).
In the simplest case of a PL that uses no advance compilation or
augmentation step, the Code Execution block might naturally refer
to source, so perhaps the example boilerplate shouldn't include
a pfree that needs to be removed in that case.

In fact, I wonder about the need for any retail pfree()s here. Those
added in this patch are the only ones in plsample.c. They are small
allocations, and maybe it would both streamline the example to leave
out the pfree calls, and be an illustration of best practice in letting
the memory context machinery handle all the deallocation at once, where
there isn't a special need to free something, like an especially large
allocation, at retail.

Thanks, I've attached v4.

I've removed all of the pfree()'s and added an elog(DEBUG1) for source
to quiet a compiler warning about source's lack of use. :) (Was that a
good way?)

Regards,
Mark

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

v4 looks good to me.

I don't think this requires any documentation change.
The patch simply adds trigger handling example code to plsample.c,
and plsample is already mentioned in the documentation on writing
a PL handler.

Regards,
-Chap

The new status of this patch is: Ready for Committer

Chapman Flack <chap@anastigmatix.net> writes:

v4 looks good to me.

Pushed with very minor editorialization. Mainly, I undid the
decision to stop printing the function source text, on the
grounds that (1) it falsified the comment immediately above,
and (2) if you have to print it anyway to avoid compiler warnings,
you're just creating confusing inconsistency between the two
handler functions.

regards, tom lane

On Thu, Apr 07, 2022 at 06:29:53PM -0400, Tom Lane wrote:

Chapman Flack <chap@anastigmatix.net> writes:

v4 looks good to me.

Pushed with very minor editorialization. Mainly, I undid the
decision to stop printing the function source text, on the
grounds that (1) it falsified the comment immediately above,
and (2) if you have to print it anyway to avoid compiler warnings,
you're just creating confusing inconsistency between the two
handler functions.

Sounds good to me, thanks!

Regards,
Mark