On Fri, Jan 22, 2021 at 3:22 PM Corey Huinker <corey.huinker@gmail.com> wrote:
>> 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 think that's a nice compromise, it makes the reader aware of the concept.
>>
>> I've attached the updated patch.
>
> Missing "break" added. Check.
> Comment updated. Check.
> Function renamed. Check.
> Attribute mapping matching test (and assertion) added. Check.
> Patch applies to an as-of-today master, passes make check and check world.
> No additional regression tests required, as no new functionality is introduced.
> No docs required, as there is nothing user-facing.
Thanks for the review.
> Questions:
> 1. There's a palloc for mapped_partkey_attnums, which is never freed, is the prevailing memory context short lived
enoughthat we don't care?
> 2. Same question for the AtrrMap map, should there be a free_attrmap().
I hadn't checked, but yes, the prevailing context is
AfterTriggerTupleContext, a short-lived one that is reset for every
trigger event tuple. I'm still inclined to explicitly free those
objects, so changed like that. While at it, I also changed
mapped_partkey_attnums to root_partattrs for readability.
Attached v4.
--
Amit Langote
EDB: http://www.enterprisedb.com