=?UTF-8?B?0JDQvdGC0YPQsNC9INCS0LjQvtC70LjQvQ==?= <violin.antuan@gmail.com> writes:
> I made patch for this problem, I changed event_trigger, for
> definitions of temporality
> DEFAULT VALUE
I looked over this patch. I understand what you want to fix,
and I agree with the general plan of pushing the namespace-lookup
code into a subroutine so we can more easily point it at a different
object for the attrdef case. However, you've done no favors for
future readers of the code:
* "process_catalog_object" is about as generic and uninformative a
name as one could easily think of. I'd suggest something more like
"identify_object_namespace" --- I'm not wedded to that exact choice,
but the name should indicate what the function is trying to do.
* The new function also lacks a header comment, which isn't OK
except maybe for extremely trivial functions with obvious APIs.
Here I think you need to explain what values it outputs, and
you certainly need to explain what the return value means.
* The comment block at lines 1297ff is now kind of dangling,
because you moved half of what it's talking about to somewhere else.
Perhaps some of that text belongs in the new function's header
comment.
* Zero comments in the new code block for "object->classId ==
AttrDefaultRelationId" are not OK either. I'd expect to see
something like "We treat a column default as temp if its table
is temp".
* I wonder if the check for is_objectclass_supported shouldn't
move into the new function too. It's not really a concern
of the outer function where the new function is getting its
information from.
* If I'm reading it correctly, the patch depends on the
assumption that attrdefs aren't supported by the
is_objectclass_supported() infrastructure. I'm not sure
that's right even today, and it sure seems like something
that could get broken by well-intentioned future patches.
Something that isn't the fault of your patch, but could be
improved while we're here:
* It seems rather messy and poorly-thought-out that schemas
themselves are handled in two separate places in the function,
at lines 1283ff and 1367ff. Seems like that could be unified
and also made to look more like the equivalent code for
objects-contained-in-schemas.
Taking my last three comments together, maybe what we want for
the overall structure in EventTriggerSQLDropAddObject is
if (object->classId == NamespaceRelationId)
{
code for the schema case;
}
else if (object->classId == AttrDefaultRelationId)
{
code for the attrdef case;
}
else
{
generic case;
}
where the second and third blocks use this new function.
regards, tom lane
