Unify drop-by-OID functions

pá 1. 5. 2020 v 16:39 odesílatel Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> napsal:

[proposal for PG 14]

There are a number of Remove${Something}ById() functions that are
essentially identical in structure and only different in which catalog
they are working on. This patch refactors this to be one generic
function. The information about which oid column, index, etc. to use
was already available in ObjectProperty for most catalogs, in a few
cases it was easily added.

Conceivably, this could be taken further by categorizing more special
cases as ObjectProperty fields or something like that, but this seemed
like a good balance.

+1

nice

Pavel

On Fri, May 1, 2020 at 10:51 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

+1

+1 from me, too, but I have a few suggestions:

+DropGenericById(const ObjectAddress *object)

How about "Generic" -> "Object" or "Generic" -> "ObjectAddress"?

+ elog(ERROR, "cache lookup failed for %s entry %u",
+ elog(ERROR, "could not find tuple for class %u entry %u",

How about "entry" -> "with OID"?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Em sex., 1 de mai. de 2020 às 11:39, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> escreveu:

[proposal for PG 14]

There are a number of Remove${Something}ById() functions that are
essentially identical in structure and only different in which catalog
they are working on. This patch refactors this to be one generic
function. The information about which oid column, index, etc. to use
was already available in ObjectProperty for most catalogs, in a few
cases it was easily added.

Conceivably, this could be taken further by categorizing more special
cases as ObjectProperty fields or something like that, but this seemed
like a good balance.

Very good.

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for
the row to be blocked as little as possible and close the table as quickly
as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

I think that lock resources, for as little time as possible, it is an
advantage..

+static void
+DropGenericById(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 entry %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 class %u entry %u",
+ object->classId, object->objectId);
+
+ CatalogTupleDelete(rel, &tup->t_self);
+ systable_endscan(scan);
+        table_close(rel, RowExclusiveLock);
+ }
+}
+

regards,
Ranier Vilela

On 2020-05-01 23:31, Ranier Vilela wrote:

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for
the row to be blocked as little as possible and close the table as
quickly as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

What do you mean by "object is cached"?

In any case, this is a refactoring patch, so significant changes to the
internal logic would not really be in scope.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Em sáb., 2 de mai. de 2020 às 05:01, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> escreveu:

On 2020-05-01 23:31, Ranier Vilela wrote:

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for
the row to be blocked as little as possible and close the table as
quickly as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

What do you mean by "object is cached"?

Well, that's what I deduced from the cacheId variable name.

regards,
Ranier Vilela

On 2020-05-01 17:44, Robert Haas wrote:

On Fri, May 1, 2020 at 10:51 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:

+1

+1 from me, too, but I have a few suggestions:

+DropGenericById(const ObjectAddress *object)

How about "Generic" -> "Object" or "Generic" -> "ObjectAddress"?

Changed to "Object", that also matches existing functions that operate
on an ObjectAddress.

