Thread: Re: [fixed] Trigger test

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

Hi Dmitrii,

Paul Jungwirth and I reviewed this patch, and here are our comments:

1. The patch applies and tests pass.
2. The patch fixes a bug in contrib/spi, which is not really a practical extension, but rather examples of how to use
SPI.The contrib/spi directory actually has four extensions: refint, autoinc, insert_username, and moddatetime. The
patchis for refint, which is a way you could implement foreign keys if it weren't built in to Postgres.
 
3. Consider updating documentation for doc/src/contrib-spi.sgml, or any file as appropriate, to reflect the changes.
4. Are there any cases where check_primary_key() and check_foreign_key() should be called using a BEFORE trigger? Will
thischange break backwards compatibility? Consider adding a test with a BEFORE trigger to ensure the error "must be
firedby AFTER trigger" is raised.
 

Thank you!

The new status of this patch is: Waiting on Author

Hi!

Thank you for the review!

> 3. Consider updating documentation for doc/src/contrib-spi.sgml, or any 
> file as appropriate, to reflect the changes.

The changes have now been added to doc/src/contrib-spi.sgml. I also 
added a consideration note about interactions with BEFORE triggers.

> 4. Are there any cases where check_primary_key() and 
> check_foreign_key() should be called using a BEFORE trigger? Will this 
> change break backwards compatibility? Consider adding a test with a 
> BEFORE trigger to ensure the error "must be fired by AFTER trigger" is 
> raised.

The usage within BEFORE triggers appears to be entirely incorrect. The 
functions check_primary_key() and check_foreign_key() are intended for 
use in creating constraint triggers, which according to the 
documentation must be AFTER ROW triggers. Therefore, any cases using 
BEFORE triggers are invalid.

I have updated the test so that it now raises the error "must be fired 
by AFTER trigger."

Can you also help me with the patch status? What status should I move 
the patch to?

Hi Dmitrii,

Thanks for the quick update!

On 3/26/25 02:45, Dmitrii Bondar wrote:
 >> 3. Consider updating documentation for doc/src/contrib-spi.sgml, or any file as appropriate, to
 >> reflect the changes.
 >
 > The changes have now been added to doc/src/contrib-spi.sgml. I also added a consideration note about
 > interactions with BEFORE triggers.

This looks good. I have a couple small grammar suggestions. This:

+   To use, create a <literal>AFTER INSERT OR UPDATE</literal> trigger using this

should be:

+   To use, create an <literal>AFTER INSERT OR UPDATE</literal> trigger using this

and this:

+   To use, create a <literal>AFTER DELETE OR UPDATE</literal> trigger using this

should be this:

+   To use, create an <literal>AFTER DELETE OR UPDATE</literal> trigger using this

Also re this part of the patch:

@@ -592,10 +598,15 @@ check_foreign_key(PG_FUNCTION_ARGS)
     }
     else
     {
+     const char* operation;
+
+     if (action == 'c')
+       operation = is_update ? "updated" : "deleted";
+     else
+       operation = "set to null";
  #ifdef REFINT_VERBOSE
       elog(NOTICE, "%s: " UINT64_FORMAT " tuple(s) of %s are %s",
-        trigger->tgname, SPI_processed, relname,
-        (action == 'c') ? "deleted" : "set to null");
+        trigger->tgname, SPI_processed, relname, operation);
  #endif
     }
     args += nkeys + 1;    /* to the next relation */

We can put all the new lines inside the #ifdef, can't we?

 > Can you also help me with the patch status? What status should I move the patch to?

I think if you make those changes we should mark this as Ready for Committer.

Yours,

-- 
Paul              ~{:-)
pj@illuminatedcomputing.com

Hi, Paul,

Thanks for the suggestions.

 > This looks good. I have a couple small grammar suggestions. This:

I have replaced the incorrect articles with the correct ones.

 > We can put all the new lines inside the #ifdef, can't we?

You're right. I have done that.

Best regards,

Dmitrii

Dmitrii Bondar <d.bondar@postgrespro.ru> writes:
> [ v6-0001-Triggers-test-fix-with-the-invalid-cache-in-refin.patch ]

I spent a little bit of time looking over this patch.  My first
instinct was "we can't really change the recommended method of
installing these triggers" --- but that would only matter if we
thought there were actual production users of these triggers,
which surely there are not (anymore).  The only reason we're
carrying refint.c at all is as an example of C-coded triggers.
So changing the example seems fine, and you're right that this
sort of change is far better done from an AFTER trigger.

However ... as an example refint.c is pretty darn awful :-(.
I'd never looked hard at it before, and now that I have,
I'm rather horrified, particularly by the shoddy quoting practices.
None of the identifiers inserted into constructed queries receive
quote_identifier() protection, and the insertion of data values
around line 500 is beyond awful.

So while I think your v6 patch fixes the problem(s) it set out
to fix, it still feels a lot like putting lipstick on a pig.
I wonder if we'd be better off to nuke refint.c altogether.
If we don't, I feel like we're morally obliged to spend more
effort trying to make it less of an example of bad practices.
Some of the things I think need to be cleaned up:

* It's ridiculous that the update-cascade case is inserting
data values textually at all.  Even if it were quoting them
properly, that's expensive and risks round-trip-conversion
problems.  That should be handled as an additional Param value
passed into the statement.

* Worse yet, that code doesn't work if used more than once,
because the first value that needs to be updated gets baked
into the plan that will be re-used later.  So the Param
approach is really essential even aside from quoting concerns.

* String comparisons to detect value equality (around line 400)
are not terribly cool either.  Proper code would be looking up
the default equality operator for the datatypes and applying that.

* Some thought needs to be given to table and column names that
require quoting.  I guess in a way there's an advantage to not
quoting the table names that come from trigger arguments: it
lets the user get around the module's failure to think about
schema-qualified names, by writing 'foo.bar' rather than just
'bar'.  But that's not documented.  If we did quote everything
then we'd really have to go over to providing separate schema
and name arguments for each of the other tables.  In any case,
not quoting the column names has nothing to recommend it.

* I'll slide gently past the question of whether this should
be expected to react on-the-fly to DDL changes in the tables.
SPI will do some of that under the hood, but it can't fix
cases where the query string would need to change (eg.
table or column renames).

So that's a long laundry list and we haven't even dug hard.
Is it worth it?  If you feel like doing the legwork then
I'm willing to support the project, but I really wonder if
we shouldn't cut our losses and just remove the module.

(I hesitate now to look at the rest of contrib/spi/ :-()

            regards, tom lane

So that's a long laundry list and we haven't even dug hard.
Is it worth it?  If you feel like doing the legwork then
I'm willing to support the project, but I really wonder if
we shouldn't cut our losses and just remove the module.

(I hesitate now to look at the rest of contrib/spi/ :-()

You wrote a note that I decided to omit. As I mentioned, the patch does not even fix the cascade update problem—there are still broken cases—because it seems impossible to address it in a gentle way (the code was patched 20 years ago; it's truly legacy).

I considered removing it entirely, but that seemed too drastic a solution (and, at the very least, I don't have enough expertise to make that decision). If everything looks acceptable, I would prefer to cut the module. The check_primary_key and check_foreign functions are clearly unused, are buggy, and no one has reported any obvious problems—so refint.c can be safely removed. Autoinc.c also looks problematic.

There are some question. When should we remove the module? Should we mark it as deprecated for now and remove it later? Should we handle it in another thread? Should we apply this patch in that case?

Best regards,

Dmitrii