Re: Unify drop-by-OID functions - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Unify drop-by-OID functions
Date
Msg-id CAEudQAoNVXCqJ8bXCDJVqnAqRZoDFQjNqK+_csR7naO8357=uQ@mail.gmail.com
Whole thread Raw
In response to Re: Unify drop-by-OID functions  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: Unify drop-by-OID functions
List pgsql-hackers
Em ter., 5 de mai. de 2020 às 14:57, Robert Haas <robertmhaas@gmail.com> escreveu:
On Tue, May 5, 2020 at 1:43 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
> I see, the famous "cliché".

By using the word cliché, and by putting it in quotes, you seem to
suggest that you consider my argument dubious. However, I stand by it.
Code shouldn't be changed without understanding the reasons behind the
current coding. Doing so very often breaks things.
Sorry Robert, It was not my intention. I didn't know that using quotes would change your understanding.
Of course, I find your arguments very valid and valuable.
And I understand that there are many interrelated things, which can break if done in the wrong order.
Maybe I used the wrong word, in this case, the cliché.
What I mean is, the cliché, does some strange things, like leaving variables to be declared, assigned in and not used.
And in that specific case, leaving resources blocked, which perhaps, in my humble opinion, could be released quickly.
I think the expected behavior is being the same, with the changes I proposed, IMHO.

+static void
+DropObjectById(const ObjectAddress *object)
+{
+ int cacheId;
+ Relation rel;
+ HeapTuple tup;
+
+ cacheId = get_object_catcache_oid(object->classId);
+
+ /*
+ * Use the system cache for the oid column, if one exists.
+ */
+ if (cacheId >= 0)
+ {
+ tup = SearchSysCache1(cacheId, ObjectIdGetDatum(object->objectId));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+    rel = table_open(object->classId, RowExclusiveLock);
+ CatalogTupleDelete(rel, &tup->t_self);
+    table_close(rel, RowExclusiveLock);
+
+ ReleaseSysCache(tup);
+ }
+ else
+ {
+ ScanKeyData skey[1];
+ SysScanDesc scan;
+
+ ScanKeyInit(&skey[0],
+ get_object_attnum_oid(object->classId),
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(object->objectId));
+
+    rel = table_open(object->classId, RowExclusiveLock);
+ scan = systable_beginscan(rel, get_object_oid_index(object->classId), true,
+  NULL, 1, skey);
+
+ /* we expect exactly one match */
+ tup = systable_getnext(scan);
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+
+    table_close(rel, RowExclusiveLock);
+ }
+}

And again, your opinion is very important to me.

best regards,
Ranier Vilela

pgsql-hackers by date:

Previous
From: Bruce Momjian
Date:
Subject: Re: PG 13 release notes, first draft
Next
From: Bruce Momjian
Date:
Subject: Re: PG 13 release notes, first draft