Re: [fixed] Trigger test - Mailing list pgsql-hackers

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