Обсуждение: trigger example for plsample

Hi everyone,

I've adapted the work that Konstantina did for pl/julia as part of her
GSOC project to add an example of handling triggers to plsample.  Which
was based from pl/tcl and pl/perl.

One aspect that I'm not sure about is whether the example should be
duplicating code (as it is now) for keeping an example contained within
a single function.  The only reason I can come up with is to try to read
through an example with minimal jumping around.

Hoping this is a good start.

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

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