Re: simplifying foreign key/RI checks

On Tue, Jan 19, 2021 at 3:46 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
> v2 patch applies and passes make check and make check-world. Perhaps, given the missing break at line 418 without any tests failing, we could add another regression test if we're into 100% code path coverage. As it is, I think the compiler warning was a sufficient alert.

Thanks for the review. I will look into checking the coverage.

> The code is easy to read, and the comments touch on the major points of what complexities arise from partitioned tables.
>
> A somewhat pedantic complaint I have brought up off-list is that this patch continues the pattern of the variable and function names making the assumption that the foreign key is referencing the primary key of the referenced table. Foreign key constraints need only reference a unique index, it doesn't have to be the primary key. Granted, that unique index is behaving exactly as a primary key would, so conceptually it is very similar, but keeping with the existing naming (pk_rel, pk_type, etc) can lead a developer to think that it would be just as correct to find the referenced relation and get the primary key index from there, which would not always be correct. This patch correctly grabs the index from the constraint itself, so no problem there.

I decided not to deviate from pk_ terminology so that the new code
doesn't look too different from the other code in the file. Although,
I guess we can at least call the main function
ri_ReferencedKeyExists() instead of ri_PrimaryKeyExists(), so I've
changed that.

> I like that this patch changes the absolute minimum of the code in order to get a very significant performance benefit. It does so in a way that should reduce resource pressure found in other places [1]. This will in turn reduce the performance penalty of "doing the right thing" in terms of defining enforced foreign keys. It seems to get a clearer performance boost than was achieved with previous efforts at statement level triggers.
>
> [1] https://www.postgresql.org/message-id/CAKkQ508Z6r5e3jdqhfPWSzSajLpHo3OYYOAmfeSAuPTo5VGfgw@mail.gmail.com

Thanks. I hadn't noticed [1] before today, but after looking it over,
it seems that what is being proposed there can still be of use. As
long as SPI is used in ri_trigger.c, it makes sense to consider any
tweaks addressing its negative impact, especially if they are not very
invasive. There's this patch too from the last month:
https://commitfest.postgresql.org/32/2930/

> This patch completely sidesteps the DELETE case, which has more insidious performance implications, but is also far less common, and whose solution will likely be very different.

Yeah, we should continue looking into the ways to make referenced-side
RI checks be less bloated.

I've attached the updated patch.

--
Amit Langote
EDB: http://www.enterprisedb.com