On Tue, Jan 19, 2021 at 3:46 PM Corey Huinker <corey.huin...@gmail.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/cakkq508z6r5e3jdqhfpwszsajlpho3oyyoamfesaupto5vg...@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
v3-0002-Avoid-using-SPI-for-some-RI-checks.patch
Description: Binary data
v3-0001-Export-get_partition_for_tuple.patch
Description: Binary data