+ elog(ERROR, "cache lookup failed for %s entry %u",
+ elog(ERROR, "could not find tuple for class %u entry %u",

How about "entry" -> "with OID"?

I changed these to just

"cache lookup failed for %s %u"
"could not find tuple for %s %u"

which matches the existing wording for the not-refactored cases. I
don't recall why I went and reworded them.

New patch attached. I'll park it until PG14 opens.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, May 1, 2020 at 5:32 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for the row to be blocked as little as possible and close the table as quickly as possible.
2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

I think that lock resources, for as little time as possible, it is an advantage..

Only if it's correct, which (3) definitely wouldn't be, and I'm
doubtful about (1) as well.

This reminds me: I think that the issues in
/messages/by-id/CA+TgmoYaFYRRdRZ94p_Qdt+1oONg6sMOvbkGHKVsFtONCrFkhw@mail.gmail.com
should be considered here - we should guarantee that there's a
snapshot registered continuously from before the call to
SearchSysCache1 until after the call to CatalogTupleDelete. In the
systable_beginscan case, we should be fine as long as the
systable_endscan follows the CatalogTupleDelete call.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Em ter., 5 de mai. de 2020 às 13:06, Robert Haas <robertmhaas@gmail.com>
escreveu:

On Fri, May 1, 2020 at 5:32 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

I can suggest improvements?

1. In case Object is cached, delay open_table until the last moment, for

the row to be blocked as little as possible and close the table as quickly
as possible.

2. In case Object is cached and the tuple is invalid, do not open table.
3. Otherwise, is it possible to call systable_endscan, after table_close?

I think that lock resources, for as little time as possible, it is an

advantage..

Only if it's correct, which (3) definitely wouldn't be, and I'm
doubtful about (1) as well.

Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must
call table_close, after systable_endscan.
Now (1) and (2), I would have no hesitation in using it.
I work with ERP, and throughout the time, the later, lock resources and
release them soon, the better, for the performance of the system as a whole.
Even if it doesn't make much difference locally, using this process,
throughout the system, efficiency is noticeable.
Apparently, it is more code, but it is less resources used and for less
time.
And (2), if it is a case, frequently, no table would be blocked in this
function.

Simple examples.

Exemple 1:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
char buf[512];
size_t result;
result = fread(&buf, sizeof(char), 512, f);
fclose(f); // we no longer need the resource, release.
if (result != 0) {
process(buf);
printf("buf=%s\n", buf);
}
}

Exemple 2:
FILE * f;
f = fopen("data.txt", "r");
if (f != NULL) {
char buf[512];
size_t result;
result = fread(&buf, sizeof(char), 512, f);
if (result != 0) {
process(buf);
printf("buf=%s\n", buf);
}
fclose(f); // resource blocked until the end.
}

regards,
Ranier Vilela

On Tue, May 5, 2020 at 1:22 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

Ok, so the question. If (3) is not safe, obvious we shouldn't use, and must call table_close, after systable_endscan.
Now (1) and (2), I would have no hesitation in using it.
I work with ERP, and throughout the time, the later, lock resources and release them soon, the better, for the performance of the system as a whole.
Even if it doesn't make much difference locally, using this process, throughout the system, efficiency is noticeable.
Apparently, it is more code, but it is less resources used and for less time.
And (2), if it is a case, frequently, no table would be blocked in this function.

Nobody here is going to question the concept that it's better to use
resources for less time rather than more, but the wisdom of sticking
to well-established coding patterns instead of inventing altogether
new ones is also well-understood. There are often good reasons why the
code is written in the way that it is, and it's important to
understand those before proposing to change things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Em ter., 5 de mai. de 2020 às 14:29, Robert Haas <robertmhaas@gmail.com>
escreveu:

On Tue, May 5, 2020 at 1:22 PM Ranier Vilela <ranier.vf@gmail.com> wrote:

Ok, so the question. If (3) is not safe, obvious we shouldn't use, and

must call table_close, after systable_endscan.

Now (1) and (2), I would have no hesitation in using it.
I work with ERP, and throughout the time, the later, lock resources and

release them soon, the better, for the performance of the system as a whole.

Even if it doesn't make much difference locally, using this process,

throughout the system, efficiency is noticeable.

Apparently, it is more code, but it is less resources used and for less

time.

And (2), if it is a case, frequently, no table would be blocked in this

function.

Nobody here is going to question the concept that it's better to use
resources for less time rather than more, but the wisdom of sticking
to well-established coding patterns instead of inventing altogether
new ones is also well-understood. There are often good reasons why the
code is written in the way that it is, and it's important to
understand those before proposing to change things.

I see, the famous "cliché".

regards,
Ranier Vilela

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2020-May-05, Ranier Vilela wrote:

And in that specific case, leaving resources blocked, which perhaps, in my
humble opinion, could be released quickly.

I very much doubt that you can measure any difference at all between
these two codings of the function.

I agree with the principle you mention, of not holding resources for
long if they can be released earlier; but in this case the table_close
call occurs across a ReleaseSysCache() call, which is hardly of
significance. It's not like you have to wait for some other
transaction, or wait for I/O, or anything like that that would make it
measurable.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Em ter., 5 de mai. de 2020 às 18:11, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-May-05, Ranier Vilela wrote:

And in that specific case, leaving resources blocked, which perhaps, in

my

humble opinion, could be released quickly.

I very much doubt that you can measure any difference at all between
these two codings of the function.

I agree with the principle you mention, of not holding resources for
long if they can be released earlier; but in this case the table_close
call occurs across a ReleaseSysCache() call, which is hardly of
significance. It's not like you have to wait for some other
transaction, or wait for I/O, or anything like that that would make it
measurable.

Locally it may not be as efficient, but it is a question, to follow the
good programming practices, among them, not to break anything, not to
block, and if it is not possible, release soon.
In at least one case, there will not even be a block, which is an
improvement.

Another version, that could, be, without using ERROR.

+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)) {
+        rel = table_open(object->classId, RowExclusiveLock);
+    CatalogTupleDelete(rel, &tup->t_self);
+        table_close(rel, RowExclusiveLock);
+    ReleaseSysCache(tup);
+       }
+       else
+ elog(LOG, "cache lookup failed for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+ }
+ 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))
+    CatalogTupleDelete(rel, &tup->t_self);
+       else
+ elog(LOG, "could not find tuple for %s %u",
+ get_object_class_descr(object->classId), object->objectId);
+
+    systable_endscan(scan);
+    table_close(rel, RowExclusiveLock);
+ }
+}
+

regards,
Ranier Vilela

Only as a homework, is this function completely useless?

ItemPointerData
systable_scan_next(Relation heapRelation,
Oid indexId,
bool indexOK,
Snapshot snapshot,
int nkeys, ScanKey key)
{
SysScanDesc scan;
HeapTuple htup;
ItemPointerData tid;

scan = systable_beginscan(heapRelation, indexId, indexOK, snapshot,
nkeys, key);
htup = systable_getnext(scan);
if (HeapTupleIsValid(htup))
tid = &htup->t_self;
else
tid = NULL;
systable_endscan(scan);

return tid;
}

regards,
Ranier Vilela

On 2020-05-04 20:57, Peter Eisentraut wrote:

New patch attached. I'll park it until PG14 opens.

committed

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-05-05 18:05, Robert Haas wrote:

This reminds me: I think that the issues in
/messages/by-id/CA+TgmoYaFYRRdRZ94p_Qdt+1oONg6sMOvbkGHKVsFtONCrFkhw@mail.gmail.com
should be considered here - we should guarantee that there's a
snapshot registered continuously from before the call to
SearchSysCache1 until after the call to CatalogTupleDelete. In the
systable_beginscan case, we should be fine as long as the
systable_endscan follows the CatalogTupleDelete call.

I considered this, but it seems independent of my patch. If there are
changes to be made, there are now fewer places to fix up.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services