[fixed] Trigger test

Hi, Hackers!

I was testing a connection pooler with `make installcheck` and noticed
that `check_foreign_key()` from the `refint` library reuses the same
cached plan for cascade `UPDATE`/`DELETE` operations. As a result, a
cascade `DELETE` is applied after an `UPDATE` command on the primary key
table (which should not happen after the commit
https://github.com/postgres/postgres/commit/d489fdfc7f4ccf0010fe0397e7272bdfc257e8f2)
I have attached a file (test.sql) to reproduce an issue and the
solution.

Regards,
Dmitrii Bondar.

Dmitrii Bondar писал(а) 2025-01-29 16:53:
> Hi, Hackers!
>
> I was testing a connection pooler with `make installcheck` and noticed
> that `check_foreign_key()` from the `refint` library reuses the same
> cached plan for cascade `UPDATE`/`DELETE` operations. As a result, a
> cascade `DELETE` is applied after an `UPDATE` command on the primary
> key table (which should not happen after the commit
> https://github.com/postgres/postgres/commit/d489fdfc7f4ccf0010fe0397e7272bdfc257e8f2)
> I have attached a file (test.sql) to reproduce an issue and the
> solution.
>
> Regards,
> Dmitrii Bondar.
Found a mistake. Now it should work even if the SPI call fails (v2
attachment). However, the whole solution looks awkward because if
`check_primary_key` is triggered by a function other than
check_foreign_key, we still encounter invalid behavior. The root of the
problem is the inability to see the row that triggered the initial
`check_foreign_key`.

I am also considering another solution (v3 attachment): instead of using
static variables, restrict the use of the `check_primary_key` and
`check_foreign` functions in BEFORE triggers so that the
`check_primary_key` trigger can find the new row. This still doesn't
solve the problem (a user could create their own BEFORE trigger that
make `UPDATE` and trigger `check_primary_key`), but it adds less new
code, at least.

Regards,
Dmitrii Bondar

Just a rebase.

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 patch is 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 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.

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(at)illuminatedcomputing(dot)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(dot)bondar(at)postgrespro(dot)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

On 04/04/2025 01:11, Tom Lane wrote:
> 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

Dmitrii Bondar <d(dot)bondar(at)postgrespro(dot)ru> writes:
> On 04/04/2025 01:11, Tom Lane wrote:
>> 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.

> 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'm not terribly concerned about whether these triggers have perfect
foreign-key semantics, since no one (in their right mind) would use
them as foreign-key enforcement anyway. What they're good for
is as examples of writing checks and updates in C-coded triggers.
As such, questions like "are identifiers and data values quoted
appropriately" seem far more urgent than whether cascade update
works per spec. Even just using a StringInfo rather than a fixed-size
char[] variable to build the query in would be an improvement.

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

I'm not that thrilled with giving up on refint.c either. But in its
current state, it's a pretty lousy example. Are we willing to put
enough effort into making it a more useful code example?

regards, tom lane

I wrote:
> Dmitrii Bondar <d(dot)bondar(at)postgrespro(dot)ru> writes:
>> 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).

> I'm not that thrilled with giving up on refint.c either. But in its
> current state, it's a pretty lousy example. Are we willing to put
> enough effort into making it a more useful code example?

I concluded that we might as well commit what we've got, since the
window for v18 is closing fast and there's not time to consider
anything more invasive. Hence, pushed. I hope you will consider
spending some effort on fixing the other problems identified in
this thread.

regards, tom lane